Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 🚀 introduce pyproject.toml file and remove old setup.py/cfg and req*.txt #110

Merged
merged 8 commits into from
Oct 17, 2023

Conversation

onuralpszr
Copy link
Contributor

@onuralpszr onuralpszr commented Sep 17, 2023

Hello, I see that project was using old setup.py style and I wanted to bring new toml style and also you can set "formatting rules" inside and next if this one accepted I would like to introduce pre-commit.ci so all of the formatters will be automatic and git-hook possible. (easy to attach thanks to pre-commit CLI)

For produce wheel/install/development I am using "poetry" for control toml file

Links:
https://pre-commit.ci/
https://python-poetry.org/
https://github.com/pre-commit/pre-commit

… req*.txt

Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
@mabulgu
Copy link
Member

mabulgu commented Sep 19, 2023

Looks great at first look @onuralpszr ! Thanks for the PR! I will review it soon.

BTW this is one of the things that I wanted to do but I dont have much time even to add an issue for it. I appreciate your contribution.

@onuralpszr
Copy link
Contributor Author

Looks great at first look @onuralpszr ! Thanks for the PR! I will review it soon.

BTW this is one of the things that I wanted to do but I dont have much time even to add an issue for it. I appreciate your contribution.

You're welcome, I am glad you like it. With these PR I can fix #104 #105 #106 very easily and also can introduce "ruff" as well.

@mabulgu
Copy link
Member

mabulgu commented Sep 19, 2023

Looks great at first look @onuralpszr ! Thanks for the PR! I will review it soon.
BTW this is one of the things that I wanted to do but I dont have much time even to add an issue for it. I appreciate your contribution.

You're welcome, I am glad you like it. With these PR I can fix #104 #105 #106 very easily and also can introduce "ruff" as well.

Nice! I had a draft trial PR here for that stuff (but without pyproject.toml which I realized its existence a lot later on) so your PR will be a good base for this one.

I see that your builds are failing. Can you check before I review your changes?

@onuralpszr
Copy link
Contributor Author

Looks great at first look @onuralpszr ! Thanks for the PR! I will review it soon.
BTW this is one of the things that I wanted to do but I dont have much time even to add an issue for it. I appreciate your contribution.

You're welcome, I am glad you like it. With these PR I can fix #104 #105 #106 very easily and also can introduce "ruff" as well.

Nice! I had a draft trial PR here for that stuff (but without pyproject.toml which I realized its existence a lot later on) so your PR will be a good base for this one.

I see that your builds are failing. Can you check before I review your changes?

Sure, I am checking CIs right now.

@onuralpszr
Copy link
Contributor Author

@mabulgu run the CI please :)

Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
@mabulgu
Copy link
Member

mabulgu commented Sep 19, 2023

@mabulgu run the CI please :)

Done. Also removed the silly rule for approval. Pls ping me if it pops up again tho, GH settings are generally complex.

@onuralpszr
Copy link
Contributor Author

@mabulgu run the CI please :)

Done. Also removed the silly rule for approval. Pls ping me if it pops up again tho, GH settings are generally complex.

LOL , Sure I will ping you, let me see what is complaining

@onuralpszr
Copy link
Contributor Author

onuralpszr commented Sep 19, 2023

@mabulgu oh btw, python 3.7 is EOL ? Can we drop it ? or do you really need it ?

For example isort asking for 3.8 minimum and so many other things not gonna be easy to install for py37

onuralpszr and others added 3 commits September 19, 2023 13:20
Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
Signed-off-by: Onuralp SEZER <thunderbirdtr@fedoraproject.org>
@onuralpszr
Copy link
Contributor Author

onuralpszr commented Sep 19, 2023

@mabulgu, I passed all the tests, but the 'click' behavior was causing the issue because the latest one has a different exit code. So, I had to revert for now, but it needs to be changed in the tests to comply with that. I thought I was missing something for a while, but then I understood that 'click' was the issue.

@onuralpszr onuralpszr force-pushed the pyproject-toml branch 5 times, most recently from 35c7cb1 to 29ff25b Compare September 20, 2023 07:34
@onuralpszr
Copy link
Contributor Author

onuralpszr commented Sep 20, 2023

@mabulgu ready for review and dropped python 3.7. Because too many dev deps needs to be adjusted and too many projects also dropped as well. I did not apply any formatting yet. If you want me do that, I can change all files for formatted versions but I don't wanna mix too commits with it.

PS : I wanted to say "friendly hello" with this way in PR :) Have a nice day 🚀

pyproject.toml Outdated
@@ -0,0 +1,117 @@
[tool.poetry]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onuralpszr I am just taking a quick look as I am doing a more detailed review. (Looks awesome btw). First question:

Why poetry?

As a seasoned dev I dont care if it is popular or not but how would you convince me to use this? I prefer using official stuff generally TBH and I wonder why you chose poetry? Pls skip its popularity and sell it to me in a way because I want to understand why we should use it in this project. The official stuff had been doing my job already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used poetry because I know how to use and handle plus I need something to handle "toml" file and my development setup. Poetry gives me "venv" generation and "poetry install" and all ready to use for me. After that "poetry shell" done. One big main reason is removal of "setup.py" and use "toml" and format is acceptable by official python because they have to comply that so that why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget the toml as you can use pyproject.toml without poetry right? pyproject.toml comes from a PEP but poetry doesnt. Since poetry is not a python official tool, I cannot rely on that project 100%. It is the same for click but click does sth related to business technicalities for me and if the official python offered me an easy cli tool I would not use click for example. I am giving the click example because you faced an exit code issue there for example. In the future we might face issues with poetry in other ways.

I dont know much about poetry so pls let me investigate and research this a bit. the project is not a rocket to the moon but I am an addict of minimalism and consistency so let me check first:)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example take this project: https://github.com/ansible/anonymizer/blob/main/pyproject.toml

