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

Use intersection rather than union for requires-python #5644

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

charliermarsh
Copy link
Member

Summary

As-is, if you have a workspace with mixed requires-python requirements, resolution will never succeed, since we'll use the union as the requires-python bound (i.e., take the lowest value), and fail when we see the package that only supports some more narrow range.

This PR modifies the behavior to take the intersection (i.e., the highest value), so if you have one package that supports Python 3.12 and later, and another that supports Python 3.8 and later, we lock for Python 3.12. If you try to sync or run with Python 3.8, we raise an error, since the lockfile will be incompatible with that request.

Konsti has a write-up in #5594 that outlines what could be a longer-term strategy.

Closes #5578.

@charliermarsh charliermarsh added bug Something isn't working preview Experimental behavior labels Jul 31, 2024
@charliermarsh charliermarsh marked this pull request as ready for review July 31, 2024 00:10
@charliermarsh charliermarsh force-pushed the charlie/min-requires-python branch 3 times, most recently from 91465a9 to 235e560 Compare July 31, 2024 00:12
@@ -101,7 +101,7 @@ pub(crate) enum ProjectError {
pub(crate) fn find_requires_python(
workspace: &Workspace,
) -> Result<Option<RequiresPython>, uv_resolver::RequiresPythonError> {
RequiresPython::union(workspace.packages().values().filter_map(|member| {
RequiresPython::intersection(workspace.packages().values().filter_map(|member| {
Copy link
Member Author

Choose a reason for hiding this comment

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

We can warn here, if people feel strongly. I'm not sure that we should, though, since users will see it every time without any way to resolve it.

Copy link

codspeed-hq bot commented Jul 31, 2024

CodSpeed Performance Report

Merging #5644 will not alter performance

Comparing charlie/min-requires-python (1566081) with main (8d14a4c)

Summary

✅ 13 untouched benchmarks

----- stderr -----
warning: `uv sync` is experimental and may change without warning
Using Python 3.8.[X] interpreter at: [PYTHON-3.8]
error: The requested Python interpreter (3.8.[X]) is incompatible with the project Python requirement: `>=3.12`
Copy link
Member

Choose a reason for hiding this comment

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

We should add a specific error message for this case that tells you how to run something on 3.8 with only bird-feeder.

Copy link
Member Author

Choose a reason for hiding this comment

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

@konstin -- What's the recommended workflow? Can we even support it right now?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say we check if the request version is in the union, but not in the intersection, and if so, recommend uv pip install -p 3.8 -e . <older-python-version-project>, which should work, i can test this after #5644 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, works.

Copy link
Member

Choose a reason for hiding this comment

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

Now i checked the right way and cargo run venv -p 3.8 && cargo run pip install -e packages/foo_lib/ indeed works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do separately.

@konstin
Copy link
Member

konstin commented Jul 31, 2024

Those codspeed numbers are dubious

@konstin
Copy link
Member

konstin commented Jul 31, 2024

I just tried the following project layout:

.
├── packages
│   ├── bar
│   │   ├── pyproject.toml
│   │   └── src
│   │       └── bar
│   │           └── __init__.py
│   └── foo_lib
│       ├── pyproject.toml
│       └── src
│           └── foo_lib
│               └── __init__.py
├── pyproject.toml
├── README.md
└── uv.lock

bar:

[project]
name = "bar"
version = "0.1.0"
requires-python = ">=3.12"
dependencies = []

[build-system]
requires = ["hatchling"]
build-backend = "hatchling.build"

foo_lib:

[project]
name = "foo-lib"
version = "0.1.0"
requires-python = ">=3.8"
dependencies = []

[build-system]
requires = ["hatchling"]
build-backend = "hatchling.build"

When trying to uv lock, i get:

Using Python 3.12.1
  × No solution found when resolving dependencies:
  ╰─▶ Because the requested Python version (>=3.8) does not satisfy Python>=3.12 and bar==0.1.0
      depends on Python>=3.12, we can conclude that bar==0.1.0 cannot be used.
      And because only bar==0.1.0 is available and you require bar, we can conclude that the
      requirements are unsatisfiable.

      hint: The `requires-python` value (>=3.8) includes Python versions that are not supported by
      your dependencies (e.g., bar==0.1.0 only supports >=3.12). Consider using a more restrictive
      `requires-python` value (like >=3.12).

It seems that the PR currently forces the same requires-python across the workspace.

@charliermarsh
Copy link
Member Author

You must be running from main? It works fine for me on this PR, I just replicated it.

@charliermarsh
Copy link
Member Author

charliermarsh commented Jul 31, 2024

version = 1
requires-python = ">=3.12"

[[distribution]]
name = "bar"
version = "0.1.0"
source = { editable = "packages/bar" }

[[distribution]]
name = "foo-lib"
version = "0.1.0"
source = { editable = "packages/foo_lib" }

[[distribution]]
name = "root"
version = "0.1.0"
source = { editable = "." }

@konstin
Copy link
Member

konstin commented Jul 31, 2024

Right branch but it was using uv lock, not cargo run lock 😶 With this branch it actually works

@charliermarsh
Copy link
Member Author

Haha yeah. That's the exact problem this was designed to fix 😂

@charliermarsh charliermarsh merged commit bf8934e into main Jul 31, 2024
57 checks passed
@charliermarsh charliermarsh deleted the charlie/min-requires-python branch July 31, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working preview Experimental behavior
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Requires-python conflicts inside a workspace
3 participants