-
Notifications
You must be signed in to change notification settings - Fork 8
Standardize pyproject and relax dependencies #54
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
Conversation
|
Ah, it looks like there's some CI changes that need to be made too. I'll take a look into these. |
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.
Thanks for this work! It's a good change.
The general summary is see if it works to remove upper bounds but keep lower ones.
|
Did some testing here with the matrix: https://github.com/acmiyaguchi/derivative/pull/1/checks. Just needed to bump the poetry version to pick up the new changes for pep 621 support. You're right that it's mostly the upper bounds on the packages/python that are causing issues, so I've added those back in. |
Jacob-Stevens-Haas
left a comment
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.
LGTM, just a few things to do to make setuptools_scm work right:
- make version dynamic metadata
- make setuptools_scm write the version in the correct place
- Delete
derivative/__version__.pyand gitignore it.
|
I imagine the version works via release tags? This seems to work locally, but I haven't used |
|
Yes, exactly, it works via git and mercurial tags, then adds the number of commits, the current commit, and potentially a save date if there are uncommitted changes. CI failures occur because accepting dynamic version metadata is optional and poetry has opted out. You can probably just rip out the poetry bit from CI and use pip to create the environment. If environment caching gives you trouble, it's probably fine to rip that out, too. |
|
@Jacob-Stevens-Haas sorry for the delay, this totally slipped my mind. Fwiw in my team at work we use https://github.com/mtkennerly/poetry-dynamic-versioning which seems to be the poetry equivalent to setuptools-scm. Anyways I did some testing locally and https://github.com/acmiyaguchi/derivative/actions/runs/18738960223 tests seem to go, I haven't tried docs or the publishing workflow, not sure what the best way to test those are on my end. |
|
No worries, man, I'm just grateful for the PR. @andgoldschmidt can you allow CI from forks in project settings? I don't think there's much a risk of malicious PRs using up the free CI allotment. |
|
@Jacob-Stevens-Haas The lowest tier was "new to github and first time contributor", which is now applied---we should be good with this, but let me know if not. Also added some missing basic branch protections. @acmiyaguchi this is great, thanks for the contribution! |
Jacob-Stevens-Haas
left a comment
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.
Great, LTGM!
|
Thanks! Feel free to squash the changes if you decide to merge it in as-is, commit history always get iffy when mucking with CI. |
I'm working on trying to relax dependencies on a downstream library called pykoopman (https://github.com/dynamicslab/pykoopman) because it forces older versions of libraries like torch and scipy onto my project. There's a few transitive dependencies that I tracked here.
I ran the following:
This seems to work fine, so I think it might be favorable to relax dependencies until there are actual incompatibilities with upstream packages.