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

perf: return least complex marker when building intersection #821

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

radoering
Copy link
Member

@radoering radoering commented Jan 19, 2025

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.

Copy link

sourcery-ai bot commented Jan 19, 2025

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"
Loading

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

Copy link

@sourcery-ai sourcery-ai bot left a 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 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.

@radoering radoering marked this pull request as draft January 19, 2025 21:51
@radoering radoering marked this pull request as ready for review January 19, 2025 22:28
Copy link

@sourcery-ai sourcery-ai bot left a 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 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 Show resolved Hide resolved
Copy link
Member

@abn abn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo.

src/poetry/core/version/markers.py Outdated Show resolved Hide resolved
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.
@radoering radoering force-pushed the perf-marker-intersection branch from f00b7cb to d5f6120 Compare January 20, 2025 15:15
@abn abn merged commit d97577e into python-poetry:main Jan 20, 2025
17 of 18 checks passed
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.

2 participants