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

Allow prereleases, locals, and URLs in non-editable path requirements #2671

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Mar 26, 2024

Summary

This PR enables the resolver to "accept" URLs, prereleases, and local version specifiers for direct dependencies of path dependencies. As a result, uv pip install . and uv pip install -e . now behave identically, in that neither has a restriction on URL dependencies and the like.

Closes #2643.
Closes #1853.

@charliermarsh charliermarsh added the enhancement New feature or request label Mar 26, 2024
@charliermarsh charliermarsh force-pushed the charlie/pre-local branch 5 times, most recently from 6874362 to f3574db Compare March 26, 2024 17:59
@charliermarsh charliermarsh marked this pull request as ready for review March 26, 2024 18:00
@charliermarsh
Copy link
Member Author

We could in theory use the same technique to resolve these recursively, upfront, for all direct URL distributions...

@zanieb zanieb self-requested a review March 26, 2024 18:06
@charliermarsh charliermarsh force-pushed the charlie/pre-local branch 2 times, most recently from e45f9f1 to 065fdc1 Compare March 26, 2024 18:08
metadata.requires_dist.iter().filter(|requirement| {
requirement.evaluate_markers(markers, &editable.extras)
})
}))
Copy link
Member Author

Choose a reason for hiding this comment

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

Going to DRY this up in a separate PR (it's already repeated excessively on main).

@charliermarsh
Copy link
Member Author

A slightly different way to solve this would be to pass these paths to SourceTreeResolver, and then include the requirements as first-party requirements directly. This would be ok but I was worried about how it might impact error messages.

///
/// The lookahead resolver resolves requirements for local dependencies, so that the resolver can
/// treat them as first-party dependencies for the purpose of analyzing their specifiers.
pub struct LookaheadResolver<'a> {
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 alternative solution would be to pass any source trees to SourceTreeResolver (so, e.g., if the user does foo @ ./bar/baz, we'd pass ./bar/baz to SourceTreeResolver alongside any pyproject.toml or setup.py files that were passed in). Then, we'd treat the requirements as part of the requirements input to the resolution.

That approach would require some refactoring of SourceTreeResolver (since it uses ExtrasSpecification, which isn't a generic concept for requirements). It would also mean that the actual Resolver wouldn't be able to distinguish between these "lookahead" requirements and actual first-party requirements, and there may not be any indication in the error messages that they came from the local directory requirement, but that might be ok. I'm open to either.

Copy link
Member Author

Choose a reason for hiding this comment

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

\cc @zanieb

Copy link
Member

Choose a reason for hiding this comment

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

This seems okay, but the whole SourceTree and SourceTreeResolver concept is so new that I don't have strong feelings.

I like the idea of them being included in the resolver manifest.

@charliermarsh
Copy link
Member Author

Also, I think this should be recursive… so that local deps can define local deps at any level.

@charliermarsh
Copy link
Member Author

Thinking about it... we could actually extend this approach to enable support for transitive URL dependencies (as long as we don't allow URL dependencies to come from registry dependencies, which is fine)... The basic idea would be: recursively resolve all direct URL references, then the requirements of all direct URL references, etc. So we'd collect all possible direct URL references upfront.

) -> Result<Vec<RequestedRequirements>> {
let requirements: Vec<_> = futures::stream::iter(self.requirements.iter())
.map(|requirement| async { self.lookahead(requirement, context, client).await })
.buffered(50)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to put that value in a global constant?

@charliermarsh
Copy link
Member Author

I wonder if we should start by allowing this to be recursive for local requirements (for editables too).

@charliermarsh
Copy link
Member Author

(But it's fine to merge as-is. That would be an extension of this work.)

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

I do have some requested changes, but overall this makes sense to me.

///
/// The lookahead resolver resolves requirements for local dependencies, so that the resolver can
/// treat them as first-party dependencies for the purpose of analyzing their specifiers.
pub struct LookaheadResolver<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

This seems okay, but the whole SourceTree and SourceTreeResolver concept is so new that I don't have strong feelings.

I like the idea of them being included in the resolver manifest.

crates/uv/src/commands/pip_install.rs Outdated Show resolved Hide resolved
crates/uv/tests/pip_compile.rs Outdated Show resolved Hide resolved
crates/uv/tests/pip_compile.rs Outdated Show resolved Hide resolved
crates/distribution-types/src/requirements.rs Show resolved Hide resolved
@charliermarsh charliermarsh enabled auto-merge (squash) March 27, 2024 22:08
@charliermarsh charliermarsh merged commit cf30932 into main Mar 27, 2024
31 checks passed
@charliermarsh charliermarsh deleted the charlie/pre-local branch March 27, 2024 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants