-
Notifications
You must be signed in to change notification settings - Fork 81
Build with uv, hatchling, updated CI/pre-commit and SPEC 0 compliant package versions #416
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
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
BorisMuzellec
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.
Thanks a lot @maltekuehl for this PR, that's amazing!
It's looking good. Re the last point I'm happy to follow Astral's recommendation to push the lock file, but in that case I think we should indeed test against latest dependencies using the --upgrade flag to make sure we anticipate future changes and deprecations.
Happy to merge after that!
|
I don't think that a library like PyDESeq2 needs a lock file. It's not an app that is deployed on the web that needs fixed versions. All scverse packages are also tested without a lock file. We even have a pre-release CI job so that we test against pre-releases. In my humble opinion, the lock file will either:
(this PR is great!) |
|
Thanks for the feedback to both @Zethson and @BorisMuzellec. Apologies for no progress on this issue, currently on holidays with limited internet and compute access. Will fix the |
|
Fixed! Ready to merge from my side now, please let me know if there's anything I missed. |
BorisMuzellec
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.
Thanks a lot @maltekuehl for this PR and @Zethson for your comments! I hope I didn't miss anything because this PR is quite big but I think it's ready to be merged.
|
I also checked the tutorial scripts from the docs to have another layer apart from tests to make sure there was no accidental change. So at least for common paths, I have high confidence and everywhere else, I tried to keep changes to a minimum. The typing fixes also should have addressed some minor bugs outside the standard workflows. Thanks both for the feedback and for merging. Will try to adapt the pytximport scaling PR to not have merge conflicts as well, but currently a bit swamped (as usual) so might take some weeks. If you have any comments and guidance on that one already, I would be happy to address them when addressing the merge conflicts. |
Reference Issue or PRs
Discussed with @BorisMuzellec in #412.
What does your PR implement? Be specific.
uvfor package management andhatchlingfor building, similar to mostscversecore packages.pre-commitmypyerrors that previously went undetected (this is the reason that many files are affected in this PR)ruffandmypychecks, too, and to test building the packageblackas it has been superseded byruffand the oldsetup.pyinstallation, which now works throughpyproject.tomluvThere is some debate about whether to commit
uv.lockfiles, Astral's position is YES for full reproducibility, but if you would prefer this to not be committed, please let me know. Downside is that if this is committed, the CI does not test against the latest version of all dependencies but against the locked one, unless--upgradeis passed. Passing this option would also be a possible solution to this question.