perf: return least complex marker when building intersection by radoering · Pull Request #821 · python-poetry/poetry-core

radoering · 2025-01-19T21:34:35Z

This is analogous to what we do when building the union. Although this means extra work, it pays off if the resulting marker is used in another marker operation.

Relates to: python-poetry/poetry#9956

The downstream failures are caused by cosmetic changes. The tests are fixed in python-poetry/poetry#10077. We should probably merge this PR with failing downstream tests and switch from the released poetry-core version to the main branch afterwards.

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

After the simplified repro reported in python-poetry/poetry#9956 (comment) is almost as fast as before (see #818), I took a look at the example from python-poetry/poetry#9956 (comment), which is still significantly slower. With this PR, it is about as fast as before:

poetry version	lock (--regenerate)	lock (--no-update)
1.8	178 s	15 s
main	>600 s	120 s
PR	151 s	19 s

Summary by Sourcery

Optimize marker intersection performance by returning the least complex marker representation. This addresses performance regressions observed when intersecting markers.

Bug Fixes:

Fixed a performance issue where marker intersection could be significantly slower in certain cases.

Tests:

Added tests to verify the performance improvements of marker intersection.

Summary by Sourcery

Optimize marker intersection performance by returning the least complex marker representation.

Bug Fixes:

Fixed a performance issue where marker intersection could be significantly slower in certain cases.

Tests:

Added tests to verify the performance improvements.

sourcery-ai · 2025-01-19T21:34:39Z

Reviewer's Guide by Sourcery

This pull request optimizes marker intersection performance by returning the least complex marker representation. This addresses performance regressions observed when intersecting markers.

Class diagram for marker hierarchy and operations

classDiagram
    class BaseMarker {
        +complexity
        +exclude(marker_name)
        +only(marker_names)
    }
    class MultiMarker {
        +markers
        +exclude(marker_name)
        +only(marker_names)
    }
    class MarkerUnion {
        +markers
        +exclude(marker_name)
        +only(marker_names)
    }
    BaseMarker <|-- MultiMarker
    BaseMarker <|-- MarkerUnion
    note for MultiMarker "Modified exclude() to use intersection"
    note for MarkerUnion "Modified exclude() to use union"

File-Level Changes

Change	Details	Files
Optimize marker intersection by returning the least complex marker representation.	Modified the `intersection` function to return the least complex marker representation among the DNF, CNF, and unnormalized forms.	`src/poetry/core/version/markers.py`
Updated tests to reflect the change in marker intersection.	Modified test cases to assert the new expected output of marker intersections. Added a new test case for marker exclusion.	`tests/version/test_markers.py`
Modified the `exclude` function to use `intersection` instead of `of`.	Changed the return statement in the `exclude` function to use `intersection` instead of `of`.	`src/poetry/core/version/markers.py`
Modified the `exclude` function in `MarkerUnion` to use `union` instead of `of`.	Changed the return statement in the `exclude` function to use `union` instead of `of`.	`src/poetry/core/version/markers.py`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @radoering - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai

Hey @radoering - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟡 Complexity: 1 issue found
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

src/poetry/core/version/markers.py

abn

Minor typo.

src/poetry/core/version/markers.py

This is analogous to what we do when building the union. Although this means extra work, it pays off if the resulting marker is used in another marker operation.

Otherwise, the expectations in the export tests had to be changed due to the previous commit.

sourcery-ai bot reviewed Jan 19, 2025

View reviewed changes

radoering marked this pull request as draft January 19, 2025 21:51

radoering marked this pull request as ready for review January 19, 2025 22:28

sourcery-ai bot reviewed Jan 19, 2025

View reviewed changes

src/poetry/core/version/markers.py Show resolved Hide resolved

radoering force-pushed the perf-marker-intersection branch from cfb877c to f00b7cb Compare January 19, 2025 22:32

radoering mentioned this pull request Jan 20, 2025

Resolving dependencies in 2.0 takes forever python-poetry/poetry#9956

Closed

abn requested changes Jan 20, 2025

View reviewed changes

src/poetry/core/version/markers.py Outdated Show resolved Hide resolved

abn mentioned this pull request Jan 20, 2025

tests: adapt tests to cosmetic changes caused by poetry-core#821 python-poetry/poetry#10077

Merged

2 tasks

radoering added 2 commits January 20, 2025 16:14

perf: return least complex marker when building intersection

0831ffa

This is analogous to what we do when building the union. Although this means extra work, it pays off if the resulting marker is used in another marker operation.

return least complex marker in exclude (for consistency)

d5f6120

Otherwise, the expectations in the export tests had to be changed due to the previous commit.

radoering force-pushed the perf-marker-intersection branch from f00b7cb to d5f6120 Compare January 20, 2025 15:15

abn approved these changes Jan 20, 2025

View reviewed changes

abn merged commit d97577e into python-poetry:main Jan 20, 2025
17 of 18 checks passed

radoering added a commit to radoering/poetry-core that referenced this pull request Feb 2, 2025

fix RecursionError introduced in python-poetry#821

33bb7e2

radoering added a commit to radoering/poetry-core that referenced this pull request Feb 2, 2025

fix endless recursion introduced in python-poetry#821

b44d832

radoering mentioned this pull request Feb 2, 2025

fix endless recursion introduced in #821 #832

Merged

2 tasks

radoering added a commit to radoering/poetry-core that referenced this pull request Feb 2, 2025

fix endless recursion introduced in python-poetry#821

588afc4

abn pushed a commit that referenced this pull request Feb 2, 2025

fix endless recursion introduced in #821

7ace184

finswimmer mentioned this pull request Feb 6, 2025

release: bump version to 2.1.0 #834

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

perf: return least complex marker when building intersection#821

perf: return least complex marker when building intersection#821
abn merged 2 commits intopython-poetry:mainfrom
radoering:perf-marker-intersection

radoering commented Jan 19, 2025 •

edited by sourcery-ai bot

Loading

Uh oh!

sourcery-ai bot commented Jan 19, 2025 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Uh oh!

sourcery-ai bot left a comment

Uh oh!

Uh oh!

abn left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

radoering commented Jan 19, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide by Sourcery

Class diagram for marker hierarchy and operations

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

radoering commented Jan 19, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jan 19, 2025 •

edited

Loading