feat: Support PEP 735 dependency groups #823
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR implements full support for PEP 735 top-level dependency-groups by reading and normalizing the new “dependency-groups” table, resolving include-group references (with cycle and missing-group checks), merging with existing tool.poetry.group settings, and propagating group metadata into created Dependency instances; it also adds validation via a JSON schema and comprehensive positive and negative tests. ER diagram for dependency-groups and group mergingerDiagram
PROJECT ||--o{ DEPENDENCY_GROUPS : has
DEPENDENCY_GROUPS ||--o{ DEPENDENCY : contains
PROJECT ||--o{ TOOL_POETRY_GROUP : has
TOOL_POETRY_GROUP ||--o{ DEPENDENCY : contains
DEPENDENCY_GROUPS ||--o{ INCLUDE_GROUP : includes
INCLUDE_GROUP {
string include-group
}
DEPENDENCY {
string name
string version
string[] groups
}
Class diagram for updated Dependency creation and group propagationclassDiagram
class Dependency {
+with_groups(groups: Iterable[str]) Dependency
+create_from_pep_508(name: str, relative_to: Path|None = None, groups: Iterable[str]|None = None) Dependency
marker
_constraint
}
class FileDependency {
<<Dependency>>
+groups: Iterable[str]|None
}
class DirectoryDependency {
<<Dependency>>
+groups: Iterable[str]|None
}
class VCSDependency {
<<Dependency>>
+groups: Iterable[str]|None
}
class URLDependency {
<<Dependency>>
+groups: Iterable[str]|None
}
Dependency <|-- FileDependency
Dependency <|-- DirectoryDependency
Dependency <|-- VCSDependency
Dependency <|-- URLDependency
class ProjectPackage {
+add_dependency_group(group: DependencyGroup)
+has_dependency_group(group_name: str) bool
+dependency_group(group_name: str) DependencyGroup
extras
}
class DependencyGroup {
+add_dependency(dep: Dependency)
name
optional
}
ProjectPackage "1" o-- "*" DependencyGroup
DependencyGroup "1" o-- "*" Dependency
Class diagram for Factory dependency group normalization and resolutionclassDiagram
class Factory {
+_normalize_dependency_group_names(dependency_groups: dict) dict
+_resolve_dependency_group_includes(dependency_groups: dict) dict
+_configure_package_dependencies(package, project, tool_poetry, dependency_groups, with_groups)
}
Factory ..> ProjectPackage
Factory ..> DependencyGroup
Factory ..> Dependency
Flow diagram for dependency group normalization and include resolutionflowchart TD
A[Read dependency-groups from pyproject] --> B[Normalize group names]
B --> C[Check for duplicate group names]
C --> D[Resolve include-group references]
D --> E{Cycle or missing group?}
E -- Yes --> F[Raise ValueError]
E -- No --> G[Merge with tool.poetry.group]
G --> H[Create DependencyGroup and add to package]
H --> I[Propagate group info to Dependency instances]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
finswimmer
force-pushed
the
poc-pep-735
branch
6 times, most recently
from
83710eb to
6473ff4
Compare
finswimmer
force-pushed
the
poc-pep-735
branch
from
6473ff4 to
e442bb4
Compare
3 tasks
finswimmer
force-pushed
the
poc-pep-735
branch
2 times, most recently
from
2d7ac12 to
0a50ac3
Compare
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add documentation for the new dependency groups feature - this is a significant change that needs to be well documented for users
- Link to the relevant issue number/tracking issue for PEP 735 implementation in the PR description
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
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
finswimmer
force-pushed
the
poc-pep-735
branch
from
c19b017 to
ed77a26
Compare
|
@sourcery-ai review |
finswimmer
changed the title
2 tasks
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider consolidating the two loops in _configure_package_dependencies that handle dependency groups (one for dependency-groups and one for tool.poetry.group) to reduce repeated logic.
- If a dependency group exists in the pyproject data but lacks a corresponding tool.poetry.group entry, consider whether you want to apply default optional settings explicitly.
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
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
finswimmer
force-pushed
the
poc-pep-735
branch
from
ed77a26 to
c34e8b2
Compare
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider consolidating the two loops in _configure_package_dependencies to reduce code duplication in processing dependency groups.
- Validate and normalize the dependency groups input early to ensure consistent types before iterating over them.
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.
tests/test_factory.py
Outdated
| @@ -844,6 +869,19 @@ def test_create_poetry_with_invalid_dev_dependencies(caplog: LogCaptureFixture) | |||
| assert any("dev" in r.groups for r in poetry.package.all_requires) | |||
|
|
|||
|
|
|||
| def test_create_poetry_with_invalid_dependency_groups() -> None: | |||
There was a problem hiding this comment.
suggestion (testing): Test more invalid group configurations.
The current test only covers one invalid case. Consider adding tests for other invalid scenarios, such as incorrect data types for dependency names, invalid characters in group names, or missing required fields.
Suggested implementation:
def test_create_poetry_with_invalid_dependency_groups() -> None:
with pytest.raises(RuntimeError) as e:
_ = Factory().create_poetry(
fixtures_dir / "project_with_invalid_dependency_groups", with_groups=False
)
expected = """\
The Poetry configuration is invalid:
- dependency-groups.testing[1] must be string
"""
assert str(e.value) == expected
def test_create_poetry_with_invalid_dependency_names() -> None:
with pytest.raises(RuntimeError) as e:
_ = Factory().create_poetry(
fixtures_dir / "project_with_invalid_dependency_names"
)
expected = """\
The Poetry configuration is invalid:
- dependencies.some_dependency must be string, got int
"""
assert str(e.value) == expected
def test_create_poetry_with_invalid_group_names() -> None:
with pytest.raises(RuntimeError) as e:
_ = Factory().create_poetry(
fixtures_dir / "project_with_invalid_group_names"
)
expected = """\
The Poetry configuration is invalid:
- dependency-groups.invalid-group-name! contains invalid characters
"""
assert str(e.value) == expected
def test_create_poetry_with_missing_required_fields() -> None:
with pytest.raises(RuntimeError) as e:
_ = Factory().create_poetry(
fixtures_dir / "project_with_missing_required_fields"
)
expected = """\
The Poetry configuration is invalid:
- dependency 'essential' is missing required field 'version'
"""
assert str(e.value) == expectedMake sure to create the corresponding fixture projects under the fixtures directory for each invalid configuration:
- project_with_invalid_dependency_names
- project_with_invalid_group_names
- project_with_missing_required_fields
These changes allow tests to cover multiple invalid scenarios as suggested.
radoering
reviewed
Feb 8, 2025
4 tasks
finswimmer
force-pushed
the
poc-pep-735
branch
from
c34e8b2 to
c501df5
Compare
radoering
approved these changes
Feb 9, 2025
radoering
requested changes
Feb 9, 2025
There was a problem hiding this comment.
Support for https://peps.python.org/pep-0735/#dependency-object-specifiers is missing, which might be confusing for users.
2 tasks
|
I'm sure there are many who are eagerly awaiting this, thank you. |
citelao
reviewed
May 4, 2025
|
Has development migrated to #837? Or is this PR blocked on that one? I'm happy to do some work to help this come in. |
There was a problem hiding this comment.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
- The newly added dependency-groups-schema.json file appears empty; please populate it with the actual JSON schema so that validation of
dependency-groupsworks correctly. - _configure_package_dependencies is becoming quite large and does multiple passes over groups—consider extracting group initialization, constraint parsing, and include handling into smaller helper methods to improve readability and maintainability.
- _resolve_dependency_group_includes currently mutates the input dict in a loop for cycle detection; switching to a dedicated graph‐based algorithm or working on a copy could make the cycle detection logic clearer and side‐effect free.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The newly added dependency-groups-schema.json file appears empty; please populate it with the actual JSON schema so that validation of `dependency-groups` works correctly.
- _configure_package_dependencies is becoming quite large and does multiple passes over groups—consider extracting group initialization, constraint parsing, and include handling into smaller helper methods to improve readability and maintainability.
- _resolve_dependency_group_includes currently mutates the input dict in a loop for cycle detection; switching to a dedicated graph‐based algorithm or working on a copy could make the cycle detection logic clearer and side‐effect free.
## Individual Comments
### Comment 1
<location> `src/poetry/core/factory.py:453` </location>
<code_context>
+ ) -> dict[NormalizedName, list[NormalizedName]]:
+ resolved_groups: set[NormalizedName] = set()
+ included: dict[NormalizedName, list[NormalizedName]] = defaultdict(list)
+ while resolved_groups != set(dependency_groups):
+ for group, dependencies in dependency_groups.items():
+ if group in resolved_groups:
</code_context>
<issue_to_address>
Possible infinite loop in _resolve_dependency_group_includes if dependency_groups is malformed.
Consider adding a maximum iteration limit or an explicit error if the loop cannot make progress, to prevent potential infinite loops with malformed input.
</issue_to_address>
### Comment 2
<location> `src/poetry/core/factory.py:466` </location>
<code_context>
+ resolved_dependencies.append(dep)
+ else:
+ included_group = canonicalize_name(dep["include-group"])
+ if included_group in included[group]:
+ raise ValueError(
+ f"Cyclic dependency group include:"
</code_context>
<issue_to_address>
Cyclic dependency group detection may not catch all cycles.
The current logic only detects direct cycles. To handle indirect cycles, implement a more comprehensive cycle detection algorithm, such as depth-first search with a visited set.
</issue_to_address>
### Comment 3
<location> `src/poetry/core/factory.py:700` </location>
<code_context>
]
result["errors"] += tool_poetry_validation_errors
+ dependency_groups = toml_data.get("dependency-groups")
+ if dependency_groups is not None:
+ dependency_groups_validation_errors = [
</code_context>
<issue_to_address>
Validation error messages for dependency-groups may be unclear.
Replacing 'data' with 'dependency-groups' in error messages may not always yield clear results, especially for complex schema errors. Please review the formatting to ensure messages remain accurate and user-friendly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
dependency_groups_validation_errors = [
e.replace("data", "dependency-groups")
for e in validate_object(dependency_groups, "dependency-groups-schema")
]
=======
dependency_groups_validation_errors = [
(
f"dependency-groups: {e[5:]}" if e.startswith("data:") else
e.replace("data", "dependency-groups", 1) if e.startswith("data") else
e
)
for e in validate_object(dependency_groups, "dependency-groups-schema")
]
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/poetry/core/packages/dependency.py:338` </location>
<code_context>
+ @classmethod
+ def _normalize_dependency_group_names(
+ cls,
+ dependency_groups: dict[str, list[str | dict[str, str]]],
+ ) -> dict[NormalizedName, list[str | dict[str, str]]]:
</code_context>
<issue_to_address>
The groups parameter in create_from_pep_508 is not documented.
Please update the docstring to include the groups parameter for clarity.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ) -> dict[NormalizedName, list[NormalizedName]]: | ||
| resolved_groups: set[NormalizedName] = set() | ||
| included: dict[NormalizedName, list[NormalizedName]] = defaultdict(list) | ||
| while resolved_groups != set(dependency_groups): |
There was a problem hiding this comment.
issue (bug_risk): Possible infinite loop in _resolve_dependency_group_includes if dependency_groups is malformed.
Consider adding a maximum iteration limit or an explicit error if the loop cannot make progress, to prevent potential infinite loops with malformed input.
| resolved_dependencies.append(dep) | ||
| else: | ||
| included_group = canonicalize_name(dep["include-group"]) | ||
| if included_group in included[group]: |
There was a problem hiding this comment.
issue (bug_risk): Cyclic dependency group detection may not catch all cycles.
The current logic only detects direct cycles. To handle indirect cycles, implement a more comprehensive cycle detection algorithm, such as depth-first search with a visited set.
Comment on lines
+702
to
+705
| dependency_groups_validation_errors = [ | ||
| e.replace("data", "dependency-groups") | ||
| for e in validate_object(dependency_groups, "dependency-groups-schema") | ||
| ] |
There was a problem hiding this comment.
suggestion: Validation error messages for dependency-groups may be unclear.
Replacing 'data' with 'dependency-groups' in error messages may not always yield clear results, especially for complex schema errors. Please review the formatting to ensure messages remain accurate and user-friendly.
Suggested change
| dependency_groups_validation_errors = [ | |
| e.replace("data", "dependency-groups") | |
| for e in validate_object(dependency_groups, "dependency-groups-schema") | |
| ] | |
| dependency_groups_validation_errors = [ | |
| ( | |
| f"dependency-groups: {e[5:]}" if e.startswith("data:") else | |
| e.replace("data", "dependency-groups", 1) if e.startswith("data") else | |
| e | |
| ) | |
| for e in validate_object(dependency_groups, "dependency-groups-schema") | |
| ] |
Comment on lines
+338
to
+341
| cls, | ||
| name: str, | ||
| relative_to: Path | None = None, | ||
| groups: Iterable[str] | None = None, |
There was a problem hiding this comment.
nitpick: The groups parameter in create_from_pep_508 is not documented.
Please update the docstring to include the groups parameter for clarity.
Comment on lines
+438
to
+442
| errors = [] | ||
| for normed_name, names in original_names.items(): | ||
| if len(names) > 1: | ||
| errors.append(f"{normed_name} ({', '.join(names)})") | ||
| if errors: |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Convert for loop into list comprehension (
list-comprehension) - Use named expression to simplify assignment and conditional (
use-named-expression)
Suggested change
| errors = [] | |
| for normed_name, names in original_names.items(): | |
| if len(names) > 1: | |
| errors.append(f"{normed_name} ({', '.join(names)})") | |
| if errors: | |
| if errors := [ | |
| f"{normed_name} ({', '.join(names)})" | |
| for normed_name, names in original_names.items() | |
| if len(names) > 1 | |
| ]: |
Comment on lines
+472
to
+480
| try: | ||
| resolved_dependencies.extend( | ||
| dependency_groups[included_group] | ||
| ) | ||
| except KeyError: | ||
| raise ValueError( | ||
| f"Dependency group '{included_group}'" | ||
| f" (included in '{group}') not found" | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
| @@ -335,7 +335,10 @@ | |||
|
|
|||
| @classmethod | |||
| def create_from_pep_508( | |||
There was a problem hiding this comment.
issue (code-quality): Low code quality found in Dependency.create_from_pep_508 - 7% (low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Required-by: python-poetry/poetry#10130
Relates-to: python-poetry/poetry#9751
Summary by Sourcery
New Features:
Summary by Sourcery
Implement full PEP 735 support for dependency-groups by parsing the new section, validating its schema, normalizing names, resolving includes with cycle detection, and propagating group membership through dependency creation, accompanied by tests for valid and error cases.
New Features:
Enhancements:
Tests: