Skip to content

Conflicting groups should handle conflicting inclusions automatically #12005

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

Merged
merged 5 commits into from
Mar 8, 2025

Conversation

jtfmumm
Copy link
Contributor

@jtfmumm jtfmumm commented Mar 6, 2025

This adds support for inferring dependency group conflict sets from the directly defined conflicts in configuration. For example, if you declare a conflict between groups alpha and beta and dev includes beta, then we will infer a conflict between dev and alpha. We will also handle a conflict between two groups if they transitively include groups that conflict with each other. See #11232 for more details.

Closes #11232

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've moved this file so that I can import DependencyGroups in conflicts.rs. It would have caused a circular dependency in uv-workspace.

}

// Propagate canonical items through the graph and populate substitutions.
// FIXME: Have we already done cycle detection before this method was
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we assume cycle detection before this point? Otherwise, we'll have to return an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a failing test that wasn't running locally for some reason that shows we can't assume it at this point. I'll update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I've done now is to just return if there's a Cycle error on toposort. I've left a FIXME comment indicating that we're currently bailing and allowing more detailed include cycle detection downstream. Is this the behavior we want? If so, I'll remove the FIXME tag.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm cc @zanieb

One thing that strikes me here is that the cycle detection error would only kick in when a relevant conflicting group is declared? What happens if there aren't any conflicts declared? Is there cycle detection happening somewhere else?

Copy link
Contributor Author

@jtfmumm jtfmumm Mar 6, 2025

Choose a reason for hiding this comment

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

Cycle detection is happening somewhere, maybe in the resolver? But that happens after this code is called. With this new version where I just return (no error) when toposort() returns an error, then only the original conflict sets are sent along.

But it does seem weird to silently swallow the error. The reason I don't want it propagated is because better cycle detection happens elsewhere ("there was a cycle" vs. "there was a cycle: dev -> test -> dev"). So I could either warn the user in this method or match at the caller and warn there. Do either of those options make sense?

Copy link
Member

Choose a reason for hiding this comment

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

I think what you're doing now is probably reasonable with a comment explaining what's going on here (basically, what you just said). @zanieb might have a different opinion though!

Copy link
Member

Choose a reason for hiding this comment

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

I don't know what our current behavior for cycles is. Are we talking about cycles in inclusions or cycles in the conflicts?

  1. If dev includes test and test includes dev don't we have a consistent set of dependencies? (just the union of test and dev?)

  2. Similarly, if test is marked as conflicting with some other group foo then dev conflicts with foo too, the cycle doesn't matter here.

  3. If test is marked as conflicting with dev, now the cycle is a problem because... they include each other and can't conflict. This applies to longer chains too, e.g., if we have test -> foo -> dev -> test a conflict between test and dev is "invalid".

If we don't have test coverage for each of these cases, we should add it. In reply to

One thing that strikes me here is that the cycle detection error would only kick in when a relevant conflicting group is declared?

This seems fine, because this form of cycle (3) is the only one that creates an unresolvable situation?

Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm misunderstanding, cycles in dependency groups are an error. Per the PEP:

Dependency Group Includes MUST NOT include cycles, and tools SHOULD report an error if they detect a cycle.

I think we have an DependencyGroupCycle error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, on main we definitely error on cycles. That's the behavior I'm relying on when just returning from the conflict expansion method when detecting a cycle during toposort.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, thanks for the correction. Maybe I was just thinking about feedback I wrote on the PEP.

@jtfmumm jtfmumm force-pushed the jtfm/conflicting-groups branch from d196583 to 6db0af9 Compare March 6, 2025 14:12
@jtfmumm jtfmumm force-pushed the jtfm/conflicting-groups branch from 6db0af9 to 65f059f Compare March 6, 2025 14:13
@jtfmumm jtfmumm force-pushed the jtfm/conflicting-groups branch from c2aeec1 to 7f88996 Compare March 6, 2025 14:27
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Nice work!

The failing duplicate_torch_and_sympy_because_of_wrong_inferences is somewhat worrisome, because that doesn't use dependency groups at all. So in theory, it shouldn't be impacted by the work here?

In your code, I wonder if something wonky is happening when no DependencyGroupSpecifier::IncludeGroup are found and thus no edges are added?

}

// Propagate canonical items through the graph and populate substitutions.
// FIXME: Have we already done cycle detection before this method was
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm cc @zanieb

One thing that strikes me here is that the cycle detection error would only kick in when a relevant conflicting group is declared? What happens if there aren't any conflicts declared? Is there cycle detection happening somewhere else?

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.

