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.

Sign up for GitHub

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

Jump to bottom

Add support for requirements files in uv run #4973

Merged
merged 4 commits into from
Jul 23, 2024
Merged

Add support for requirements files in uv run #4973

merged 4 commits into from
Jul 23, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Jul 10, 2024 •
edited by charliermarsh
Loading

Closes #4824.

@zanieb zanieb added cli Related to the command line interface preview Experimental behavior labels Jul 10, 2024
@zanieb zanieb force-pushed the zb/run-requirements branch 2 times, most recently from caecbd4 to 7a8b84f Compare July 10, 2024 18:38
@zanieb
Copy link
Member Author

zanieb commented Jul 10, 2024

Terrifyingly easy to add. I want to add some more test coverage and I'm sure there are some niche behaviors I need to explore.

zanieb
zanieb commented Jul 10, 2024
View reviewed changes
crates/uv-cli/src/lib.rs Outdated
///
/// If `-` is provided, then requirements will be read from stdin.
#[arg(long, short, group = "sources", value_parser = parse_file_path)]
pub requirement: Vec<PathBuf>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we say -r / --with-requirements instead?

@zanieb zanieb mentioned this pull request Jul 10, 2024
Merged
@zanieb zanieb changed the base branch from main to zb/override-satisfies July 11, 2024 17:46
@zanieb zanieb mentioned this pull request Jul 11, 2024
Closed
zanieb
zanieb commented Jul 16, 2024
View reviewed changes
crates/uv/src/commands/project/run.rs Outdated
Comment on lines 61 to 66
// These cases seem quite complex because it changes the current package — let's just
// ban it entirely for now
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, uv run -r could imply --isolated (which might make sense but is more limiting?)

Copy link
Member

Choose a reason for hiding this comment

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

Am I correct that the code would work, and here, you just mean that semantically -r foo/bar/pyproject.toml should use foo/bar as the project?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the code should work as-is, it's just weird since it may imply a change of project.

Copy link
Member Author

Choose a reason for hiding this comment

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

Like.. you should use --with ./foo/bar instead, right?

charliermarsh
charliermarsh approved these changes Jul 17, 2024
View reviewed changes
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Seems fine (it's good that it was easy!). Let's also add to uv tool run? What about uv tool install...?

@charliermarsh
Copy link
Member

Also closes #4824.

@zanieb
Copy link
Member Author

zanieb commented Jul 17, 2024

Yeah I think we need it everywhere. What do you think about names?

  • --with-requirements <path>
  • --with-requirements-file <path>
  • --requirements-file <path>
  • --requirements <path>

I think the --with prefix makes sense since it's important to note it's "adding" requirements alongside whatever else?

Per #4824 (comment) I think we want to support --override foo==1.0.0 directly so maybe the constraint and override options are:

  • --constraint <single>
  • --override <single>
  • --constraints <path>
  • --overrides <path>

Do we want:

  • --constraint-file / --override-file or
  • --constraints-path / --overrides-path?

Here I think it's reasonable to drop the --with prefix because we could apply these constraints and overrides to all project dependencies not just those being added on top?

@charliermarsh
Copy link
Member

We don't accept --override foo==1.0.0 in the pip API -- is it necessary to support here? It seems fine to me to be consistent with the pip API on those.

@charliermarsh
Copy link
Member

Or, at least, we definitely don't need to support that to start IMO. And if we want to support it, we should probably support it in the pip API too.

@zanieb
Copy link
Member Author

zanieb commented Jul 17, 2024

Yeah I don't think that's in scope for this pull request, just want to nail down the plan so we can make sure it's coherent.

@charliermarsh
Copy link
Member

I think I'd be ok with --requirements, --overrides, and --constraints (with the short options too) -- it just feels the most intuitive coming from the other APIs. --with-* is also ok, but we'd probably end up wanting aliases for the shorter versions?

@charliermarsh
Copy link
Member

@zanieb - I'm happy to finish this PR if we can get consensus on the names, no trouble.

@zanieb
Copy link
Member Author

zanieb commented Jul 18, 2024

@charliermarsh I am comfortable finishing it up this week

@zanieb zanieb mentioned this pull request Jul 19, 2024
Closed
@zanieb zanieb marked this pull request as ready for review July 19, 2024 18:05
@zanieb zanieb changed the base branch from zb/override-satisfies to main July 19, 2024 20:40
charliermarsh
charliermarsh reviewed Jul 20, 2024
View reviewed changes
crates/uv/src/commands/project/run.rs Outdated
@@ -281,6 +310,7 @@ pub(crate) async fn run(
// any `--with` requirements, and we already have a base environment, then there's no need to
// create an additional environment.
let skip_ephemeral = base_interpreter.as_ref().is_some_and(|base_interpreter| {
// No additional requrements
Copy link
Member

Choose a reason for hiding this comment

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

Typo :D

charliermarsh
charliermarsh reviewed Jul 20, 2024
View reviewed changes
crates/uv/src/commands/project/run.rs
printer,
match spec {
None => Some(venv),
Some(spec) if spec.is_empty() => Some(venv),
Copy link
Member

Choose a reason for hiding this comment

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

You might be able to simplify this with a spec.filter(|spec| !spec.is_empty()) somewhere.

@charliermarsh
Copy link
Member

As discussed I'll take this over and try to get it merged today.

@zanieb
Copy link
Member Author

zanieb commented Jul 23, 2024

Note that we're using plural --constraints, --requirements and --overrides here to make room for the singular interface in the future. However, this differs from the uv pip interface which uses singular names to refer to files.

@charliermarsh charliermarsh changed the title Add support for requirements files, constraints, and overrides in uv run Add support for requirements files in uv run Jul 23, 2024
@charliermarsh
Copy link
Member

We decided to narrow the scope to requirements files for now. Constraints and overrides require that we resolve alongside the base environment, diff the result, and install any changes into the ephemeral environment. Which is doable, but a lot more complex.

zanieb
zanieb commented Jul 23, 2024
View reviewed changes
crates/uv-cli/src/lib.rs Outdated Show resolved Hide resolved
@charliermarsh charliermarsh enabled auto-merge (squash) July 23, 2024 16:44
zanieb
zanieb commented Jul 23, 2024
View reviewed changes
Copy link
Member Author

@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.

"Approved"

@zanieb
Copy link
Member Author

zanieb commented Jul 23, 2024

Thanks for taking this over!

@charliermarsh charliermarsh merged commit 5f1f9c8 into main Jul 23, 2024
50 of 54 checks passed
@charliermarsh charliermarsh deleted the zb/run-requirements branch July 23, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees
No one assigned
Labels
cli Related to the command line interface preview Experimental behavior
Projects
uv: release-ready
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--override not supported by uv run
2 participants
@zanieb @charliermarsh