-
Notifications
You must be signed in to change notification settings - Fork 537
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
Set pyproject.toml #3821
Set pyproject.toml #3821
Conversation
Hi @bigstones! Agree, poetry is much better and the best practice. If you have the time, the install script may need to be updated. And removing the Thanks for the contribution! I'll follow up next week. |
Hi @benjaminpkane! I will try to work on it as I have time this week. Thanks for your reply |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3821 +/- ##
===========================================
- Coverage 15.85% 15.65% -0.21%
===========================================
Files 731 728 -3
Lines 81886 80536 -1350
Branches 1093 1074 -19
===========================================
- Hits 12987 12609 -378
+ Misses 68899 67927 -972
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks for the updates! Changes look great 🚀 I will test tomorrow |
[build-system] | ||
requires = ["importlib-metadata; python_version<'3.8'", "setuptools", "wheel"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjaminpkane we still need this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 👍
Will need to update GitHub workflows as well
Will circle back to this after 0.23.0 is released. Pardon the delay! Want to make sure development will not be interrupted before merging |
Hi @bigstones. I think we can move to a |
I believe Poetry is beneficial for dynamic requires settings due to its support for managing virtual environments. As for the package_dir configuration, I'm aware that Poetry also provides support for it, and I'll look into it a bit more for confirmation. Thanks |
Ok, I will wait for your confirmation. The requirement is we need to ensure that one and only one of The
|
Thank you for your patience. I've reviewed the two points: It's confirmed that Poetry does not provide dynamic requires, and it seems unlikely to be supported in the future as well. You can find more details in the following link: python-poetry/poetry#8509. Additionally, I verified that installing FiftyOne with the poetry build command results in the installation of opencv-python-headless. To set the package_dir, it appears that a directory structure change is necessary. Currently, 'recipes' and 'tutorials' are located under 'docs/source,' and it seems that they need to be moved to 'docs/source/fiftyone.' More information can be found in the following links: https://python-poetry.org/docs/pyproject/#packages and python-poetry/poetry#6258. I've made the code changes and pushed them. If you encounter any issues, please let me know. Thank you. |
Thanks @bigstones. It would be ideal if we could use poetry, but without the ability install a correct Moving to a |
Oh I see, I will modify the project to maintain the necessary files from setup.py while also making them compatible with pyproject.toml. |
Hi @bigstones. I've done a fair amount of investigating to see how we could use poetry but it doesn't seem like a good fit right now. I've pushed a new branch named hatch that includes your This new setup has actually reduces the build and source distribution sizes and makes things a lot simpler to manage! 🙇 🚀 Note that when python-poetry/poetry#3332 is resolved, poetry can likely be used to manage a development install of fiftyone. With pip install fiftyone --no-build-isolation --no-binary fiftyone,voxel51-eta Note that using |
Hi @benjaminpkane , thank you for your answer. I'm glad to learn about the Hatchling library. As it's a library I'm encountering for the first time, it seems necessary to conduct more tests. I think I'll have some time to do this around next year. Wishing you a lot of happiness in the New Year, and I'll be returning soon. |
Hi @bigstones. I think I have most of the hatchling work completed. Will merge this and finish it up! Thanks! |
What changes are proposed in this pull request?
(Please fill in changes proposed in this fix)
I think it easier to configure the environment with poetry.
I created a pyproject.toml file based on my setup.py file.
How is this patch tested? If it is not, please explain why.
(Details)
$ poetry shell
$ poetry update
$ poetry install
$ python
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
No. You can skip the rest of this section.
Yes. Give a description of this change to be included in the release
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
If you want to set up a development environment, you can do it easily with poetry.
What areas of FiftyOne does this PR affect?
App: FiftyOne application changes
Build: Build and test infrastructure changes
Core: Core
fiftyone
Python library changesDocumentation: FiftyOne documentation changes
Other