Switch to poetry for dependency management #208
Conversation
There was a problem hiding this comment.
Nice! 🚢
Out of curiosity, did the act tool work well to test the actions? If so, it's worth adding a comment with the specific act command you have used. Similar to what we did here: https://github.com/unitaryfund/metriq-gym/blob/8f556ac7d94257a2605203aecfe8f026aafadd39/.github/workflows/test.yml#L2
| source /venv/bin/activate && \ | ||
| ./benchmarks/scripts/run_benchmarks.sh 8 && \ | ||
| python ./benchmarks/scripts/plot_avg_benchmarks_over_time.py && \ | ||
| python ./benchmarks/scripts/plot_latest_benchmarks.py | ||
| poetry run ./benchmarks/scripts/run_benchmarks.sh 8 && \ | ||
| poetry run python ./benchmarks/scripts/plot_avg_benchmarks_over_time.py && \ | ||
| poetry run python ./benchmarks/scripts/plot_latest_benchmarks.py |
There was a problem hiding this comment.
Nice that you can use poetry run to run bash scripts!
| # Install Poetry | ||
| RUN pip3 install poetry==2.0.1 |
There was a problem hiding this comment.
Any reason not to use the "official installer" here?
There was a problem hiding this comment.
Only (minor reason) was to avoid needing to apt-get curl in the dockerfile. But admit thats a minor point. Happy to change if that's better.
There was a problem hiding this comment.
avoid needing to apt-get curl in the dockerfile
ahh yeah true. I guess if it works, then 🤷🏻. I was just hesitant about using pip to install poetry since it's not on their docs, but I guess pipx (which is documented) is just pip with some fancy virtual env management.
| "License :: OSI Approved :: GNU General Public License v3 (GPLv3)", | ||
| "Operating System :: OS Independent", | ||
| ], | ||
| python_requires=">=3.12", # Python version required |
There was a problem hiding this comment.
Let's make sure we carry python_requires over to the pyproject.toml: docs.
It seems like a few others may have gotten dropped as well, such as the description and classifiers.
There was a problem hiding this comment.
what does python_requires add over https://github.com/unitaryfund/ucc/pull/208/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R12 ?
There was a problem hiding this comment.
Ah good point, I missed that. I don't think I was looking for "python" as a project dependency of a python project. It looks like that does the job! Just wanted to make sure we had it somewhere that python 3.12 is required.
There was a problem hiding this comment.
Yes, I think this would cover it. But you have a good point @natestemen. I'm realizing this is the "legacy" format of pyproject.toml for poetry. As you note, the current version is https://packaging.python.org/en/latest/guides/writing-pyproject-toml/ (and can compare to what poetry new <project> would create. The new format is what I used in the #209 version with uv. I'll look to rewrite this to use that format.
Yes I did, was very easy, thanks for the pointer. Will add the reference. |
Use updated pyproject.toml syntax to specifiy project requirements. Add comment to link to act for testing github actions
|
Will do @Misty-W ! Is there a preference between merge commit versus rebase commit? |
I vote for rebase (but not a hill I'd die on)! I think whatever we choose, let's make it the only option for merging PRs in the future. |
|
Awesome -- I'm down with rebase too and more how I've worked in the past. Doing that now. |
|
I switched the repo settings to only allow rebase and merge. |
Addresses #78 by switching to poetry for dependency management. I tried separating into a few logical commits to make it easier to review.