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

Detect python version from python project by default in uv venv #5592

Merged

Conversation

Vigilans
Copy link
Contributor

@Vigilans Vigilans commented Jul 30, 2024

Summary

uv venv should support adopting python version specified in requires-python from pyproject.toml. This allows customization on the venv setup when syncing from python project.

Closes #5552.

It also serves as a workaround to close #5258.

Test Plan

  1. Run uv venv in folder with pyroject.toml specifying requries-python = "<3.10". Python 3.9 is selected for venv.
  2. Change to requries-python = "<3.11" and run uv venv again. Python 3.10 is selected now.
  3. Switch to a folder without pyproject.toml then run uv venv. Python 3.12 is selected now.

Comment on lines 148 to 168
let project = match VirtualProject::discover(
&std::env::current_dir().into_diagnostic()?,
&DiscoveryOptions::default(),
)
.await
{
Ok(project) => Some(project),
Err(WorkspaceError::MissingPyprojectToml) => None,
Err(WorkspaceError::NonWorkspace(_)) => None,
Err(err) => return Err(err).into_diagnostic(),
};

if let Some(project) = project {
interpreter_request = find_requires_python(project.workspace())
.into_diagnostic()?
.as_ref()
.map(RequiresPython::specifiers)
.map(|specifiers| {
PythonRequest::Version(VersionRequest::Range(specifiers.clone()))
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code here is adopted from run.rs:

match VirtualProject::discover(&CWD, &DiscoveryOptions::default()).await {
Ok(project) => Some(project),
Err(WorkspaceError::MissingPyprojectToml) => None,
Err(WorkspaceError::NonWorkspace(_)) => None,
Err(err) => return Err(err.into()),
}
};

and FoundInterpreter in mod.rs:
let requires_python = find_requires_python(workspace)?;
// (1) Explicit request from user
let python_request = if let Some(request) = python_request {
Some(request)
// (2) Request from `.python-version`
} else if let Some(request) = request_from_version_file(workspace.install_path()).await? {
Some(request)
// (3) `Requires-Python` in `pyproject.toml`
} else {
requires_python
.as_ref()
.map(RequiresPython::specifiers)
.map(|specifiers| PythonRequest::Version(VersionRequest::Range(specifiers.clone())))
};

@charliermarsh charliermarsh force-pushed the vigilans/uv-venv-detect-project-python-version branch from 81c2502 to c704aee Compare July 30, 2024 12:32
@charliermarsh
Copy link
Member

Can you add a test for this in tests/venv.rs?

@charliermarsh charliermarsh added configuration Settings and such preview Experimental behavior labels Jul 30, 2024
@T-256
Copy link
Contributor

T-256 commented Jul 30, 2024

When initializing python project, it is common to specify minimum version in pyproject.toml (e.g. requires-python = ">=3.8"). Within development of project, we need to have minimum supported environment to avoid using newest features that is unavailable in minimum required python version.

Is it make sense to have an option to change resolution strategy for python discovery? just like --resolution for dependencies resolver.

Example

pyproject.toml

[project]
requires-python = ">=3.8"

By using uv venv --python-resolution=lowest-minor it will select 3.8.19. (IIRC, this is default behavior of Rye?!)

@zanieb
Copy link
Member

zanieb commented Jul 30, 2024

Note this interacts with some work-in-progress at #5035; specifically the merged lines on my branch. I don't think that should block this change though.

We'll also need to consider the following in the future (no changes needed here):

@zanieb
Copy link
Member

zanieb commented Jul 30, 2024

I am tempted to agree that we should be selecting the lowest compatible version by default? This is tough to balance, as it's nice to get the performance and error handling improvements of a newer release. It may make sense to just expose a way to use the lowest compatible version instead (as suggested by @T-256) so it's easy to do in CI? However, I think in that case you'd want to actually guarantee you're getting the lowest version not the one nearest to your lower bound?

@T-256
Copy link
Contributor

T-256 commented Jul 30, 2024

I am tempted to agree that we should be selecting the lowest compatible version by default?

Another case for inconsistency: today if you specify requires-python = ">=3.10" and uv venv it will create 3.12 environment, but in next year when a contributor wants init an environment, it would be 3.13 which brings different development environments.
IMO, this is not a big concern since it could be solved by .python-version file. then I think we should be stricter on python version resolving and tip users to use (and also commit) .python-version file.

However, I think in that case you'd want to actually guarantee you're getting the lowest version not the one nearest to your lower bound?

Yes, it is makes sense to be strict on there. for example, with requires-python = ">=3.10" and installed python 3.12, it should download and use 3.10. in cases which download is unavailable it should fail with error.

@T-256
Copy link
Contributor

T-256 commented Jul 30, 2024

It may make sense to just expose a way to use the lowest compatible version instead (as suggested by @T-256) so it's easy to do in CI?

@zanieb what if minimum version is not provided and only used lower-bound? for example requires-python = "<3.12" should select which python version when used with that shipped option (assume --python-resolution=lowest-minor)?

@Vigilans
Copy link
Contributor Author

Vigilans commented Jul 30, 2024

I prefer to use the newest version.

Pros:

  • For users who want to stick to specific python version, they will use == 3.8.* or >=3.8,<3.9.
  • For users who specify only lower bound, using newest python version that breaks their project, is a hint to users that they should set upper bound to their pyproject.toml. This helps make their specification more robust.

Cons:

  • For non-author users cloning the project, they may fail to run the project because the selected version is too new. Even users add the newer version to upper bound, it may be hard to sync the change to upstream.

A flag altering this behavior to use lowest possible version should be introduced indeed to ensure stable build in CI like docker image building, or for build script/devcontainer to ensure end users can run on happy path after their cloning.

@zanieb
Copy link
Member

zanieb commented Jul 30, 2024

Note we basically never recommend adding an upper bound to Python version constraints and actively ignore it in some contexts.

in next year when a contributor wants init an environment, it would be 3.13 which brings different development environments.

Yeah this problem is solved by encouraging pin of the version (see also #4970)

Copy link
Contributor Author

@Vigilans Vigilans left a comment

Choose a reason for hiding this comment

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

Test added. Found some more behaviors that should be reviewed.

Comment on lines 265 to 335
// With `requires-python = ">=3.10"`, we should prefer first possible version 3.11
let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(indoc! { r###"
[project]
name = "foo"
version = "1.0.0"
requires-python = ">=3.10"
dependencies = []
"###
})?;

uv_snapshot!(context.filters(), context.venv()
.arg("--preview"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Using Python 3.11.[X] interpreter at: [PYTHON-3.11]
Creating virtualenv at: .venv
Activate with: source .venv/bin/activate
"###
);

// With `requires-python = ">=3.11"`, we should prefer first possible version 3.11
let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(indoc! { r###"
[project]
name = "foo"
version = "1.0.0"
requires-python = ">=3.10"
dependencies = []
"###
})?;

uv_snapshot!(context.filters(), context.venv()
.arg("--preview"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Using Python 3.11.[X] interpreter at: [PYTHON-3.11]
Creating virtualenv at: .venv
Activate with: source .venv/bin/activate
"###
);

// With `requires-python = ">=3.12"`, we should prefer first possible version 3.12
let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(indoc! { r###"
[project]
name = "foo"
version = "1.0.0"
requires-python = ">=3.10"
dependencies = []
"###
})?;

uv_snapshot!(context.filters(), context.venv()
.arg("--preview"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Using Python 3.11.[X] interpreter at: [PYTHON-3.11]
Creating virtualenv at: .venv
Activate with: source .venv/bin/activate
"###
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that the default behavior for >=3.x is not select newest version, but select first possible version in the list. For a python list [3.11, 3.10, 3.12]:

  • >=3.10 will select 3.11.
  • >=3.11 will select 3.11.
  • >3.11 will select 3.11 (becuase 3.11.x > 3.11).
  • >=3.12 will select 3.12.

@T-256
Copy link
Contributor

T-256 commented Jul 30, 2024

I prefer to use the newest version.

@Vigilans I Agree, And I think it could be opt-in to that behavior.

created #5609 to track it separately.

[project]
name = "foo"
version = "1.0.0"
requires-python = ">=3.10"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be 3.11?

[project]
name = "foo"
version = "1.0.0"
requires-python = ">=3.10"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be 3.12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, I got these tests code totally wrong! Since the commit is pushed near my bed time I did not check them carefully after pasting. Good news that tests still pass after correction. Apology for my carelessness.

@Vigilans Vigilans force-pushed the vigilans/uv-venv-detect-project-python-version branch from e4baa70 to f12a994 Compare July 31, 2024 04:35
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.

Thanks!

@zanieb zanieb enabled auto-merge (squash) July 31, 2024 13:38
@zanieb zanieb merged commit 0dcec9e into astral-sh:main Jul 31, 2024
57 checks passed
charliermarsh pushed a commit that referenced this pull request Aug 18, 2024
## Summary

Fixes #6177

This ensures a `pyproject.toml` file without a `[project]` table is not
a fatal error for `uv venv`, which is just trying to discover/respect
the project's `python-requires` (#5592).

Similarly, any caught `WorkspaceError` is now also non-fatal and instead
prints a warning message (feeback welcome here, felt less surprising
than e.g. a malformed `pyproject.toml` breaking `uv venv`).

## Test Plan

I added two test cases: `cargo test -p uv --test venv`

Also, existing venv tests were failing for me since I use fish and the
printed activation script was `source .venv/bin/activate.fish` (to
repro, just run the tests with `SHELL=fish`). So added an insta filter
to normalize that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Settings and such preview Experimental behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uv venv should support respecting pyproject.toml Expose more fine-grained control for uv sync command
4 participants