-
Notifications
You must be signed in to change notification settings - Fork 324
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
[py-tx] Migrate setup.py to pyproject.toml for modern packaging #1746
base: main
Are you sure you want to change the base?
Conversation
37c5d75
to
ae1a6ed
Compare
Aware it's not passing; working on it.. |
86cffb0
to
d427439
Compare
@Dcallies Ready to take a look now? Linting error aside.. I don't want to touch it bc usually it results in mypy getting more mad lol Then we can version bump I'm guessing? |
Err... caused more stuff to break when attemping to fix types. Gonna skip that |
Sorry, been traveling! Looking now. |
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.
Hmm, the rollout of this might be tricky, I believe you can dry run the packaging publishing steps, but it might be up to me to test them against the test pypi instance.
I have some blocking questions inline, and my toplevel one is mostly about how we can test that packaging still works. I have the credentials for the pypi instances and can test them if we need to.
Thanks for giving this a try!
python-threatexchange/pyproject.toml
Outdated
|
||
[tool.hatch.version] | ||
path = "version.txt" | ||
pattern = "^(?P<version>[\\d.]+)$" |
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.
blocking: I don't think this will correctly capture our version strings, which are in \d+.\d+.\d+ form.
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.
Yep, changed to "^(?P<version>\\d+\\.\\d+\\.\\d+)$"
python-threatexchange/pyproject.toml
Outdated
[project] | ||
name = "threatexchange" | ||
dynamic = ["version"] | ||
description = "Python Library for Signal Exchange" |
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.
The old code currently loads a long description (shown on the pypi) from DESCRIPTION.rst and README.md, will we need that for this to retain behavior with pypi?
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.
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.
Ended up using the rst one... as that was probably the intention
extras_require = {} | ||
|
||
for extension_dir in extensions_dir.iterdir(): | ||
requirements = extension_dir / "requirements.txt" |
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.
Did you go through these and include them? Or do we have options to make these partially dynamic (I don't know which is better).
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.
Yeah I did;
extensions = [
"vpdq",
"py-tlsh",
"pdfminer.six",
"pytesseract",
]
Are the four ones. I think it's easier to state them outright instead of finding them
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.
What do you recommend we do with the requirements.txt in those underlying extensions? Should we delete them?
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.
Maybe leave it for now? I don't think anything else uses it though
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.
Okay, I think the next steps are on me to figure out how to test the package building, and make sure the produced package is still installable. Let me patch you and see if I can figure that out - will aim to do it tomorrow.
I did a quick google search and apparently testpypi is a thing - maybe that helps |
Summary
Migrate setup.py to pyproject.toml for modern packaging!
Refactor away the setup.py file in the main py-tx project to adopt pyproject.toml format.
Used hatchling though.. not sure if that is best
Closes #1611!
Test Plan
Ran
python3 -m pip install -e .
; everything looks good I think..