-
Notifications
You must be signed in to change notification settings - Fork 399
feat: remove input-hash for PyPI dependencies #5174
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: main
Are you sure you want to change the base?
Conversation
baszalmstra
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.
Nice! This is a good start but its not enough, we need to also compare the requirements, required python version optional dependencies, etc. Furthermore, some fields can be dynamic which means we need to invoke python, and we might also have to deal with setup.py only sources.
Indeed! It might be good to let Claude explore the uv codebase as well for inspiration, although if I recall their satisfiability check is less extensive, they regenerate more often. |
76d5042 to
2b7191e
Compare
2b7191e to
08b5efe
Compare
|
Attaching benchmarks here: Dynamic Metadata (minimal-project with hatch_build.py, setup-project with setup.py)Main branch:
This branch:
Static Metadata (minimal-project without hatch_build.py)Main branch:
This branch:
|
| pub struct EnvironmentBuildCache { | ||
| /// Lazily initialized build dispatch dependencies (interpreter, env, etc.) | ||
| pub lazy_build_dispatch_deps: LazyBuildDispatchDependencies, | ||
| /// Optional conda prefix updater (created during satisfiability checking) | ||
| pub conda_prefix_updater: OnceCell<CondaPrefixUpdater>, | ||
| /// Cached registry client (with Online connectivity) | ||
| pub registry_client: OnceCell<Arc<RegistryClient>>, | ||
| /// Cached index locations | ||
| pub index_locations: OnceCell<IndexLocations>, | ||
| /// Cached build options | ||
| pub build_options: OnceCell<BuildOptions>, | ||
| /// Cached flat index (populated lazily when needed, async initialization) | ||
| pub flat_index: AsyncOnceCell<FlatIndex>, | ||
| /// Cached dependency metadata | ||
| pub dependency_metadata: OnceCell<DependencyMetadata>, | ||
| } |
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.
I'm not sure we need to cache most of these structs, as the most interesting things are chached to disk. I think there are non in-memory caches, only the InFlight which is not here and maybe in the RegistryClient. I'd rather not introduce another struct containing too many OnceCell's. Maybe we can figure out what we need to keep? I think the LazyBuildDispatch is something we should cache?
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.
removed all other fields except conda_prefix_updater. I think it's ok to keep it, because it contains an in-memory result ( OnceCell field) of installing a prefix environment.
| "**/*.ipynb", | ||
| "**/docs_hooks.py", | ||
| "scripts/test_native_certs.py", | ||
| "examples/dynamic-source-deps/minimal-project/hatch_build.py", |
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.
this will be removed when we remove dynamic-source-deps
| @@ -0,0 +1,19 @@ | |||
| [workspace] | |||
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's added just to show how to test it
|
Test plan:
If all of these cases succeed next to the integration and unit tests I would say it is ready to merge, I did a similar thing for the conda input-hash removal. |
|
added some stuff to cache reading static metadata, these are the new results ( with cache clean on every run ) Dynamic Metadata (minimal-project with hatch_build.py, setup-project with setup.py)Main branch:
This branch:
Static Metadata (minimal-project without hatch_build.py)Main branch:
This branch:
|
| if !current.is_version_dynamic | ||
| && let Some(current_version) = ¤t.version | ||
| && &locked.version != current_version | ||
| { | ||
| return Some(MetadataMismatch::Version { | ||
| locked: locked.version.clone(), | ||
| current: current_version.clone(), | ||
| }); | ||
| } |
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.
This needs some extra logic to work correctly when switching between dynamic and non-dynamic versions.
| /// Cache for build-related resources that can be shared between | ||
| /// satisfiability checking and PyPI resolution. | ||
| #[derive(Default)] | ||
| pub struct EnvironmentBuildCache { |
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 name it something with PyPI to differentiate from pixi build?
|
Might be good to user-test if |
Replace hash-based validation for path-based PyPI packages with metadata-based validation. Instead of computing and storing a hash of pyproject.toml, setup.py, and setup.cfg, we now:
This approach is similar to how PR #5011 removed input_hash for conda packages. The metadata comparison is more explicit about what changed (name vs version) rather than just indicating "something changed via hash mismatch".
Changes:
Description
Fixes #{issue}
How Has This Been Tested?
AI Disclosure
Tools: {e.g., Claude, Codex, GitHub Copilot, ChatGPT, etc.}
Checklist:
schema/model.py.