Looks good from my perspective modulo the Cargo.lock changes which we should verify before merging.


----- stderr -----
Resolved 5 packages in [TIME]
error: Groups `dev` and `dev2` are incompatible with the transitively inferred conflicts: {`example:dev`, `example:dev2`}
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand this error, are we just repeating the groups here at the end? What's the value of that?

Copy link
Member

@charliermarsh charliermarsh Mar 8, 2025

Choose a reason for hiding this comment

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

I believe this is identical to the existing error we show in these cases, except "transitively" has been inserted. Perhaps this should be hashed out separately from this PR, since it doesn't seem to be a change? But for reference, the set at the end can be larger than "just" these two items. The set at the end is of arbitrary size, and the groups we list at the start are the activated, conflicting members.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, if it's not changing here that's fine and we can revisit it separately — I just looked at the test case not the implementation.

@jtfmumm jtfmumm merged commit b7968e7 into main Mar 8, 2025
98 checks passed
@jtfmumm jtfmumm deleted the jtfm/conflicting-groups branch March 8, 2025 18:21
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Mar 24, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.6.5` -> `0.6.9` |

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.6.9`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#069)

[Compare Source](astral-sh/uv@0.6.8...0.6.9)

##### Enhancements

-   Use `keyring --mode creds` when `authenticate = "always"` ([#&#8203;12316](astral-sh/uv#12316))
-   Fail with specific error message when no password is present and `authenticate = "always"` ([#&#8203;12313](astral-sh/uv#12313))

##### Bug fixes

-   Add boolish value parser for `UV_MANAGED_PYTHON` flags ([#&#8203;12345](astral-sh/uv#12345))
-   Make deserialization non-fatal when assessing source tree revisions ([#&#8203;12319](astral-sh/uv#12319))
-   Use resolver-returned wheel over alternate cached wheel ([#&#8203;12301](astral-sh/uv#12301))

##### Documentation

-   Add experimental `--torch-backend` to the PyTorch guide ([#&#8203;12317](astral-sh/uv#12317))
-   Fix `#keyring-provider` references in alternative index docs ([#&#8203;12315](astral-sh/uv#12315))
-   Fix `--directory` path in examples ([#&#8203;12165](astral-sh/uv#12165))

##### Preview changes

-   Automatically infer the PyTorch index via `--torch-backend=auto` ([#&#8203;12070](astral-sh/uv#12070))

### [`v0.6.8`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#068)

[Compare Source](astral-sh/uv@0.6.7...0.6.8)

##### Enhancements

-   Add support for enabling all groups by default with `default-groups = "all"` ([#&#8203;12289](astral-sh/uv#12289))
-   Add simpler `--managed-python` and `--no-managed-python` flags for toggling Python preferences ([#&#8203;12246](astral-sh/uv#12246))

##### Performance

-   Avoid allocations for default cache keys ([#&#8203;12063](astral-sh/uv#12063))

##### Bug fixes

-   Allow local version mismatches when validating lockfile ([#&#8203;12285](astral-sh/uv#12285))
-   Allow owned string when deserializing `requires-python` ([#&#8203;12278](astral-sh/uv#12278))
-   Make cache errors non-fatal in `Planner::build` ([#&#8203;12281](astral-sh/uv#12281))

### [`v0.6.7`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#067)

[Compare Source](astral-sh/uv@0.6.6...0.6.7)

##### Python

-   Add CPython 3.14.0a6
-   Fix regression where extension modules would use wrong `CXX` compiler on Linux
-   Enable FTS3 enhanced query syntax for SQLite

See the [`python-build-standalone` release notes](https://github.com/astral-sh/python-build-standalone/releases/tag/20250317) for more details.

##### Enhancements

-   Add support for `-c` constraints in `uv add` ([#&#8203;12209](astral-sh/uv#12209))
-   Add support for `--global` default version in `uv python pin` ([#&#8203;12115](astral-sh/uv#12115))
-   Always reinstall local source trees passed to `uv pip install` ([#&#8203;12176](astral-sh/uv#12176))
-   Render token claims on publish permission error ([#&#8203;12135](astral-sh/uv#12135))
-   Add pip-compatible `--group` flag to `uv pip install` and `uv pip compile` ([#&#8203;11686](astral-sh/uv#11686))

##### Preview features

-   Avoid creating duplicate directory entries in built wheels ([#&#8203;12206](astral-sh/uv#12206))
-   Allow overriding module names for editable builds ([#&#8203;12137](astral-sh/uv#12137))

##### Performance

-   Avoid replicating core-metadata field on `File` struct ([#&#8203;12159](astral-sh/uv#12159))

##### Bug fixes

-   Add `src` to default cache keys ([#&#8203;12062](astral-sh/uv#12062))
-   Discard insufficient fork markers ([#&#8203;10682](astral-sh/uv#10682))
-   Ensure `python pin --global` creates parent directories if missing ([#&#8203;12180](astral-sh/uv#12180))
-   Fix GraalPy abi tag parsing and discovery ([#&#8203;12154](astral-sh/uv#12154))
-   Remove extraneous script packages in `uv sync --script` ([#&#8203;12158](astral-sh/uv#12158))
-   Remove redundant `activate.bat` output ([#&#8203;12160](astral-sh/uv#12160))
-   Avoid subsequent index hint when no versions are available on the first index ([#&#8203;9332](astral-sh/uv#9332))
-   Error on lockfiles with incoherent wheel versions ([#&#8203;12235](astral-sh/uv#12235))

##### Rust API

-   Update `BaseClientBuild` to accept custom proxies ([#&#8203;12232](astral-sh/uv#12232))

##### Documentation

-   Make testpypi index explicit in example snippet ([#&#8203;12148](astral-sh/uv#12148))
-   Reverse and format the archived changelogs ([#&#8203;12099](astral-sh/uv#12099))
-   Use consistent commas around i.e. and e.g. ([#&#8203;12157](astral-sh/uv#12157))
-   Fix typos in MRE docs ([#&#8203;12198](astral-sh/uv#12198))
-   Fix double space typo ([#&#8203;12171](astral-sh/uv#12171))

### [`v0.6.6`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#066)

[Compare Source](astral-sh/uv@0.6.5...0.6.6)

##### Python

-   Add support for dynamic musl Python distributions on x86-64 Linux ([#&#8203;12121](astral-sh/uv#12121))
-   Allow the experimental JIT to be enabled at runtime on Python 3.13 and 3.14 on Linux
-   Upgrade the build toolchain to LLVM 20, improving performance

See the [`python-build-standalone` release notes](https://github.com/astral-sh/python-build-standalone/releases/tag/20250311) for more details.

##### Enhancements

-   Add `--marker` flag to `uv add` ([#&#8203;12012](astral-sh/uv#12012))
-   Allow overriding module name for uv build backend ([#&#8203;11884](astral-sh/uv#11884))
-   Sync latest Python releases ([#&#8203;12120](astral-sh/uv#12120))
-   Use 'Upload' instead of 'Download' in publish reporter ([#&#8203;12029](astral-sh/uv#12029))
-   Add `[index].authenticate` allowing authentication to be required on an index ([#&#8203;11896](astral-sh/uv#11896))
-   Add support for Windows legacy scripts in `uv tool run` ([#&#8203;12079](astral-sh/uv#12079))
-   Propagate conflicting dependency groups when using `include-group` ([#&#8203;12005](astral-sh/uv#12005))
-   Show ambiguous requirements when `uv add` failed ([#&#8203;12106](astral-sh/uv#12106))

##### Performance

-   Cache workspace discovery ([#&#8203;12096](astral-sh/uv#12096))
-   Insert dependencies into fork state prior to fetching metadata ([#&#8203;12057](astral-sh/uv#12057))
-   Remove some allocations from `uv-auth` ([#&#8203;12077](astral-sh/uv#12077))

##### Bug fixes

-   Avoid considering `PATH` updated when the `export` is commented in the shellrc ([#&#8203;12043](astral-sh/uv#12043))
-   Fix `uv publish` retry on network failures ([#&#8203;12041](astral-sh/uv#12041))
-   Use a sized stream in `uv publish` to comply with WSGI PyPI server constraints ([#&#8203;12111](astral-sh/uv#12111))
-   Fix `uv python install --reinstall` when the version was not previously installed ([#&#8203;12124](astral-sh/uv#12124))

##### Preview features

-   Fix `uv_build` invocation ([#&#8203;12058](astral-sh/uv#12058))

##### Documentation

-   Quote versions string in `python-versions.md` ([#&#8203;12112](astral-sh/uv#12112))
-   Fix tool concept page headings ([#&#8203;12053](astral-sh/uv#12053))
-   Update the `[index].authenticate` docs ([#&#8203;12102](astral-sh/uv#12102))
-   Update versioning policy ([#&#8203;11666](astral-sh/uv#11666))

</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:eyJjcmVhdGVkSW5WZXIiOiIzOS4xOTQuMCIsInVwZGF0ZWRJblZlciI6IjM5LjIwOS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflicting groups should handle conflicting inclusions automatically
4 participants