perf: add support for AtomicMultiMarker and AtomicMarkerUnion for extra markers#818
Merged
radoering merged 2 commits intopython-poetry:mainfrom Jan 18, 2025
Merged
Conversation
Reviewer's Guide by SourceryThis pull request introduces Class diagram for new Extra Constraint classesclassDiagram
class Constraint {
+value: str
+operator: str
+intersect(other)
+union(other)
+invert()
}
class ExtraConstraint {
+intersect(other)
+union(other)
}
class MultiConstraint {
+OPERATORS: tuple
+constraints: list
+intersect(other)
+union(other)
}
class ExtraMultiConstraint {
+OPERATORS: tuple
+intersect(other)
+union(other)
}
Constraint <|-- ExtraConstraint
MultiConstraint <|-- ExtraMultiConstraint
note for ExtraConstraint "New class for handling extra markers"
note for ExtraMultiConstraint "Specialized multi-constraint for extras"
Class diagram for Atomic Marker classesclassDiagram
class SingleMarkerLike {
+name: str
+constraint: Constraint
+validate(environment)
+invert()
}
class AtomicMultiMarker {
+complexity: tuple
+validate(environment)
+invert()
+expand()
}
class AtomicMarkerUnion {
+complexity: tuple
+validate(environment)
+invert()
+expand()
}
SingleMarkerLike <|-- AtomicMultiMarker
SingleMarkerLike <|-- AtomicMarkerUnion
note for AtomicMultiMarker "Handles multiple marker constraints"
note for AtomicMarkerUnion "Handles union of marker constraints"
Flow diagram for Extra Marker Resolutionflowchart TD
A[Parse Extra Constraint] --> B{Constraint Type?}
B -->|Single| C[ExtraConstraint]
B -->|Multiple| D[ExtraMultiConstraint]
C --> E{Operation?}
D --> E
E -->|Intersect| F[Handle Intersect]
E -->|Union| G[Handle Union]
F --> H[Return Result]
G --> H
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
AtomicMultiMarker and AtomicMarkerUnion for extra markersAtomicMultiMarker and AtomicMarkerUnion for extra markers
There was a problem hiding this comment.
Hey @radoering - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please update the documentation to reflect the new extras handling behavior and performance characteristics
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dd1b814 to
b84db79
Compare
abn
approved these changes
Jan 18, 2025
Extra constraints are slightly different from standard generic constraints. For example, the standard generic constraint "==linux, ==win32" can never be satisfied (i.e., is "empty"), but the extra constraint "==extra1, ==extra2" can be satisfied (if both extras are requested).
b84db79 to
f39b0b3
Compare
This was referenced Jan 18, 2025
3 tasks
This file contains hidden or 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
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.
Relates to: python-poetry/poetry#9956
The downstream failures are caused by cosmetic changes. The tests are fixed in python-poetry/poetry#10073. We should probably merge this PR with failing downstream tests and switch from the released poetry-core version to the main branch afterwards.
This seems to mostly fix the performance regression reported in python-poetry/poetry#9956 - at least for the simplified repro. It might still not be enough for the real world examples...
Using the simplified repro from python-poetry/poetry#9956 (comment), Poetry 1.8 can solve it in 0.2 s, no matter if there is only one extra or 14 extras. Poetry 2 takes longer the more extras are defined:
Summary by Sourcery
Tests: