-
Notifications
You must be signed in to change notification settings - Fork 504
UV <---> Pip must stay in sync. #1264
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
base: v2.0-refactor
Are you sure you want to change the base?
Conversation
resolution of the dependency-groups from Pep 735 that uv enables.
Greptile OverviewGreptile SummaryThis PR introduces a pre-commit hook and script to automatically synchronize
Important Files ChangedFile Analysis
|
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.
3 files reviewed, 1 comment
test/ci_tests/sync_deps.py
Outdated
| # limitations under the License. | ||
|
|
||
|
|
||
| #!/usr/bin/env python3 |
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.
style: Shebang line should be at line 1 to work when executing the script directly. Currently it's after the license header, which means ./sync_deps.py won't work, only python sync_deps.py. This is fine since the pre-commit hook calls it via python, but worth noting for consistency.
Now, we allow unbare external imports anywhere. But the import list is smaller, and more easily managed with uv / pip equally. extras are now chained and built in the optional deps.
| "zarr>=2.14.2", | ||
| "jaxtyping", | ||
| {include-group="nn"}, | ||
| "nvidia-physicsnemo[utils-extras]", |
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.
No natten? 😅
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.
We should add it! But unless it's easy to install cross-platform, it needs to go into an optional dep.
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.
nn-extras here is optional right? Natten should def be optional regardless, but I think nn-extras is the right place for it
| healpix = [ | ||
| "earth2grid", | ||
| ] | ||
|
|
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.
It seems the gnns and healpix here are more just general deps groups associated with certain functionality/use-cases but not a part of the base pnm subpackages (nn, models, datapipes ...). Maybe separate them out with a small comment header for clarity? Like
# Use-case Specific Dependency Groups
or something...
|
|
||
| # Earth2 grid is not on pypi .... | ||
| [tool.uv.sources] | ||
| earth2grid = { url = "https://github.com/NVlabs/earth2grid/archive/main.tar.gz" } |
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.
May want to look at the earth2studio toml to see the various things Nick added to handle earth2grid...I think there is also a need to include --no-build-isolation
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.
Relevant lines appear to be:
no-build-isolation-package = ["earth2grid", "flash-attn", "torch-harmonics"]
[tool.uv.extra-build-dependencies]
earth2grid = ["setuptools", "torch"]
[tool.uv.sources]
...
earth2grid = { git = "https://github.com/NVlabs/earth2grid.git", rev = "661445e2c68edc76f52632aa0528af482357f1b8" }
I can add the missing pieces, though I suspect we don't need to lock to a revision.
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 need to double check with Nick when he's back why he pinned version, it may have just been out of "abundance of caution" :)
This PR is about dependencies and installation. Goals:
To this end, the core deps are listed, and there is linting against them. We can't import non-core deps "bare" any more.
Further, optional deps are almost* entirely protected with
check_version_spec, etc., to raise runtime errors at first use only. Meaning, a missing dep will not break the rest of the code, and won't even crash until a user attempts to use it. And I've tried to list out the optional deps per package, too, though some could use versioning.Everything below is outdated. I left it to show the original design.
Ensure that the pip dependency group exclusively matches the resolution of the dependency-groups from Pep 735 that uv enables.
Requirements are to ensure
pip install .still works, with or without uv, even though we want uv to be the sole source of truth. Rather than maintaining two dep lists, especially since one is nested, I made a pre-commit step to ensure that the pip requirements is always exactly what dependency-groups specifies.Basically, the workflow is this:
test/ci_tests/sync_deps.pyphysicsnemodependency group (no extras) and resolves all the groups and dependencies.pyproject.tomland fail pre-commit.This should achieve the following:
pip install .dependencies also update.PhysicsNeMo Pull Request
Description
Checklist
Dependencies
Review Process
All PRs are reviewed by the PhysicsNeMo team before merging.
Depending on which files are changed, GitHub may automatically assign a maintainer for review.
We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.
AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.