-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Support reading dependency-groups from pyproject.tomls with no project #13742
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
Conversation
| Ok(Self::NonProject(workspace)) | ||
| } else if default_missing_workspace { | ||
| // Otherwise it's a pyproject.toml that maybe contains dependency-groups | ||
| // that we want to treat like a project/workspace to handle those uniformly | ||
| let project_path = std::path::absolute(project_root) | ||
| .map_err(WorkspaceError::Normalize)? | ||
| .clone(); | ||
|
|
||
| let workspace = Workspace::collect_members( | ||
| project_path, | ||
| ToolUvWorkspace::default(), | ||
| pyproject_toml, | ||
| None, | ||
| options, | ||
| cache, | ||
| ) | ||
| .await?; | ||
|
|
||
| Ok(Self::NonProject(workspace)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is why the existing virtual workspace support is insufficient (at least one test fails if I try to do this universally -- although it's a test that specifically checks that we error out if [project] and [tool.uv.workspace] are missing).
| for (pyproject_path, groups) in groups { | ||
| let empty = PathBuf::new(); | ||
| let pyproject_parent = pyproject_path.parent().unwrap_or(&empty); | ||
| // TODO(Gankra): this surely isn't right but VirtualProject::discover asserts on relative paths... hrm. | ||
| let project_dir = current_dir().unwrap().join(pyproject_parent); | ||
| let metadata = SourcedDependencyGroups::from_virtual_project( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the assert is telling me I'm still using the wrong API though:
This method requires an absolute path and panics otherwise, i.e. this method only supports discovering the main workspace.
I think this is saying it won't properly detect a package under a workspace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I now properly understand this code and the assert and my solution are both essentially correct, but the scary "this can only discover the root" warning is wrong or misleading. Cleaned it up.
|
As always: this requires Way More Tests... |
CodSpeed Walltime Performance ReportMerging #13742 will not alter performanceComparing Summary
|
|
@konstin I think it's good for Charlie to review, but it'd be good for you to own other support that's needed here. |
Will be fixed in: astral-sh/uv#13742
0d0d59a to
690e750
Compare
|
(nb this is temporarily based on top of #13735 because the two merge conflict) |
690e750 to
3605cf3
Compare
|
Could you add like a table for the different cases of the current interactions of workspace and sources when we read dependency groups? E.g., a no-project file that is {member,root} of a workspace where the {root, a member} defines sources. I know these didn't change with this PR, but it would be helpful to have them documented. |
|
If they're not changing in this pull request we probably shouldn't block this pull request on it? Can we open an issue for that and follow-up there? |
|
We don't change the behavior for workspace discovery and sources here, but we apply them to a new class of inputs, and it helps for reviewing to have a clear understanding which semantics we're applying, especially with non-project |
So I think the crux of "nothing interesting has changed here" is that a no-project file cannot be a member of a workspace. It can only be the root of a workspace. The mode I added just allows you to omit the [tool.uv.workspace] file, in which case it's the only member of its workspace. |
|
Furthermore, this ultra-special case I've added is only permitted by |
I see, that explains it! |
| metadata | ||
| .name | ||
| .clone() | ||
| .unwrap_or(PackageName::from_str("none").unwrap()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this an option instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh ok this is used way less than I thought, literally only in this one place. Cool fixed.
(or legacy tool.uv.workspace). This cleaves out a dedicated SourcedDependencyGroups type based on RequiresDist but with only the DependencyGroup handling implemented. This allows `uv pip` to read `dependency-groups` from pyproject.tomls that only have that table defined, per PEP 735, and as implemented by `pip`. However we want our implementation to respect various uv features when they're available: * `tool.uv.sources` * `tool.uv.index` * `tool.uv.dependency-groups.mygroup.requires-python` (#13735) As such we want to opportunistically detect "as much as possible" while doing as little as possible when things are missing. The issue with the old RequiresDist path was that it fundamentally wanted to build the package, and if `[project]` was missing it would try to desperately run setuptools on the pyproject.toml to try to find metadata and make a hash of things. At the same time, the old code also put in a lot of effort to try to pretend that `uv pip` dependency-groups worked like `uv` dependency-groups with defaults and non-only semantics, only to separate them back out again. By explicitly separating them out, we confidently get the expected behaviour. Note that dependency-group support is still included in RequiresDist, as some `uv` paths still use it. It's unclear to me if those paths want this same treatment -- for now I conclude no. Fixes #13138
488e0c7 to
fb63a50
Compare
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.7.13` -> `0.7.14` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.7.14`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0714) [Compare Source](astral-sh/uv@0.7.13...0.7.14) ##### Enhancements - Add XPU to `--torch-backend` ([#​14172](astral-sh/uv#14172)) - Add ROCm backends to `--torch-backend` ([#​14120](astral-sh/uv#14120)) - Remove preview label from `--torch-backend` ([#​14119](astral-sh/uv#14119)) - Add `[tool.uv.dependency-groups].mygroup.requires-python` ([#​13735](astral-sh/uv#13735)) - Add auto-detection for AMD GPUs ([#​14176](astral-sh/uv#14176)) - Show retries for HTTP status code errors ([#​13897](astral-sh/uv#13897)) - Support transparent Python patch version upgrades ([#​13954](astral-sh/uv#13954)) - Warn on empty index directory ([#​13940](astral-sh/uv#13940)) - Publish to DockerHub ([#​14088](astral-sh/uv#14088)) ##### Performance - Make cold resolves about 10% faster ([#​14035](astral-sh/uv#14035)) ##### Bug fixes - Don't use walrus operator in interpreter query script ([#​14108](astral-sh/uv#14108)) - Fix handling of changes to `requires-python` ([#​14076](astral-sh/uv#14076)) - Fix implied `platform_machine` marker for `win_amd64` platform tag ([#​14041](astral-sh/uv#14041)) - Only update existing symlink directories on preview uninstall ([#​14179](astral-sh/uv#14179)) - Serialize Python requests for tools as canonicalized strings ([#​14109](astral-sh/uv#14109)) - Support netrc and same-origin credential propagation on index redirects ([#​14126](astral-sh/uv#14126)) - Support reading `dependency-groups` from pyproject.tomls with no `[project]` ([#​13742](astral-sh/uv#13742)) - Handle an existing shebang in `uv init --script` ([#​14141](astral-sh/uv#14141)) - Prevent concurrent updates of the environment in `uv run` ([#​14153](astral-sh/uv#14153)) - Filter managed Python distributions by platform before querying when included in request ([#​13936](astral-sh/uv#13936)) ##### Documentation - Replace cuda124 with cuda128 ([#​14168](astral-sh/uv#14168)) - Document the way member sources shadow workspace sources ([#​14136](astral-sh/uv#14136)) - Sync documented PyTorch integration index for CUDA and ROCm versions from PyTorch website ([#​14100](astral-sh/uv#14100)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MC42Mi4xIiwidXBkYXRlZEluVmVyIjoiNDAuNjIuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
(or legacy tool.uv.workspace).
This cleaves out a dedicated SourcedDependencyGroups type based on RequiresDist but with only the DependencyGroup handling implemented. This allows
uv pipto readdependency-groupsfrom pyproject.tomls that only have that table defined, per PEP 735, and as implemented bypip.However we want our implementation to respect various uv features when they're available:
tool.uv.sourcestool.uv.indextool.uv.dependency-groups.mygroup.requires-python(Add[tool.uv.dependency-groups].mygroup.requires-python#13735)As such we want to opportunistically detect "as much as possible" while doing as little as possible when things are missing. The issue with the old RequiresDist path was that it fundamentally wanted to build the package, and if
[project]was missing it would try to desperately run setuptools on the pyproject.toml to try to find metadata and make a hash of things.At the same time, the old code also put in a lot of effort to try to pretend that
uv pipdependency-groups worked likeuvdependency-groups with defaults and non-only semantics, only to separate them back out again. By explicitly separating them out, we confidently get the expected behaviour.Note that dependency-group support is still included in RequiresDist, as some
uvpaths still use it. It's unclear to me if those paths want this same treatment -- for now I conclude no.Fixes #13138