Skip to content
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

Add support for extras in editable requirements #1531

Merged
merged 2 commits into from
Feb 16, 2024
Merged

Conversation

charliermarsh
Copy link
Member

Summary

If you're developing on a package like attrs locally, and it has a recursive extra like attrs[dev], it turns out that we then try to find the attrs in attrs[dev] from the registry, rather than recognizing that it's part of the editable.

This PR fixes the issue by making editables slightly more first-class throughout the resolver. Instead of mocking metadata, we explicitly check for extras in various places. Part of the problem here is that we treated editables as URL dependencies, but when we saw an extra like attrs[dev], we didn't map that back to the URL. So now, we treat them as registry dependencies, but with the appropriate guardrails throughout.

Closes #1447.

Test Plan

  • Cloned attrs.
  • Ran cargo run venv && cargo run pip install -e ".[dev]" -v.

@charliermarsh charliermarsh force-pushed the charlie/editable branch 2 times, most recently from c43865b to 8871bc2 Compare February 16, 2024 19:27
// Mock editable responses.
let package_id = dist.package_id();
index.distributions.register(package_id.clone());
index.distributions.done(package_id, metadata.clone());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing this is good.

@@ -633,6 +625,16 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
}

PubGrubPackage::Package(package_name, extra, None) => {
// If the dist is an editable, return the version from the editable metadata.
if let Some((_local, metadata)) = self.editables.get(package_name) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only downside to all of this is that we now pay the cost of doing these lookups for all packages.

@charliermarsh charliermarsh requested a review from zanieb February 16, 2024 19:31
@charliermarsh charliermarsh added the bug Something isn't working label Feb 16, 2024
@matthewfeickert
Copy link

matthewfeickert commented Feb 16, 2024

Ah, thank you very much for this! I was just about to report it given

uv pip install --upgrade ".[all,test]"
...
error: Failed to parse `.[all,test]`
  Caused by: Expected package name starting with an alphanumeric character, found '.'
.[all,test]

. :)

(Go Astral team go! 🚀 ⭐)

@charliermarsh charliermarsh merged commit 6392961 into main Feb 16, 2024
7 checks passed
@charliermarsh charliermarsh deleted the charlie/editable branch February 16, 2024 23:48
@matthewfeickert
Copy link

@charliermarsh @zanieb we're still seeing this in https://github.com/scikit-hep/pyhf when we try to do

$ uv --version
uv 0.1.4
$ python -m uv pip install --upgrade ".[all,test]"
error: Failed to parse `.[all,test]`
  Caused by: Expected package name starting with an alphanumeric character, found '.'
.[all,test]
^
$

@charliermarsh
Copy link
Member Author

That’s a slightly different issue — you can either use -e (like -e “.[all,test]”) or provide a package name like “pyhf @ .[all,test]”. We have a limitation that we require package names upfront for non-editable requirements, but we’ll be lifting that in the future.

@matthewfeickert
Copy link

Ah okay great. I was alreadying watching Issue #313 but for some reason hadn't made the connection. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing support for recursive dep groups
3 participants