fix: address mypy issues #83
Conversation
📝 WalkthroughWalkthroughThis pull request implements several updates across configuration, workflow, and executor modules. The changes include adding a new entry in the Git attributes to handle the Changes
Sequence Diagram(s)sequenceDiagram
participant Event as "GitHub Event"
participant Runner as "GitHub Runner"
participant Checkout as "Checkout Action (v4)"
participant Setup as "Pixi Setup (v0.8.3)"
participant Build as "Build (pixi check-build)"
participant Publish as "Publish to PyPI"
Event->>Runner: Trigger release
Runner->>Checkout: Checkout code
Checkout->>Setup: Set up Pixi environment
Setup->>Build: Execute build step
Build->>Publish: Publish distribution
sequenceDiagram
participant Event as "GitHub Event"
participant Runner as "GitHub Runner"
participant QC as "Quality-Control Job"
participant Test as "Testing Job"
Event->>Runner: Trigger test workflow
Runner->>QC: Start Quality-Control Job
QC->>QC: Set permissions & Checkout (v4)
QC->>QC: Install Pixi (dev)
QC->>QC: Run Ruff Format, Ruff Lint, Mypy
QC->>QC: Complete quality checks
Runner->>Test: Start Testing Job
Test->>Test: Checkout (v4) & Install Pixi
Test->>Test: Execute test suite
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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:✅ 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. |
Errors to address:
Summary by CodeRabbit
Chores
Tests
Refactor
Overall, these improvements contribute to a more stable and efficient release process for end users.