Skip to content
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 union of multi markers #308

Merged
merged 1 commit into from
Apr 30, 2022

Conversation

radoering
Copy link
Member

Relates-to: python-poetry/poetry#5192

  • Added tests for changed code.
  • Updated documentation for changed code.

Improves the union of multi markers with all kinds of markers. See added test cases for new simplifications that were not possible before.

Improving the union of multi markers is similar but a bit more complicated than #226. That's because both MarkerUnion.intersect() and MarkerUnion.union() as well as MultiMarker.union() prefer to return a MarkerUnion of MultiMarkers instead of a MultiMarker of MarkerUnions. Therefore, in addition to the standard MultiMarker.union() method a MultiMarker.union_simplify() method is introduced. This method prefers to return a MultiMarker instead of a MarkerUnion, but returns None if no simplification is possible in order to avoid an endless recursion.

@radoering
Copy link
Member Author

@dimbleby I wonder if you might review this change? Even if your review doesn't affect mergeability, I think it could increase confidence and speed up the process, since you have already made some changes in that direction and seem to have a good understanding of marker handling.

@radoering radoering requested a review from a team March 24, 2022 19:07
@dimbleby
Copy link
Contributor

This is all getting quite elaborate, isn't it?

I don't see a way in which this is not correct, it clearly does achieve some new simplifications and the test coverage is reassuring.

However... I must admit that I am finding it quite hard to follow!

We are beginning to reach diminishing returns on these marker simplifications: this one is a fairly significant increase in overall complexity, for relatively modest gains.

To my taste - and probably this is a matter of taste, reasonable people might disagree - I think it's not worth it.

But that's not a hill that I would want to die on, I don't intend to argue about it. As I say, I don't see bugs.

@radoering
Copy link
Member Author

I can understand your concerns and I agree that it is a significant increase in complexity but I think it is a significant gain, too. If it was just about cosmetics I might even doubt whether it is worth it. But the main reason why marker simplifications are necessary is that they lay the foundations for reducing the number of overrides (regarding python-poetry/poetry#4695) - and that is not just a performance issue.

Any omitted marker simplification can lead to an unnecessary override and any unnecessary override can lead to an unnecessary failure in dependency resolution because the solver tries to find a solution for a case that is not relevant due to markers that can never be fulfilled. Considering that users experiencing an unfounded solver failure sometimes can't do anything but reducing the number of supported python versions, I think it's worth it.

Nevertheless, thanks for the review. If someone comes up with a simpler solution, I will be happy to adopt it. If there isn't one, at least in my opinion a complex solution is better than no solution at all.

@radoering radoering force-pushed the improve-union-of-multi-markers branch from b8b63e6 to f8ae8bd Compare April 1, 2022 16:37
@sonarcloud
Copy link

sonarcloud bot commented Apr 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
1.7% 1.7% Duplication

@abn abn merged commit 8c75c1a into python-poetry:master Apr 30, 2022
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.

3 participants