small refactoring in pursuit of fixing conflicting extras/groups #9386
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, we had copied the behavior of `try_markers` to return `None` in the case where the marker was always true. I believe this was done because it somewhat implies that there is no forking happening. But I find this somewhat strange personally, and instead flipped this around so that it still returns a marker in that case. The one call site that is impacted by this is the resolution graph construction. If we left it as-is, it would end up with a list of one marker that is always true in some cases. And this in turn results in writing an empty `resolution-markers` to the lock file. Probably the output logic should be tweaked instead, but we leave it alone for now.
It was missing a closing parenthesis.
This was almost not used any more with the refactor toward 'UniversalMarker', so this just removes the final uses.
I think Ibraheem had this routine at some point in the past, but we ended up dropping it because we didn't have a use for it. Well, now we do! It turns out that when we generate "conflict markers," they don't actually take "world knowledge" into account. In particular, there is "world knowledge" that a particular set of extras cannot be enabled simultaneously. This in turn allows us to simplify most conflict markers. If we didn't do this, it's likely that lock files would become littered with conflict markers whenever any conflicts are declared. This is somewhat (although not completely) analogous to how we "simplify" markers with respect to `requires-python`. That is, `requires-python` reflects world knowledge that enables markers to be written more simply than they otherwise would be without world knowledge.
When we generate conflict markers for each resolution after the
resolver runs, it turns out that generating them just from exclusion
rules is not sufficient.
For example, if `foo` and `bar` are declared as conflicting extras, then
we end up with the following forks:
A: extra != 'foo'
B: extra != 'bar'
C: extra != 'foo' and extra != 'bar'
Now let's take an example where these forks don't share the same version
for all packages. Consider a case where `idna==3.9` is in forks A and C,
but where `idna==3.10` is in fork B. If we combine the markers in forks
A and C through disjunction, we get the following:
idna==3.9: extra != 'foo' or (extra != 'foo' and extra != 'bar')
idna==3.10: extra != 'bar'
Which simplifies to:
idna==3.9: extra != 'foo'
idna==3.10: extra != 'bar'
But these are clearly not disjoint. Both dependencies could be selected,
for example, when neither `foo` nor `bar` are active. We can remedy this
by keeping around the inclusion rules for each fork:
A: extra != 'foo' and extra == 'bar'
B: extra != 'bar' and extra == 'foo'
C: extra != 'foo' and extra != 'bar'
And so for `idna`, we have:
idna==3.9: (extra != 'foo' and extra == 'bar') or (extra != 'foo' and extra != 'bar')
idna==3.10: extra != 'bar' and extra == 'foo'
Which simplifies to:
idna==3.9: extra != 'foo'
idna==3.10: extra != 'bar' and extra == 'foo'
And these *are* properly disjoint. There is no way for them both to be
active. This also correctly accounts for fork C where neither `foo` nor
`bar` are active, and yet, `idna==3.9` is still enabled but `idna==3.10`
is not. (In the [motivating example], this comes from `baz` being enabled.)
That is, this captures the idea that for `idna==3.10` to be installed,
there must actually be a specific extra that is enabled. That's what
makes it disjoint from `idna==3.9`.
We aren't quite done yet, because this does add *too many* conflict
markers to dependency edges that don't need it. In the next commit,
we'll add in our world knowledge to simplify these conflict markers.
[motivating example]: #9289
BurntSushi
changed the title
This change is correct because disjointness checks now incorporate conflicts. In this case, there are actually four forks. Two of them correspond to `sys_platform == 'darwin'` and `sys_platform != 'darwin'`, but neither of those contain `jinja2==3.1.3`. Instead, they contain other versions of `jinja2` linked to other extras. If we ever add conflicts to our `resolution-markers` in the lock file, then those forks should show up here again. (Because, of course, some forks do contain `jinja2==3.1.3` here.)
charliermarsh
approved these changes
Nov 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR splits off some commits from #9370. These are smaller
refactoring commits that shouldn't change any behavior. This is just in
the interest of making #9370 a bit smaller, which I expect might get a
bit bigger.
Ref #9289