-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
improve performance for merging marker overrides - volume 2 #10111
improve performance for merging marker overrides - volume 2 #10111
Conversation
Reviewer's Guide by SourceryThis pull request improves the performance of merging marker overrides by adding a shortcut for cases where the markers are the same for all overrides. Additionally, it introduces a function to remove a marker from another marker, which helps to hit the performance shortcut more often. Sequence diagram for improved marker override mergingsequenceDiagram
participant Solver
participant MergeOverride
participant RemoveMarker
Solver->>MergeOverride: merge_override_packages()
Note over MergeOverride: Process each package
loop For each package override
MergeOverride->>RemoveMarker: remove_other_from_marker()
RemoveMarker-->>MergeOverride: simplified marker
end
Note over MergeOverride: Check if markers are same
alt Markers are same
Note over MergeOverride: Use performance shortcut
else Markers differ
Note over MergeOverride: Use fallback algorithm
end
MergeOverride-->>Solver: merged packages
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @radoering - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add documentation for the marker simplification optimization, as it changes how markers are generated and could affect downstream users.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 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.
Pull Request Check List
Related to: #9956
Testing with the
pyproject.toml
from #9956 (comment):Getting closer...
By the way, this PR increases the probability to get more concise markers as can be seen in the test case (or the mentioned
pyproject.toml
).Summary by Sourcery
Tests: