refactor!: executor settings moved into direct attribute of the executor base class #96
Conversation
📝 WalkthroughWalkthroughAbstractExecutor now accepts and stores an optional ExecutorSettingsBase; RealExecutor was updated to accept and forward executor_settings, add a post_init hook and call it conditionally; WorkflowExecutorInterface gains a read-only dag property; DeploymentMethod enum removed and deployment settings now use Set[str]; README example adjusted to use self.executor_settings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant RE as RealExecutor
participant AE as AbstractExecutor
participant WF as WorkflowExecutorInterface
Caller->>RE: __init__(workflow, logger, executor_settings?, post_init=True)
RE->>AE: super().__init__(workflow, logger, executor_settings)
note right of AE: stores self.executor_settings
alt post_init == True
RE->>RE: __post_init__()
end
sequenceDiagram
autonumber
participant Component
participant WF as WorkflowExecutorInterface
participant DAG as DAGExecutorInterface
Component->>WF: access dag
WF-->>Component: DAGExecutorInterface (abstract property)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snakemake_interface_executor_plugins/executors/real.py (1)
63-69: f-string bug: placeholder not interpolated in log message.The last line is a plain string; {self.workflow.persistence.path} will be printed literally.
self.logger.info( f"Failed to set marker file for job started ({e}). " "Snakemake will work, but cannot ensure that output files " "are complete in case of a kill signal or power loss. " "Please ensure write permissions for the " - "directory {self.workflow.persistence.path}." + f"directory {self.workflow.persistence.path}." )
🧹 Nitpick comments (6)
README.md (1)
87-92: Accessing settings via self.executor_settings is correct; consider a tiny usage hint in the snippet.Add a short example to show typed access to a custom ExecutorSettings field, which helps plugin authors migrate.
class Executor(RemoteExecutor): def __post_init__(self): # access workflow self.workflow # access executor specific settings self.executor_settings + # Example: typed access to a custom setting defined above + if self.executor_settings and isinstance(self.executor_settings, ExecutorSettings): + myparam = self.executor_settings.myparamsnakemake_interface_executor_plugins/workflow.py (1)
29-32: New dag property is a breaking interface change — ensure downstream implementations are updated.Please confirm all WorkflowExecutorInterface implementations now provide dag, and surface this in release notes/migration docs.
I can draft a short migration note for plugin authors if helpful.
snakemake_interface_executor_plugins/executors/base.py (2)
25-33: Constructor now requires executor_settings — consider defaulting to None for smoother migration.Keeping it required is fine for a refactor!, but a default can ease adoption and accidental arg ordering mistakes.
- executor_settings: Optional[ExecutorSettingsBase], + executor_settings: Optional[ExecutorSettingsBase] = None,
32-33: Minor consistency: prefer using self.dag in callers.Since you cache workflow.dag on self.dag, downstream code (e.g., RealExecutor.get_job_args) can use self.dag for consistency.
snakemake_interface_executor_plugins/executors/real.py (2)
38-38: Redundant assignment to self.executor_settings.Base.init already sets this; the extra assignment is unnecessary.
- self.executor_settings = executor_settings
85-86: Use cached self.dag for consistency.Not required, but matches the new base attribute.
- unneeded_temp_files = list(self.workflow.dag.get_unneeded_temp_files(job)) + unneeded_temp_files = list(self.dag.get_unneeded_temp_files(job))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
README.md(1 hunks)snakemake_interface_executor_plugins/executors/base.py(2 hunks)snakemake_interface_executor_plugins/executors/real.py(2 hunks)snakemake_interface_executor_plugins/workflow.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.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.
Files:
snakemake_interface_executor_plugins/workflow.pysnakemake_interface_executor_plugins/executors/real.pysnakemake_interface_executor_plugins/executors/base.py
🧬 Code graph analysis (3)
snakemake_interface_executor_plugins/workflow.py (1)
snakemake_interface_executor_plugins/dag.py (1)
DAGExecutorInterface(12-20)
snakemake_interface_executor_plugins/executors/real.py (3)
snakemake_interface_executor_plugins/executors/base.py (2)
AbstractExecutor(24-121)SubmittedJobInfo(18-21)snakemake_interface_executor_plugins/logging.py (1)
LoggerExecutorInterface(9-17)snakemake_interface_executor_plugins/settings.py (2)
ExecMode(121-129)ExecutorSettingsBase(83-92)
snakemake_interface_executor_plugins/executors/base.py (2)
snakemake_interface_executor_plugins/settings.py (1)
ExecutorSettingsBase(83-92)snakemake_interface_executor_plugins/workflow.py (1)
dag(31-31)
🔇 Additional comments (3)
snakemake_interface_executor_plugins/workflow.py (1)
12-12: Import of DAGExecutorInterface looks good.No issues.
snakemake_interface_executor_plugins/executors/real.py (2)
26-37: Init changes align with base; post_init hook is a good extension point.Looks good.
84-114: Verify fallback injoin_cli_argsflattens nested lists and skips falsy entries. Confirmed the primary join filters out empty orNonevalues and raises aTypeErroron nested lists; ensure theexceptblock correctly expands any inner lists and omits falsy items.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_interface_executor_plugins/settings.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.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.
Files:
snakemake_interface_executor_plugins/settings.py
| @property | ||
| @abstractmethod | ||
| def deployment_method(self) -> Set[DeploymentMethod]: ... | ||
| def deployment_method(self) -> Set[str]: ... |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify all implementations and usages have been updated.
The change from Set[DeploymentMethod] to Set[str] is a breaking change that removes compile-time type safety. While this increases flexibility for extensibility, ensure that:
- All implementations of
DeploymentSettingsExecutorInterfacehave been updated to returnSet[str] - All code that calls
deployment_methodhas been updated to handle strings instead of enum values - Valid deployment method string values are documented or validated somewhere
Run the following script to verify implementations and usages:
🏁 Script executed:
#!/bin/bash
# Description: Find all implementations of deployment_method and usages to verify they've been updated
echo "=== Searching for implementations of deployment_method ==="
ast-grep --pattern 'def deployment_method($$$) -> $RET:
$$$'
echo -e "\n=== Searching for calls to deployment_method ==="
rg -nP --type=py -C3 '\.deployment_method\b'
echo -e "\n=== Checking for any remaining references to DeploymentMethod enum ==="
rg -nP --type=py '\bDeploymentMethod\b'Length of output: 282
Document deployment_method return type change and add migration guide.
- Return type changed from
Set[DeploymentMethod]toSet[str]; there are no remaining internal references toDeploymentMethod. - This is a breaking change for external plugins and consumers—update documentation to reflect the new string‐based identifiers and provide migration instructions.
- Consider listing or validating allowed deployment method strings to prevent typos at runtime.
🤖 Prompt for AI Agents
In snakemake_interface_executor_plugins/settings.py around line 182, the return
type for deployment_method was changed from Set[DeploymentMethod] to Set[str];
update the public documentation and changelog to note this breaking change, add
a short migration guide showing how to map previous DeploymentMethod enum values
to the new string identifiers, update any API reference and docstrings in this
module to show Set[str], and add a recommended list of allowed deployment method
strings (and optional runtime validation utility or helper function) so plugin
authors can validate inputs and avoid typos.
Summary by CodeRabbit
New Features
Refactor
Documentation
Breaking Changes