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

Switch PyProject to Poetry #2111

Merged
merged 30 commits into from
Jun 9, 2024
Merged

Conversation

ppfeister
Copy link
Member

@ppfeister ppfeister commented May 8, 2024

Continuation of #2036

Package info

Sherlock Project added as Author
Homepage URL set to the github[.]io homepage (PyPi homepage already set, but not pip show)
Dynamic versioning fixed (pulled from sherlock[.]py)
Maintainers added
Classifiers added
Keywords added
Description added
etc


Refocused PR : PrProject being merged elsewhere to expedite package review process and resolve dependency issues (it takes time). This PR is now only to discuss switching to Poetry. (main pr #2116)

::: >>> Jump to start of refocused discussion <<< :::

@ppfeister ppfeister marked this pull request as ready for review May 8, 2024 07:38
Copy link
Collaborator

@matheusfelipeog matheusfelipeog left a comment

Choose a reason for hiding this comment

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

Just a comment to start a discussion:

I personally use the Poetry package manager instead of directly using pip for projects like this. It has a slightly different format from what setuptools accepts; you can view it here: https://python-poetry.org/docs/pyproject/

I find its dependency resolution mechanism better, with features that prevent the addition of dependencies that are not compatible with others in the project programmatically.

Since in the future we intend to add Sherlock to PyPI, Poetry can also help with that, as it has native build and publish mechanisms, eliminating the need to use other tools like twine.

For the average user, nothing would change; pip would work normally.
For development, well, it has all these features I mentioned above and a few more.

What do you think about this?


Refs:

@ppfeister
Copy link
Member Author

ppfeister commented May 8, 2024

Personally indifferent. No reservations or preference either way.
As long as normal pip usage isn't broken, then additional package manager support can only really be a plus.

As someone who hasn't used poetry, would this be as simple as making the pyproject compatible? Misread. It does in fact appear that simple.

@ppfeister
Copy link
Member Author

So, I may be wrong on this, but it seems that poetry does not support dynamic versioning OOTB.

The package version will either have to be manually updated each time, or it would require an additional plugin that versions based on git tags (which sherlock doesn't currently use). Thoughts?

@ppfeister
Copy link
Member Author

ppfeister commented May 8, 2024

It also doesn't seem that poetry has support for dynamic loading of dependencies -- they would need to be explicitly listed in the pyproj. Thoughts there?

Seems finicky on my end. I'm able to install with poetry right now, but unable to uninstall it with poetry (need to use pip). I'm also unable to run sherlock via sherlock due to an odd import error, needing to run it with poetry run sherlock instead. I'm sure both problems can be solved, just haven't seen why yet.

Latest commit cb22902 is what I have for poetry.

@sdushantha sdushantha requested a review from matheusfelipeog May 9, 2024 21:25
@ppfeister ppfeister changed the title Sherlock Packaging (PyProject) Switch PyProject to Poetry May 11, 2024
@ppfeister ppfeister requested a review from sdushantha as a code owner May 16, 2024 03:17
@ppfeister
Copy link
Member Author

ppfeister commented May 16, 2024

Rebased onto master to fix conflict (some ~183 behind).

Poetry rewritten in a way that actually works.

Discovered that the current version of pyproject has some ~undesirable~ install issues, namely it doesn't install the src into a named directory and dumps them straight into site-packages for some unknown reason. Not only is this not pretty, it can cause some conflicts. This version fixes that behavior.

Due to how poetry handles packaging, the imports had to be rewritten.

Due to how the imports were rewritten, the (already poorly located) unit tests no longer function as desired.

Unit tests were relocated to the root where they probably should be anyways. Unit tests work from this location with modified imports.

**** PR action currently uses the old unit test location, so failure here is expected. ****

GitHub Actions for PRs will be fixed in another PR. I'll probably take advantage of that fixup to switch from the unittest module to tox, which is a bit more portable in my opinion.

This also removes the requirements.txt file, as dependencies should now be listed within the pyproj. If some sort of workflow requires the requirements.txt file, poetry itself can dump it from the pyproj.

Removed the setup.py and setup.cfg items as they are no longer necessary with the new layout.

Removed the rpm spec file.

Dynamic versioning still needs to be set up.

@ppfeister
Copy link
Member Author

(a couple minor doc updates included, i.e. pyproj readme logo and capitalization)

@ppfeister
Copy link
Member Author

Dynamic versioning added but not yet enabled. Relies on named tags and applies throughout Sherlock, rather than pulling from src.

ppfeister added a commit to ppfeister/pkg that referenced this pull request May 18, 2024
Fixes conflict caused by switch to Poetry in sherlock-project/sherlock#2111
ppfeister and others added 3 commits May 18, 2024 02:22
This reverts commit 606743b.
Co-authored-by: Matheus Felipe <matheusfelipeog@gmail.com>
@ppfeister ppfeister requested a review from matheusfelipeog May 18, 2024 22:02
sherlock/sherlock.py Show resolved Hide resolved
Copy link
Member

@sdushantha sdushantha left a comment

Choose a reason for hiding this comment

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

LGMT

@ppfeister ppfeister self-assigned this May 24, 2024
@ppfeister ppfeister dismissed matheusfelipeog’s stale review May 31, 2024 18:44

Proceeding with careful review to fix issues downstream

@ppfeister ppfeister closed this in 3211b90 Jun 9, 2024
@ppfeister ppfeister merged commit b8ca8aa into sherlock-project:master Jun 9, 2024
1 of 3 checks passed
@ppfeister ppfeister deleted the feature/pyproj branch June 10, 2024 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants