fix(solver): prune sibling deps disjoint with override marker context#10944
fix(solver): prune sibling deps disjoint with override marker context#10944dimbleby wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
complete_package, the new marker-pruning logic relies onself._overrides_marker_intersectionalways being a meaningful context; consider either explicitly guarding this with an "in override" condition or asserting/documenting what the default intersection represents when not in an override branch so future changes don't break this assumption. - The comment above the new
if self._overrides_marker_intersection.intersect(dep.marker).is_empty():check describes behavior "when solving under an override" but the code runs unconditionally; aligning the implementation with the comment (or clarifying the comment) would make the control flow and intent easier to follow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `complete_package`, the new marker-pruning logic relies on `self._overrides_marker_intersection` always being a meaningful context; consider either explicitly guarding this with an "in override" condition or asserting/documenting what the default intersection represents when not in an override branch so future changes don't break this assumption.
- The comment above the new `if self._overrides_marker_intersection.intersect(dep.marker).is_empty():` check describes behavior "when solving under an override" but the code runs unconditionally; aligning the implementation with the comment (or clarifying the comment) would make the control flow and intent easier to follow.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Looks like a clear improvement. I just wonder if we should let it close #5506 because it only fixes the issue if one of both dependencies has multiple constraints. It does not fix the issue if both dependencies have just a single constraint. For example, it fixes but not the derived example |
|
You are right. My AI friend has a fix that addresses both but it is a little expensive in locking time. (Nowhere near #6969 levels, but enough to notice on large solves.) I will poke at that for a bit. If you merge this first, or if I do not find a way to recover performance, I will create a separate pull request for consideration. If everything can be made excellent, I will add a second commit here. |
|
pushed a supplementary - but more complicated - fix which:
|
When the solver retries under an override branch for a duplicate dependency, sibling dependencies whose markers are disjoint with the override's effective marker context were still considered. Their transitive constraints then spuriously conflicted with the overridden constraint, causing resolution to fail even when a valid solution existed (e.g. a transitive dep pinning a different version of the overridden package on a marker branch that does not apply). Extend the existing overrides_marker_intersection mechanism into complete_package so such siblings are pruned during the solve phase, mirroring the filter already applied to duplicate deps. Fixes python-poetry#5506. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…thon-poetry#5506) A package required at conflicting versions by two different packages was reported as an unsolvable conflict, even when the two requirements apply under markers that can never be true at the same time (for example one only on Windows and the other only on Linux). The provider only reconciles conflicting requirements that come from the same package, so this case was missed. Now, when a resolution fails, the solver checks whether the failure is caused by such a pair and, if so, re-runs resolution twice, each run restricted to one side of the markers, so that only one of the requirements applies per run. This check runs only after a resolution has already failed, so successful resolutions are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Another one that looks plausible, has a convincing testcase, is probably right...
When the solver retries under an override branch for a duplicate dependency, sibling dependencies whose markers are disjoint with the override's effective marker context were still considered. Their transitive constraints then spuriously conflicted with the overridden constraint, causing resolution to fail even when a valid solution existed (e.g. a transitive dep pinning a different version of the overridden package on a marker branch that does not apply).
Extend the existing overrides_marker_intersection mechanism into complete_package so such siblings are pruned during the solve phase, mirroring the filter already applied to duplicate deps.
Fixes #5506.