Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Executor types and method contracts snakemake_interface_executor_plugins/jobs.py, snakemake_interface_executor_plugins/executors/*, snakemake_interface_executor_plugins/base.py, snakemake_interface_executor_plugins/scheduler.py, snakemake_interface_executor_plugins/workflow.py, .github/workflows/test.yml |
Executor classes gain type annotations. get_target_spec now returns a list of TargetSpec values. RemoteExecutor.get_job_args accepts and forwards keyword arguments. Workflow and scheduler interfaces add properties and callbacks. The Mypy step runs pixi run --environment dev type-check with if: always(). |
Shared-filesystem choice parsing snakemake_interface_executor_plugins/settings.py, tests/tests.py |
_parse_choices_into accepts a list of choices and returns the specified container type. Tests cover list and set results, valid choices, invalid combinations with none, and forwarding remote job arguments. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Other
Suggested reviewers: johanneskoester
Merge Risk: 
70030
-
The changes are ready to merge after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the primary change: resolving mypy issues across executor interfaces and implementations. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 5 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.gitignore (1)
162-165: Clarify and Confirm Ignore Entries for Package Management TransitionThe new entries for
poetry.lock,.pixi, and*.egg-infoclearly reflect the shift toward Pixi-based management. Please confirm that:
- Keeping
poetry.lock(now un-commented) in the ignore list is intentional, especially if the project is fully transitioning away from Poetry.- Ignoring
.pixiand*.egg-infomeets the desired artifact cleanup goals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pixi.lockis excluded by!**/*.lockpyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (10)
.gitattributes(1 hunks).github/workflows/release-please.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks)snakemake_interface_executor_plugins/executors/base.py(2 hunks)snakemake_interface_executor_plugins/executors/real.py(3 hunks)snakemake_interface_executor_plugins/executors/remote.py(4 hunks)snakemake_interface_executor_plugins/jobs.py(2 hunks)snakemake_interface_executor_plugins/scheduler.py(1 hunks)snakemake_interface_executor_plugins/workflow.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
snakemake_interface_executor_plugins/workflow.pysnakemake_interface_executor_plugins/scheduler.pysnakemake_interface_executor_plugins/jobs.pysnakemake_interface_executor_plugins/executors/real.pysnakemake_interface_executor_plugins/executors/remote.pysnakemake_interface_executor_plugins/executors/base.py
🔇 Additional comments (33)
snakemake_interface_executor_plugins/jobs.py (2)
8-8: Added appropriate imports for type annotations.The imports for
ListfromtypingandTargetSpecfromsnakemake_interface_executor_plugins.utilshave been correctly added to support the type signature changes.Also applies to: 11-11
62-62: Return type correction aligns with actual implementation.The return type of
get_target_spec()has been updated fromstrtoList[TargetSpec]to properly reflect the actual implementation, fixing the mypy type error mentioned in the PR description.snakemake_interface_executor_plugins/workflow.py (3)
24-24: Added import for ExecutorSettingsBase.The import for
ExecutorSettingsBasehas been correctly added to support the new abstract property.
57-59: Added required abstract property for executor settings.The
executor_settingsproperty has been added to the interface, properly typed as returningExecutorSettingsBase. This fixes one of the mypy errors mentioned in the PR objectives.
81-83: Added required abstract property for dag.The
dagproperty has been added to the interface, addressing the missing "dag" attribute inWorkflowExecutorInterfacementioned in the PR description.snakemake_interface_executor_plugins/executors/base.py (3)
24-25: Added proper type annotations for class attributes.Added type annotations for
workflowandloggerattributes, improving type safety and making the class structure clearer.
33-35: Added informative comment about type limitations.The comment explaining why
dagcannot be type-annotated (because the type is defined in the Snakemake framework) is helpful for maintaining the code in the future.
54-55: Fixed logical condition in resource filtering.The condition for filtering resources has been corrected by separating the
isdigit(value)check from the resource exclusion check, which fixes a potential logical error in the code.snakemake_interface_executor_plugins/scheduler.py (2)
7-8: Added necessary imports for type annotations.The imports for
CallablefromtypingandJobExecutorInterfacehave been correctly added to support the type annotations for the callback attributes.
12-14: Added missing callback attributes with proper type annotations.The
submit_callback,finish_callback, anderror_callbackattributes have been added with appropriate type annotations, addressing the missing attributes mentioned in the PR description.snakemake_interface_executor_plugins/executors/real.py (3)
7-7: Good change from Dict to Mapping for better type flexibility.Changing from
DicttoMappingis a good practice as it focuses on the interface rather than the implementation, allowing more flexibility in the return types while maintaining type safety.
24-28: Well-structured type annotations added to class attributes.These type annotations correctly document the expected types for class attributes, making the code more maintainable and helping address the mypy issues mentioned in the PR objectives.
185-185: Appropriate return type update to Mapping.The return type annotation change from
Dict[str, str]toMapping[str, str]aligns with the import change and provides better API design. This addresses part of the type mismatch issues mentioned in the PR objectives.snakemake_interface_executor_plugins/executors/remote.py (4)
15-15: Good addition of Optional import for nullable types.Adding
Optionalto the imports correctly supports the type annotations for attributes that may beNone.
39-52: Comprehensive type annotations added to class attributes.These thorough type annotations clearly document the expected types for all class attributes, addressing the mypy issues mentioned in the PR objectives. The
Optionalannotation is correctly used for attributes that can beNone.
144-144: **Method signature updated to include kwargs parameter.Adding the
**kwargsparameter toget_job_argsimproves compatibility with the parent class and consistency across the codebase. This addresses the incompatible method signatures issue mentioned in the PR objectives.
134-134:Details
✅ Verification successful
Type ignore comment added to ExecMode return.
The
# type: ignorecomment bypasses type checking for this line. While generally such comments should be avoided, it's acceptable here if there's a legitimate reason why standard type checking doesn't work for this specific return.
🏁 Script executed:
#!/bin/bash # Check the implementation of ExecMode in the codebase to understand why type: ignore is needed # Look for ExecMode definition rg -A 5 "class ExecMode" --type pyLength of output: 473
Clarification on the Use of
# type: ignoreforExecMode.REMOTEThe current implementation:
return ExecMode.REMOTE # type: ignoreis acceptable. The
ExecModeis defined insnakemake_interface_executor_plugins/settings.pyas a subclass ofSettingsEnumBase, and the type checker struggles with this dynamic enum, which necessitates bypassing type checks for this return statement.
- Suggestion (Optional):
- Consider adding a brief inline note or linking to a related issue for future reference, clarifying why standard type checking fails in this specific instance.
.gitattributes (1)
1-2: Proper Configuration for Pixi Lock FileThe entry for
pixi.lockis set to use a binary merge strategy and provides YAML syntax highlighting while marking it as generated. This configuration should effectively prevent merge conflicts on the lock file..github/workflows/release-please.yml (5)
20-24: Renaming and Conditional Execution in 'publish-pypi' JobThe new
publish-pypijob clearly communicates its role in publishing to PyPI. The use ofneeds: release-pleasealong with the conditional${{ needs.release-please.outputs.release_created }}ensures that publishing only occurs when a release has been created.
25-27: Updated Checkout ActionSwitching to
actions/checkout@v4is a welcome update that leverages the latest improvements in the checkout process.
28-33: Install Pixi Step for Build Environment SetupReplacing the Poetry installation with the Pixi setup using
prefix-dev/setup-pixi@v0.8.3(withenvironments: publishand a pinnedpixi-version: v0.42.1) is clearly aligned with the project’s toolchain transition. Ensure the version pinning is deliberate for build reproducibility.
34-38: Build Step Using PixiRenaming the step to "Build source and wheel distribution + check build" and using the command
pixi run --environment publish check-buildeffectively captures the new build process. The inline comment about building into thedist/directory is additionally helpful.
39-44: Publish Step for DistributionThe updated publishing step now employs
pypa/gh-action-pypi-publish@release/v1with secure references forPYPI_USERNAMEandPYPI_PASSWORD. This change streamlines the distribution process under the new workflow..github/workflows/test.yml (10)
10-15: Set Job Permissions ExplicitlyThe added permission settings (read for contents; write for checks, issues, and pull-requests) are clearly defined and help ensure that the workflow has the appropriate access for its operations.
20-21: Checkout Code for Quality-ControlUsing
actions/checkout@v4in the quality-control job is properly updated to leverage the latest checkout improvements.
23-29: Installation of Pixi in Quality-Control JobThe step that installs Pixi with
prefix-dev/setup-pixi@v0.8.3using the development environment (with caching enabled) is well configured. Consistency with the release workflow reinforces the project’s transition away from Poetry.
30-34: Ruff Format Step in Quality-ControlThe Ruff Format check using the command
pixi run --environment dev format --check(withif: always()) is an effective way to enforce code style standards, ensuring this step executes regardless of previous failures.
35-39: Ruff Lint Step in Quality-ControlThe lint check using
pixi run --environment dev lint --diffis a solid approach to capture potential code issues. The unconditional execution (if: always()) reinforces its reliability in the quality control process.
40-44: Mypy Type-Checking in Quality-ControlIncluding a dedicated Mypy step with
pixi run --environment dev type-checkdirectly addresses the primary PR objectives regarding type safety. This ensures that any type-related issues are flagged during the CI cycle.
45-47: Quality Control Summary StepThe final "Collect QC" step, which simply echoes a success message, confirms the completion of quality checks. If more detailed reporting becomes necessary later, consider expanding this step.
51-52: Testing Job: Checkout StepThe testing job also benefits from the updated checkout action (
actions/checkout@v4), ensuring consistent behavior across workflows.
53-59: Install Pixi Step in Testing JobThis step mirrors the preparation in the quality-control job by installing Pixi with the specified version, environment, and caching enabled. Consistency here is key to maintaining the unified setup across jobs.
60-62: Execute Test Suite via PixiThe test execution command (
pixi run --environment dev test --show-capture=all -s -vv) is sufficiently detailed and should provide comprehensive output for debugging. The verbosity flags will be helpful during test failures.
|
Is there no caching in the pixi action without the lock file? I don't think the lock file should be in the repo in case of libraries. |
Keep the upstream version check and the PR's typing fixes while enabling the CI mypy job. Match the shared settings parser signature, and forward remote job kwargs so `cores` overrides still reach the parent. Add tests for both behaviors. Verified: mypy found no issues in 17 files, Ruff format/lint pass, 10 tests pass at 61.59% coverage. Oracle approved; upstream CI and maintainer review remain the merge gates.
Both PR jobs fail in setup-pixi before tests because this repository does not track pixi.lock. Keep the mypy quality gate, but restore the upstream no-cache setup until a lockfile is deliberately committed. Verified: job 124976573330 reports ENOENT on pixi.lock; workflow YAML parses and git diff --check passes. Local mypy, Ruff and 10 tests passed before this CI-only edit.
|
I refreshed this branch against current main without rewriting its history. Mypy now reports no issues in 17 files; Ruff and all 10 tests pass. The update also preserves |
Errors to address:
Summary by CodeRabbit
"none"when combined with other choices.