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: add support for AtomicMultiMarker and AtomicMarkerUnion for extra markers #818

Merged
merged 2 commits into from
Jan 18, 2025

Conversation

radoering
Copy link
Member

@radoering radoering commented Jan 18, 2025

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.

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

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:

[tool.poetry.dependencies]
python = ">=3.8,<3.11"
opencv-python = {version = "^4.6.0.66", optional = true}
ale-py = { version = "0.8.1", optional = true }

[tool.poetry.extras]
extra01 = ["ale-py"]
extra02 = ["ale-py"]
extra03 = ["ale-py"]
# ...
number of extras poetry 2.0.1 PR
1 0.5 s 0.5 s
2 1.7 s 0.6 s
3 6.4 s 0.6 s
4 19 s 0.6 s
5 57 s 0.6 s
... ... ...
14 ? 0.7 s

Summary by Sourcery

Tests:

  • Add tests for the new extra markers logic.

Copy link

sourcery-ai bot commented Jan 18, 2025

Reviewer's Guide by Sourcery

This pull request introduces AtomicMultiMarker and AtomicMarkerUnion to improve the performance of extra markers. It addresses a performance regression where the time to solve dependencies increased significantly with the number of extras defined. The changes involve new classes and modifications to existing constraint logic to handle extra markers more efficiently.

Class diagram for new Extra Constraint classes

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

Class diagram for Atomic Marker classes

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

Flow diagram for Extra Marker Resolution

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

File-Level Changes

Change Details Files
Added AtomicMultiMarker and AtomicMarkerUnion classes to represent extra markers.
  • Introduced AtomicMultiMarker to represent a conjunction of extra markers.
  • Introduced AtomicMarkerUnion to represent a disjunction of extra markers.
  • Modified __init__ to handle extra markers.
  • Added validate method to AtomicMultiMarker and AtomicMarkerUnion.
  • Added invert method to AtomicMultiMarker and AtomicMarkerUnion.
  • Added expand method to AtomicMultiMarker and AtomicMarkerUnion.
  • Added __str__ method to AtomicMultiMarker and AtomicMarkerUnion.
src/poetry/core/version/markers.py
Modified MultiConstraint and ExtraMultiConstraint to handle extra markers.
  • Added OPERATORS to MultiConstraint.
  • Modified intersect to handle extra markers.
  • Modified union to handle extra markers.
  • Added ExtraMultiConstraint to handle extra markers with == and != operators.
src/poetry/core/constraints/generic/multi_constraint.py
Modified Constraint and added ExtraConstraint to handle extra markers.
  • Added ExtraConstraint to represent extra markers with == and != operators.
  • Modified intersect to handle extra markers.
  • Modified union to handle extra markers.
src/poetry/core/constraints/generic/constraint.py
Modified parser to handle extra markers.
  • Added parse_extra_constraint to parse extra markers.
  • Modified parse_constraint to use _parse_constraint.
  • Modified _parse_single_constraint to handle extra markers.
src/poetry/core/constraints/generic/parser.py
Modified UnionConstraint to handle extra markers.
  • Modified invert to handle extra markers.
  • Modified intersect to handle extra markers.
  • Modified union to handle extra markers.
src/poetry/core/constraints/generic/union_constraint.py
Added tests for extra markers.
  • Added tests for invert with extra markers.
  • Added tests for intersect with extra markers.
  • Added tests for union with extra markers.
  • Added tests for parsing extra markers.
  • Added tests for markers with extra markers.
tests/constraints/generic/test_constraint.py
tests/version/test_markers.py
tests/constraints/generic/test_main.py
Modified Dependency to handle extra markers.
  • Modified marker to handle extra markers.
  • Added tests for dependency with extra markers.
src/poetry/core/packages/dependency.py
tests/packages/test_main.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

@radoering radoering changed the title perf: add support for AtomicMultiMarker and AtomicMarkerUnion for extra markers perf: add support for AtomicMultiMarker and AtomicMarkerUnion for extra markers Jan 18, 2025
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 - 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

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.

tests/version/test_markers.py Show resolved Hide resolved
tests/constraints/generic/test_constraint.py Show resolved Hide resolved
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).
@radoering radoering merged commit b1505c6 into python-poetry:main Jan 18, 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