This also uses a pyproject.toml but uses the official setuptools as far as I understand. So using poetry feel like: while it has many many features, it is 3rd party and we could do the same with the good old setup tools.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onuralpszr I took a look at the general poetry stuff and I think I dont want to put yet another dependency to the project. Click was different (and as you can see we have very small count of deps currently)

Would you mind converting your pyproject.toml and the project in this PR to use the setuptools (like before)?

As I said before this project might be a good sample: https://github.com/ansible/anonymizer/blob/2f2e428d1bb9b1cd2950bec6ef3ba03639ee6e5f/pyproject.toml#L1 (it also uses tox but don't mind that, I am not ready for tox yet:)) )

Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
@onuralpszr
Copy link
Contributor Author

@mabulgu if we going to control ALL of the formatting and etc via "pre-commit" for saving time in CI we can remove "flake" section, because pre-commit ci gonna be different CI and it is "soooo much faster" like 1-2 seconds and done. that means for CI test we can remove "poetry" and use "pip install ." plus install twine because twine is dev tool. This should be more minimalist approach

@onuralpszr onuralpszr force-pushed the pyproject-toml branch 14 times, most recently from 001f95c to cb1d4d1 Compare September 24, 2023 23:31
Signed-off-by: Onuralp SEZER <thunderbirdtr@fedoraproject.org>
@onuralpszr
Copy link
Contributor Author

onuralpszr commented Sep 24, 2023

@mabulgu I added cache for poetry so install time just become "1 second" as long as we change nothing on deps "cache" will stay which is a good thing because CI can work faster. Plus if we turn off flake in action (which I really really want to not use flake8 in action) I can speed so much more as well. for now top times now become 25-30 (previous was extra 10-15 seconds longer) I also updated gh-action versions for better support which handle warnings (check your old CIs you will see what I meant)

@mabulgu
Copy link
Member

mabulgu commented Sep 25, 2023

Probably I explained it in a wrong way. It is not only about timing here. It is just because of the YAGNI principle. If you really dont need sth you should not be using it so it was what I thought about venv. The timing was just a small side effect, nothing else.

Re: poetry cache - thanks for it, I think it will be useful

Re: flake check - even if we use it we should be checking stuff there as a validation because pre-commit can be skipped by uninstalling and anybody can commit a code without those checks. Lets keep it for now.

@onuralpszr
Copy link
Contributor Author

onuralpszr commented Sep 25, 2023

Re: flake check - even if we use it we should be checking stuff there as a validation because pre-commit can be skipped by uninstalling and anybody can commit a code without those checks. Lets keep it for now.

no you can't skip that.
Example repo with pre-commit (I setup before)
https://github.com/roboflow/supervision/commits?author=pre-commit-ci%5Bbot%5D

There will be "bot" will handle that.

@onuralpszr
Copy link
Contributor Author

Probably I explained it in a wrong way. It is not only about timing here. It is just because of the YAGNI principle. If you really dont need sth you should not be using it so it was what I thought about venv. The timing was just a small side effect, nothing else.

Re: poetry cache - thanks for it, I think it will be useful

Sure, I understand that but I made it better :)

Copy link
Member

@mabulgu mabulgu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onuralpszr after doing the following I think we can merge this PR:

Make will be used both ci/cd and dev process so it is important to reflect formatting, linting, building etc. here.

Thanks!

If you have any doubts you cannot do the reverting to setuptools because of time concerns pls let me know. I can take over your pr.

My best

@mabulgu
Copy link
Member

mabulgu commented Sep 29, 2023

Re: flake check - even if we use it we should be checking stuff there as a validation because pre-commit can be skipped by uninstalling and anybody can commit a code without those checks. Lets keep it for now.

no you can't skip that. Example repo with pre-commit (I setup before) https://github.com/roboflow/supervision/commits?author=pre-commit-ci%5Bbot%5D

There will be "bot" will handle that.

I think I missed this commetn of yours, sorry for that. When you uninstall precommit or not install it, you can simply skip it and commit things in the repo. So since it depends on the self-responsibility of the developer and how he/she reads the contributor guide, an extra direct check always is good.

@mabulgu
Copy link
Member

mabulgu commented Sep 29, 2023

@onuralpszr after doing the following I think we can merge this PR:

Make will be used both ci/cd and dev process so it is important to reflect formatting, linting, building etc. here.

Thanks!

If you have any doubts you cannot do the reverting to setuptools because of time concerns pls let me know. I can take over your pr.

My best

Regarding our conversation on linkedin: you said that you already applied the rules in my legacy draft pr, so pls skip that.

re: additions in the makefile - I can gather up the useful commands in the makefile later on so thats on me.

If you have doubts about removing poetry and using setuptools, pls let me know shortly so I can take this over and work on the required changes. Thanks!

Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
@onuralpszr
Copy link
Contributor Author

onuralpszr commented Oct 1, 2023

@mabulgu it is now setuptools with proper CI with dev tools and pip cache for speed up.

Minor request if possible, can you make this PR labeled "hacktoberfest-accepted" but in order to for that you need to add topic "hacktoberfest" into project page (you can click gear on next to so it will count as PR in HB event for me too.

@onuralpszr
Copy link
Contributor Author

@mabulgu friendly ping* :)

@mabulgu
Copy link
Member

mabulgu commented Oct 5, 2023

@mabulgu friendly ping* :)

I will re-check the PR as soon as possible. I have been quite busy recently so you need to bear with me a bit. Thanks for your patience!

Copy link
Member

@mabulgu mabulgu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!!

@mabulgu mabulgu merged commit 0322e77 into SystemCraftsman:main Oct 17, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants