feat: refactor #7
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of the Snakemake AWS Batch executor plugin. The changes focus on improving AWS Batch job management by introducing new classes like Changes
Sequence DiagramsequenceDiagram
participant User
participant Executor
participant BatchJobBuilder
participant BatchClient
participant AWSBatch
User->>Executor: Run Snakemake job
Executor->>BatchJobBuilder: Build job definition
BatchJobBuilder->>BatchClient: Register job definition
BatchClient->>AWSBatch: Create job definition
BatchJobBuilder->>BatchClient: Submit job
BatchClient->>AWSBatch: Submit job
AWSBatch-->>BatchClient: Job submission response
BatchClient-->>BatchJobBuilder: Job details
BatchJobBuilder-->>Executor: Job submission confirmation
Executor-->>User: Job execution started
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Nitpick comments (6)
snakemake_executor_plugin_aws_batch/batch_descriptor.py (1)
49-49: Use a safer guard instead of assertion for empty queues.
Ifself.job_queueis unexpectedly empty, an assertion failure may prematurely terminate the program. Consider raising an exception or logging a warning to handle this edge case more gracefully.- assert self.job_queue + if not self.job_queue: + self.lock.release() + raise RuntimeError("No jobs in queue. Unable to update descriptions.")snakemake_executor_plugin_aws_batch/batch_job_builder.py (3)
3-3: Remove unused import.
shlexis not referenced in this file. Removing it prevents confusion and aligns with the static analysis suggestion.-import shlex🧰 Tools
🪛 Ruff (0.8.2)
3-3:
shleximported but unusedRemove unused import:
shlex(F401)
73-75: Preserve traceback details when re-raising exceptions.
Raising from the original exception maintains contextual information for debugging.except Exception as e: - raise WorkflowError(e) + raise WorkflowError(e) from e🧰 Tools
🪛 Ruff (0.8.2)
74-74: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
95-96: Apply the same best practice for re-raising exceptions.
Useraise ... from ehere as well.except Exception as e: - raise WorkflowError(e) + raise WorkflowError(e) from e🧰 Tools
🪛 Ruff (0.8.2)
96-96: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
snakemake_executor_plugin_aws_batch/batch_client.py (2)
3-14: Optional credential handling for uniform usage.
At present, this code relies on the default AWS credential resolution. Consider adding a note in the docstring about how the AWS credentials are acquired (e.g., from a default AWS profile or environment variables) so that users are aware of the credential chain mechanism. This helps avoid confusion when credentials are missing or misconfigured.
15-25: Handle client initialization exceptions for robustness.
boto3.client()can raise exceptions if the service doesn’t exist, the region name is invalid, or credentials are not found. It may be beneficial to wrap this call in a try-except block for more robust error handling or logging, especially for production usage.Would you like help with adding exception handling to capture and log Boto3 client initialization errors?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (5)
snakemake_executor_plugin_aws_batch/__init__.py(6 hunks)snakemake_executor_plugin_aws_batch/batch_client.py(1 hunks)snakemake_executor_plugin_aws_batch/batch_descriptor.py(1 hunks)snakemake_executor_plugin_aws_batch/batch_job_builder.py(1 hunks)tests/__init__.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/init.py
🧰 Additional context used
📓 Path-based instructions (4)
snakemake_executor_plugin_aws_batch/batch_client.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/batch_descriptor.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🪛 Ruff (0.8.2)
snakemake_executor_plugin_aws_batch/batch_job_builder.py
3-3: shlex imported but unused
Remove unused import: shlex
(F401)
74-74: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
96-96: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (12)
snakemake_executor_plugin_aws_batch/batch_descriptor.py (2)
6-11: Robust concurrency structure.
The approach of using a class-level lock and storing job descriptions in a dictionary helps maintain thread safety. No immediate issues found.
69-71: Raise a controlled exception rather than asserting incomplete results.
The assertion here may abruptly crash the process. Consider logging a warning and raising an exception with relevant details instead, so users can diagnose partial responses from AWS.- assert ( - not job_ids - ), "AWS Batch DescribeJobs didn't return all expected results" + if job_ids: + raise RuntimeError( + "AWS Batch DescribeJobs didn't return all expected results for " + f"job IDs: {job_ids}" + )✅ Verification successful
Replace assertion with RuntimeError for better error handling and debugging
The assertion is checking a critical condition where AWS Batch API fails to return all requested job descriptions. Converting to RuntimeError with detailed job IDs will help users diagnose API issues while maintaining the fail-fast behavior for this critical error condition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Get the full context of the file cat snakemake_executor_plugin_aws_batch/batch_descriptor.py # Find similar assertion patterns and error handling rg "assert" --type py -C 3 rg "RuntimeError" --type py -C 3Length of output: 5078
snakemake_executor_plugin_aws_batch/__init__.py (8)
3-3: Confirm whether shlex is used.
shlexis imported here and appears utilized later at line 129. No immediate issues found as it is indeed used.
5-5: Dependency imports look coherent.
Imports fromBatchJobDescriber,BatchClient, andBatchJobBuilderalign with the new architecture.
105-106: Container image retrieval.
Fetching the container_image fromremote_execution_settingslooks correct. Verify thatremote_execution_settings.container_imageis always non-empty.
129-130: Effective command quoting.
Usingshlex.quotehelps protect against shell injection. No issues found.
133-138: Clean job builder initialization.
InstantiatingBatchJobBuildercentralizes job definition logic. This fosters modular, maintainable code.
142-142: Concise debug logging.
Good job logging essential submission details for troubleshooting.
302-302: Graceful job definition cleanup.
Invoking_deregister_jobfor each active job upon shutdown ensures a clean environment. No concerns here.
151-151:⚠️ Potential issueUndefined variables for job info.
References tojob_paramsandjob_defare not declared in this scope. This leads to potential NameErrors. Ensure they are defined or passed down fromBatchJobBuilder.self.report_job_submission( SubmittedJobInfo( job=job, - external_jobid=job_info["jobId"], - aux={ - "jobs_params": job_params, - "job_def_arn": job_def["jobDefinitionArn"], - }, + external_jobid=job_info["jobId"], + aux={ + "job_params": <some_reference>, + "job_def_arn": job_info.get("jobDefinitionArn"), + }, ) )Likely invalid or redundant comment.
snakemake_executor_plugin_aws_batch/batch_client.py (2)
27-34: Clean inheritance and initialization sequence.
TheBatchClientclass cleanly reusesAWSClientinitialization, limiting repeated code. This structure is straightforward and maintains clarity, aligning well with the design principle of reusability.
1-2: Verify Boto3's availability and compatibility.
Ensure that boto3 is appropriately specified as a dependency (e.g., insetup.py) and that the environment has the correct version installed for compatibility with AWS features used in this code.✅ Verification successful
✓ Boto3 dependency is properly specified
The codebase correctly declares boto3 (version ^1.36.5) as a dependency, which ensures compatibility with AWS features used in the code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm 'boto3' is declared in the project's setup or requirements files fd -i -H "setup.py" --exec cat {} | rg "boto3" fd -i -H "requirements.txt" --exec cat {} | rg "boto3"Length of output: 101
Script:
#!/bin/bash # Search for boto3 in all possible Python dependency files and documentation echo "=== Searching dependency files ===" fd -e toml -e cfg -e txt -H "^(py)?project.toml$|setup.cfg$|requirements" --exec cat {} | rg -i "boto3" echo -e "\n=== Checking documentation ===" fd -e md -e rst -H "^README|^INSTALL|^REQUIREMENTS" --exec cat {} | rg -i "boto3" echo -e "\n=== Checking plugin metadata ===" fd -e py -H "^setup.py$|^__init__.py$" --exec cat {} | rg -i "boto3|install_requires|requires"Length of output: 543
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (2)
1-9: Remove unused import.The
shlexmodule is imported but never used in this file.-import shlex🧰 Tools
🪛 Ruff (0.8.2)
3-3:
shleximported but unusedRemove unused import:
shlex(F401)
60-61: Make timeout and tags initialization more explicit.The empty timeout dictionary and tags conversion could be more explicit about their purpose.
- timeout = dict() - tags = self.settings.tags if isinstance(self.settings.tags, dict) else dict() + # Initialize empty timeout as no timeout constraints are set + timeout = {} + # Convert tags to dictionary or use empty dict if None/invalid + tags = self.settings.tags if isinstance(self.settings.tags, dict) else {}snakemake_executor_plugin_aws_batch/__init__.py (1)
133-138: Fix indentation in BatchJobBuilder initialization.The parameters are over-indented.
- job_definition = BatchJobBuilder(logger=self.logger, - job=job, - container_image=self.container_image, - settings=self.settings, - job_command=remote_command, - batch_client=self.batch_client) + job_definition = BatchJobBuilder( + logger=self.logger, + job=job, + container_image=self.container_image, + settings=self.settings, + job_command=remote_command, + batch_client=self.batch_client + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_aws_batch/__init__.py(6 hunks)snakemake_executor_plugin_aws_batch/batch_job_builder.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🪛 Ruff (0.8.2)
snakemake_executor_plugin_aws_batch/batch_job_builder.py
3-3: shlex imported but unused
Remove unused import: shlex
(F401)
74-74: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
96-96: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (3)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (2)
12-14: LGTM!The enum is well-defined with clear values for job definition types.
48-58: Consider validating resource requirements.The container properties include GPU, VCPU, and memory requirements, but there's no validation of these values. Consider adding validation to ensure they are within acceptable ranges.
snakemake_executor_plugin_aws_batch/__init__.py (1)
129-130: LGTM!The command formatting using shlex.quote ensures proper shell escaping.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Line range hint
147-162: Fix inconsistent variable usage in run_job method.The method uses inconsistent variables:
job_info["job_params"]is used butjob_paramsis not set in the responsejob_definition["jobDefinitionArn"]is used butjob_definitionis not set- job_info = job_definition.submit() + job_info = job_definition.submit_job() self.report_job_submission( SubmittedJobInfo( job=job, external_jobid=job_info["jobId"], aux={ - "jobs_params": job_info["job_params"], - "job_def_arn": job_definition["jobDefinitionArn"], + "job_params": job_info["job_params"], + "job_def_arn": job_info["job_def"]["jobDefinitionArn"], }, ) )
🧹 Nitpick comments (4)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
29-45: Add type hints for clarity.Consider adding type hints to improve code clarity:
job_commandparametercreated_job_defslistdef __init__( self, logger, job: JobExecutorInterface, container_image: str, settings: ExecutorSettings, - job_command: str, + job_command: str, batch_client: BatchClient, ): self.logger = logger self.job = job self.container_image = container_image self.settings = settings self.job_command = job_command self.batch_client = batch_client - self.created_job_defs = [] + self.created_job_defs: List[dict] = []snakemake_executor_plugin_aws_batch/__init__.py (3)
2-2: Update copyright year.The copyright year is set to 2025, which is incorrect.
-__copyright__ = "Copyright 2025, Snakemake community" +__copyright__ = "Copyright 2024, Snakemake community"
12-12: Remove trailing whitespace.There is trailing whitespace at the end of this line.
-from snakemake_executor_plugin_aws_batch.batch_job_builder import BatchJobBuilder +from snakemake_executor_plugin_aws_batch.batch_job_builder import BatchJobBuilder🧰 Tools
🪛 GitHub Actions: CI
[warning] 12-12: Trailing whitespace detected at column 82
310-310: Add newline at end of file.Add a newline at the end of the file to follow Python coding standards.
self._deregister_job(j) +🧰 Tools
🪛 GitHub Actions: CI
[warning] 310-310: No newline at end of file at column 36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (6)
.vscode/settings.json(1 hunks)snakemake_executor_plugin_aws_batch/__init__.py(8 hunks)snakemake_executor_plugin_aws_batch/batch_client.py(1 hunks)snakemake_executor_plugin_aws_batch/batch_descriptor.py(1 hunks)snakemake_executor_plugin_aws_batch/batch_job_builder.py(1 hunks)tests/__init__.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🚧 Files skipped from review as they are similar to previous changes (3)
- snakemake_executor_plugin_aws_batch/batch_client.py
- tests/init.py
- snakemake_executor_plugin_aws_batch/batch_descriptor.py
🧰 Additional context used
📓 Path-based instructions (2)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🪛 GitHub Actions: CI
snakemake_executor_plugin_aws_batch/__init__.py
[warning] 12-12: Trailing whitespace detected at column 82
[warning] 310-310: No newline at end of file at column 36
snakemake_executor_plugin_aws_batch/batch_job_builder.py
[error] 7-7: Circular import detected: Cannot import 'ExecutorSettings' from 'snakemake_executor_plugin_aws_batch' due to circular dependency
🪛 Ruff (0.8.2)
snakemake_executor_plugin_aws_batch/batch_job_builder.py
85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
107-107: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (6)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (5)
13-15: LGTM!The enum is well-defined and follows Python naming conventions.
18-20: LGTM!The enum is well-defined and follows Python naming conventions.
23-26: LGTM!The enum is well-defined and follows Python naming conventions.
87-107: 🛠️ Refactor suggestionImprove error handling and add return type hint.
The method needs better error handling and type hints for clarity.
- def submit_job(self): + def submit_job(self) -> dict: job_def, job_name = self.build_job_definition() # ... rest of the method ... except Exception as e: - raise WorkflowError(e) + raise WorkflowError(f"Failed to submit job: {e}") from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
107-107: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
47-85: 🛠️ Refactor suggestionImprove error handling and add return type hint.
The method needs better error handling and type hints for clarity.
- def build_job_definition(self): + def build_job_definition(self) -> Tuple[dict, str]: job_uuid = str(uuid.uuid4()) job_name = f"snakejob-{self.job.name}-{job_uuid}" job_definition_name = f"snakejob-def-{self.job.name}-{job_uuid}" # ... rest of the method ... except Exception as e: - raise WorkflowError(e) + raise WorkflowError(f"Failed to register job definition: {e}") from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
85-85: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Line range hint
36-80: LGTM!The settings class is well-structured with appropriate metadata and help text for each field.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
snakemake_executor_plugin_aws_batch/__init__.py (2)
Line range hint
41-47: Fix region type annotation.The region field is typed as
Optional[int]but should beOptional[str]since AWS regions are strings (e.g., "us-west-2").- region: Optional[int] = field( + region: Optional[str] = field( default=None, metadata={ "help": "AWS Region", "env_var": False, "required": True, }, )
Line range hint
275-290: Improve job cleanup error handling.The job cleanup methods should be more robust in handling missing keys in the aux dictionary.
def _deregister_job(self, job: SubmittedJobInfo): """deregister batch job definition""" try: - job_def_arn = job.aux["jobDefArn"] + job_def_arn = job.aux.get("jobDefArn") + if not job_def_arn: + self.logger.warning(f"No job definition ARN found for job {job.external_jobid}") + return self.logger.debug(f"de-registering Batch job definition {job_def_arn}") self.batch_client.deregister_job_definition(jobDefinition=job_def_arn) except Exception as e: # AWS expires job definitions after 6mo # so failing to delete them isn't fatal self.logger.info( - "failed to deregister Batch job definition " - f"{job_def_arn} with error {e}" + f"Failed to deregister Batch job definition {job_def_arn}: {str(e)}" )
🧹 Nitpick comments (3)
snakemake_executor_plugin_aws_batch/__init__.py (3)
2-2: Update copyright year.The copyright year is set to 2025, which is a future date. Consider updating it to the current year (2024).
-__copyright__ = "Copyright 2025, Snakemake community" +__copyright__ = "Copyright 2024, Snakemake community"
123-123: Add region validation.Consider validating the AWS region before initializing the BatchClient to provide a better error message if an invalid region is provided.
- self.batch_client = BatchClient(region_name=self.settings.region) + try: + self.batch_client = BatchClient(region_name=self.settings.region) + except Exception as e: + raise WorkflowError(f"Invalid AWS region {self.settings.region}: {str(e)}")
Line range hint
219-222: Improve error logging for unknown job status.The log message for unknown job status has a formatting error and could provide more helpful information.
if job_status not in known_status_list: - self.logger.info(f"unknown job status {job_status} from AWS Batch") - self.logger.debug("log details: {log_details} with status: {job_status}") + self.logger.warning(f"Unknown job status '{job_status}' from AWS Batch") + self.logger.debug(f"Log details: {log_details}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_aws_batch/__init__.py(6 hunks)snakemake_executor_plugin_aws_batch/batch_job_builder.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snakemake_executor_plugin_aws_batch/batch_job_builder.py
🧰 Additional context used
📓 Path-based instructions (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: formatting
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
snakemake_executor_plugin_aws_batch/__init__.py (2)
Line range hint
32-40: Fix incorrect type for region field.The region field type should be
Optional[str]as AWS regions are strings (e.g., "us-west-2").Apply this diff to fix the type:
- region: Optional[int] = field( + region: Optional[str] = field( default=None, metadata={ "help": "AWS Region", "env_var": False, "required": True, }, )
Line range hint
154-165: Improve error handling and job info access.The error handling is too generic and the job info access is unsafe.
Apply this diff to improve the implementation:
except Exception as e: - raise WorkflowError(e) + raise WorkflowError(f"Failed to submit job: {e}") from e self.report_job_submission( SubmittedJobInfo( job=job, external_jobid=job_info["jobId"], aux={ - "jobs_params": job_info["job_params"], - "job_def_arn": job_info["jobDefinitionArn"], + "jobs_params": job_info.get("job_params", {}), + "job_def_arn": job_info.get("jobDefinitionArn"), }, ) )
♻️ Duplicate comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
203-203:⚠️ Potential issueFix incorrect return type annotation.
The return type annotation doesn't match the actual return values. The function returns a tuple of (Optional[int], Optional[str]).
Apply this diff to fix the type:
- def _get_job_status(self, job: SubmittedJobInfo) -> Union[int, Optional[str]]: + def _get_job_status(self, job: SubmittedJobInfo) -> tuple[Optional[int], Optional[str]]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snakemake_executor_plugin_aws_batch/__init__.py(6 hunks)snakemake_executor_plugin_aws_batch/batch_client.py(1 hunks)snakemake_executor_plugin_aws_batch/batch_job_builder.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snakemake_executor_plugin_aws_batch/batch_job_builder.py
🧰 Additional context used
📓 Path-based instructions (2)
snakemake_executor_plugin_aws_batch/batch_client.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
28-45: Add type hints to improve code maintainability.Consider adding type hints to the constructor parameters for better code maintainability and IDE support.
def __init__( self, - logger, - job: JobExecutorInterface, - container_image: str, - settings, - job_command: str, - batch_client: BatchClient, + logger: logging.Logger, + job: JobExecutorInterface, + container_image: str, + settings: ExecutorSettings, + job_command: str, + batch_client: BatchClient, ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_aws_batch/__init__.py(7 hunks)snakemake_executor_plugin_aws_batch/batch_job_builder.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snakemake_executor_plugin_aws_batch/init.py
🧰 Additional context used
📓 Path-based instructions (1)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🔇 Additional comments (5)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (5)
1-10: LGTM!The imports are well-organized and the constant is appropriately defined.
12-26: LGTM!The enums correctly define AWS Batch job properties and are used consistently throughout the code.
87-88: Improve error handling by preserving exception context.When re-raising exceptions, preserve the original traceback using
raise ... from esyntax.
109-110: Improve error handling by preserving exception context.When re-raising exceptions, preserve the original traceback using
raise ... from esyntax.
93-99: Verify job queue existence before submission.Consider validating that the job queue exists before attempting job submission to provide better error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
2-2: Update copyright year.The copyright year 2025 is incorrect as it's beyond the current date.
-__copyright__ = "Copyright 2025, Snakemake community" +__copyright__ = "Copyright 2024, Snakemake community"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_aws_batch/__init__.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🔇 Additional comments (7)
snakemake_executor_plugin_aws_batch/__init__.py (7)
Line range hint
49-74: LGTM! Well-structured settings with clear documentation.The changes to the settings class are well-thought-out:
- Renamed
execution_roletojob_rolefor better clarity- Clear help text explaining the purpose of each setting
- Appropriate default timeout value that meets AWS Batch minimum requirements
112-122: LGTM! Clean initialization with good debugging support.The initialization logic is well-structured with:
- Clear separation of concerns
- Helpful debug logging
- Proper tracking of job definitions
135-136: LGTM! Secure command formatting.Proper use of
shlex.quotefor command formatting prevents command injection vulnerabilities.
139-147: LGTM! Well-structured job creation.The use of BatchJobBuilder provides a clean abstraction for job creation with all necessary parameters.
Line range hint
168-202: LGTM! Comprehensive job status handling.The status checking implementation is robust with:
- Clear handling of all AWS Batch job states
- Informative error messages
- Proper async implementation
Line range hint
271-290: LGTM! Thorough cleanup implementation.The job termination and cleanup logic is well-implemented with:
- Proper resource cleanup
- Graceful handling of termination failures
- Helpful debug logging
159-163:⚠️ Potential issueAdd safe dictionary access for job info.
The current code assumes keys will always exist in the job_info dictionary, which could lead to KeyError exceptions.
external_jobid=job_info["jobId"], aux={ - "jobs_params": job_info["job_params"], - "job_def_arn": job_info["jobDefinitionArn"], + "jobs_params": job_info.get("job_params", {}), + "job_def_arn": job_info.get("jobDefinitionArn"), },Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
51-55:⚠️ Potential issueAdd resource validation to prevent invalid configurations.
The resource extraction lacks validation for negative or invalid values.
- gpu = str(self.job.resources.get("_gpus", str(0))) - vcpu = str(self.job.resources.get("_cores", str(1))) - mem = str(self.job.resources.get("mem_mb", str(2048))) + gpu = max(0, int(self.job.resources.get("_gpus", 0))) + vcpu = max(1, int(self.job.resources.get("_cores", 1))) + mem = max(1, int(self.job.resources.get("mem_mb", 2048)))
🧹 Nitpick comments (2)
terraform/main.tf (1)
61-84: Consider adding tags to the compute environment.Adding tags to the compute environment would improve resource tracking and cost allocation.
resource "aws_batch_compute_environment" "sample" { compute_environment_name = var.aws_batch_compute_environment_name + tags = var.tags compute_resources { # ... existing configuration ... } }terraform/README.md (1)
7-11: Improve code block formatting and documentation.The code block should specify the shell language and include more detailed instructions.
-``` +```shell terraform init terraform plan terraform apply
+## Prerequisites
+
+- AWS credentials configured
+- Terraform installed
+- Required AWS permissions
+
+## Variables
+
+Update the following variables invars.tf:
+
+-aws_provider_region: Your target AWS region
+-aws_batch_subnet_ids: Your subnet IDs
+-aws_batch_security_group_ids: Your security group IDs<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 7-7: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e1b1bb0ce968bbace5129654b15caa7a56e083ce and bed0327578a98284850ff6b585e2398f1bfe4b66. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `.gitignore` (1 hunks) * `snakemake_executor_plugin_aws_batch/batch_job_builder.py` (1 hunks) * `terraform/README.md` (1 hunks) * `terraform/main.tf` (1 hunks) * `terraform/vars.tf` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)</summary> Pattern `**/*.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 the `self` argument of methods. Do not suggest type annotation of the `cls` argument of classmethods. Do not suggest return type annotation if a function or method does not contain a `return` statement. </details> </details><details> <summary>🪛 Markdownlint (0.37.0)</summary> <details> <summary>terraform/README.md</summary> 7-7: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)</summary> `59-60`: **Review privileged container configuration.** The container is running with privileged mode enabled, which grants extended privileges and could pose security risks. </details> <details> <summary>.gitignore (1)</summary> `162-165`: **LGTM! Appropriate gitignore patterns for Terraform.** The addition of Terraform-specific patterns and tracking of poetry.lock is correct and follows best practices. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
snakemake_executor_plugin_aws_batch/__init__.py (3)
2-2: Update copyright year.The copyright year is set to 2025, which is beyond the current date. Consider updating it to 2024 or the actual year of creation.
-__copyright__ = "Copyright 2025, Snakemake community" +__copyright__ = "Copyright 2024, Snakemake community"
156-160: Add defensive job_info access.The code assumes that job_info will always contain certain keys. Consider adding defensive programming to handle potential missing keys.
self.logger.debug(f"Job info: {pformat(job_info, indent=2)}") self.report_job_submission( SubmittedJobInfo( - job=job, external_jobid=job_info["jobId"], aux=dict(job_info) + job=job, + external_jobid=job_info.get("jobId"), + aux={k: v for k, v in job_info.items() if v is not None} ) )
Line range hint
266-285: Improve error handling in cleanup methods.The error handling in _terminate_job and _deregister_job could be improved:
- Log the actual exception details for better debugging
- Consider retrying on transient AWS API errors
def _terminate_job(self, job: SubmittedJobInfo): """terminate job from submitted job info""" try: self.logger.debug(f"terminating job {job.external_jobid}") self.batch_client.terminate_job( jobId=job.external_jobid, reason="terminated by snakemake", ) except Exception as e: - job_spec = job.aux["job_params"] + job_spec = job.aux.get("job_params", "unknown") self.logger.info( - f"failed to terminate Batch job definition: {job_spec} with error: {e}" + f"Failed to terminate Batch job definition: {job_spec}", + exc_info=True ) def _deregister_job(self, job: SubmittedJobInfo): """deregister batch job definition""" try: - job_def_arn = job.aux["jobDefArn"] + job_def_arn = job.aux.get("jobDefArn") + if not job_def_arn: + self.logger.warning("No job definition ARN found to deregister") + return self.logger.debug(f"de-registering Batch job definition {job_def_arn}") self.batch_client.deregister_job_definition(jobDefinition=job_def_arn) except Exception as e: # AWS expires job definitions after 6mo # so failing to delete them isn't fatal self.logger.info( "failed to deregister Batch job definition " - f"{job_def_arn} with error {e}" + f"{job_def_arn}", + exc_info=True )terraform/vars.tf (4)
1-5: Add validation for AWS region.Consider adding validation to ensure only supported AWS regions are provided.
variable "aws_provider_region" { description = "The AWS region to deploy resources in" type = string default = "us-west-2" + validation { + condition = can(regex("^[a-z]{2}-[a-z]+-[1-9]$", var.aws_provider_region)) + error_message = "The aws_provider_region value must be a valid AWS region, e.g., us-west-2" + } }
7-11: Add name prefix for IAM roles.Consider adding a name prefix to IAM roles to prevent naming conflicts in shared accounts and improve resource organization.
+variable "resource_prefix" { + description = "Prefix to be used for all resource names" + type = string + default = "snakemake" +} variable "ecs_instance_role_name" { description = "The name of the ECS instance role" type = string - default = "ecs_instance_role01" + default = null }Then update the role name references to use the prefix:
locals { ecs_instance_role_name = coalesce(var.ecs_instance_role_name, "${var.resource_prefix}-ecs-instance-role") batch_service_role_name = coalesce(var.aws_batch_service_role_name, "${var.resource_prefix}-batch-service-role") }Also applies to: 19-23
68-72: Add validation for instance types.Consider adding validation to ensure only valid EC2 instance types are provided and add cost-optimization recommendations.
variable "instance_types" { description = "The allowed instance types for the compute environment" type = list(string) default = ["c4.large", "c4.xlarge", "c4.2xlarge", "c4.4xlarge", "c4.8xlarge"] + validation { + condition = length(var.instance_types) > 0 + error_message = "At least one instance type must be specified" + } }Consider using a mix of Spot and On-Demand instances for cost optimization:
variable "use_spot_instances" { description = "Whether to use Spot instances for cost optimization" type = bool default = true }
104-108: Add validation for job queue state.Add validation to ensure only valid states are provided.
variable "aws_batch_job_queue_state" { description = "The state of the AWS Batch job queue" type = string default = "ENABLED" + validation { + condition = contains(["ENABLED", "DISABLED"], var.aws_batch_job_queue_state) + error_message = "Job queue state must be either ENABLED or DISABLED" + } }terraform/main.tf (3)
77-80: Consider multi-AZ deployment for high availability.Using a single subnet in a single AZ creates a single point of failure.
Consider creating subnets in multiple AZs:
variable "availability_zones" { description = "List of availability zones" type = list(string) } resource "aws_subnet" "batch_subnets" { count = length(var.availability_zones) vpc_id = aws_vpc.vpc01.id cidr_block = cidrsubnet(var.aws_vpc_cidr_block, 8, count.index) availability_zone = var.availability_zones[count.index] }
82-105: Consider cost optimization strategies.The compute environment could be optimized for cost and performance.
Add allocation strategy and spot fleet configuration:
resource "aws_batch_compute_environment" "sample" { compute_environment_name = var.aws_batch_compute_environment_name compute_resources { instance_role = aws_iam_instance_profile.ecs_instance_role.arn instance_type = var.instance_types max_vcpus = var.max_vcpus min_vcpus = var.min_vcpus placement_group = aws_placement_group.sample.name security_group_ids = [aws_security_group.sg01.id] subnets = [aws_subnet.subnet01.id] type = var.aws_batch_compute_resource_type + allocation_strategy = "BEST_FIT_PROGRESSIVE" + spot_iam_fleet_role = var.use_spot_instances ? aws_iam_role.spot_fleet_role.arn : null + bid_percentage = var.use_spot_instances ? 60 : null } service_role = aws_iam_role.aws_batch_service_role.arn type = var.aws_batch_compute_environment_type depends_on = [aws_iam_role_policy_attachment.aws_batch_service_role] }
108-117: Consider multiple compute environments for job queue.Adding multiple compute environments with different instance types can improve job scheduling flexibility.
resource "aws_batch_job_queue" "snakequeue" { name = var.aws_batch_job_queue_name state = var.aws_batch_job_queue_state priority = 1 - compute_environment_order { - order = 1 - compute_environment = aws_batch_compute_environment.sample.arn - } + dynamic "compute_environment_order" { + for_each = var.compute_environments + content { + order = compute_environment_order.value.order + compute_environment = compute_environment_order.value.arn + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snakemake_executor_plugin_aws_batch/__init__.py(6 hunks)terraform/main.tf(1 hunks)terraform/vars.tf(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🪛 Ruff (0.8.2)
snakemake_executor_plugin_aws_batch/__init__.py
154-154: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (3)
snakemake_executor_plugin_aws_batch/__init__.py (2)
Line range hint
69-76: Verify if 60 seconds is sufficient as a minimum timeout.While 60 seconds is mentioned as the minimum in the help text, consider if this is sufficient for most AWS Batch jobs to start up, especially considering potential container pull times and resource provisioning delays.
Line range hint
1-285: Overall code improvements look good!The refactoring has successfully improved the code's modularity and error handling. The separation of concerns between the Executor, BatchClient, BatchJobBuilder, and BatchJobDescriber classes is well done. The changes align well with the PR objectives.
🧰 Tools
🪛 Ruff (0.8.2)
154-154: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
terraform/main.tf (1)
1-54: LGTM! IAM configurations follow security best practices.The IAM role configurations are well-structured with:
- Proper assume role policies
- Use of AWS-managed policies
- Separate roles for ECS and Batch services
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
snakemake_executor_plugin_aws_batch/batch_client.py (2)
4-26: 🛠️ Refactor suggestionAdd error handling and type hints to improve robustness.
The AWS client initialization could fail for various reasons (invalid credentials, region, etc.). Consider adding proper error handling and type hints.
30-46: 🛠️ Refactor suggestionAdd error handling, type hints, and kwargs validation to BatchClient.
The
submit_jobmethod should validate required parameters and handle AWS-specific errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snakemake_executor_plugin_aws_batch/batch_client.py(1 hunks)terraform/main.tf(1 hunks)terraform/vars.tf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- terraform/vars.tf
🧰 Additional context used
📓 Path-based instructions (1)
snakemake_executor_plugin_aws_batch/batch_client.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🔇 Additional comments (4)
terraform/main.tf (4)
5-54: LGTM! Well-structured IAM configuration.The IAM roles and policies are properly configured with clear separation of concerns and follow AWS best practices.
80-108: LGTM! Well-structured networking configuration.The VPC, subnet, and routing configurations are properly set up. The comment about
map_public_ip_on_launchbeing required for AWS Batch jobs is helpful documentation.
110-133: LGTM! Well-configured AWS Batch compute environment.The compute environment is properly configured with:
- Instance role and service role
- Placement group for performance
- Security group and subnet configuration
- Proper dependency on IAM role policy attachment
136-145: LGTM! Well-configured AWS Batch job queue.The job queue is properly configured with priority and compute environment order.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Line range hint
34-41: Fix incorrect type for AWS region.The region field is typed as
Optional[int]but AWS regions are strings (e.g., "us-east-1").- region: Optional[int] = field( + region: Optional[str] = field( default=None, metadata={ "help": "AWS Region", "env_var": False, "required": True, }, )
♻️ Duplicate comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
151-152:⚠️ Potential issueImprove error handling by preserving exception context.
When re-raising exceptions, preserve the original exception context for better debugging.
except Exception as e: - raise WorkflowError(e) + raise WorkflowError(f"Failed to submit AWS Batch job: {e}") from e🧰 Tools
🪛 Ruff (0.8.2)
152-152: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🧹 Nitpick comments (3)
snakemake_executor_plugin_aws_batch/__init__.py (3)
2-2: Update copyright year.The copyright year is set to 2025, which is the current year. Consider using a range (e.g., "2023-2025") to reflect both the initial creation and current modification years.
-__copyright__ = "Copyright 2025, Snakemake community" +__copyright__ = "Copyright 2023-2025, Snakemake community"
121-121: Add region name validation.Consider validating the region name before initializing the BatchClient to provide early feedback for misconfiguration.
+ if not self.settings.region or not isinstance(self.settings.region, str): + raise WorkflowError(f"Invalid AWS region: {self.settings.region}") self.batch_client = BatchClient(region_name=self.settings.region)
Line range hint
251-259: Improve error message formatting in job failure handling.The error messages could be more informative by using f-strings and better formatting.
if ( status_reason and "Host EC2" in status_reason and "terminated" in status_reason ): raise WorkflowError( - "AWS Batch job interrupted (likely spot instance termination)" - f"with error {failure_info}" + f"AWS Batch job interrupted (likely spot instance termination). " + f"Error details: {failure_info}" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_aws_batch/__init__.py(7 hunks)snakemake_executor_plugin_aws_batch/batch_job_builder.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snakemake_executor_plugin_aws_batch/batch_job_builder.py
🧰 Additional context used
📓 Path-based instructions (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🪛 Ruff (0.8.2)
snakemake_executor_plugin_aws_batch/__init__.py
152-152: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: formatting
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Line range hint
35-38: Fix region field type.The region field is incorrectly typed as
Optional[int]. AWS regions are strings (e.g., "us-east-1").Apply this diff to fix the type:
- region: Optional[int] = field( + region: Optional[str] = field(
♻️ Duplicate comments (7)
snakemake_executor_plugin_aws_batch/batch_client.py (3)
4-26: 🛠️ Refactor suggestionAdd error handling and type hints to improve robustness.
The AWS client initialization could fail for various reasons (invalid credentials, region, etc.). Consider adding proper error handling and type hints.
30-46: 🛠️ Refactor suggestionAdd error handling, type hints, and kwargs validation to BatchClient.
The
submit_jobmethod should validate required parameters and handle AWS-specific errors.
48-55: 🛠️ Refactor suggestionAdd error handling and parameter validation to describe_jobs method.
Similar to submit_job, this method should validate input parameters and handle AWS errors.
snakemake_executor_plugin_aws_batch/batch_job_builder.py (2)
97-98: 🛠️ Refactor suggestionImprove error handling by preserving exception context.
When re-raising exceptions, preserve the original traceback using
raise ... from esyntax.
119-120: 🛠️ Refactor suggestionImprove error handling by preserving exception context.
When re-raising exceptions, preserve the original traceback using
raise ... from esyntax.snakemake_executor_plugin_aws_batch/__init__.py (2)
154-155: 🛠️ Refactor suggestionImprove error handling by preserving exception context.
When re-raising exceptions, preserve the original traceback using
raise ... from esyntax.🧰 Tools
🪛 Ruff (0.8.2)
155-155: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
158-160: 🛠️ Refactor suggestionValidate job_info dictionary before creating SubmittedJobInfo.
The direct use of
dict(job_info)as aux data could lead to issues if required keys are missing.
🧹 Nitpick comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Line range hint
67-74: Consider increasing the default task timeout.The default task timeout of 60 seconds might be too short for many batch jobs. Consider increasing it to a more reasonable default (e.g., 3600 seconds / 1 hour) to prevent premature job termination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
snakemake_executor_plugin_aws_batch/__init__.py(6 hunks)snakemake_executor_plugin_aws_batch/batch_client.py(1 hunks)snakemake_executor_plugin_aws_batch/batch_job_builder.py(1 hunks)terraform/vars.tf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- terraform/vars.tf
🧰 Additional context used
📓 Path-based instructions (3)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/batch_client.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🪛 Ruff (0.8.2)
snakemake_executor_plugin_aws_batch/__init__.py
155-155: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (5)
snakemake_executor_plugin_aws_batch/batch_client.py (1)
57-73: 🛠️ Refactor suggestionAdd error handling to job management methods.
The
deregister_job_definitionandterminate_jobmethods should include proper error handling and parameter validation.Apply this diff to improve the implementation:
- def deregister_job_definition(self, **kwargs): + def deregister_job_definition(self, **kwargs) -> dict: """ Deregister a job definition in AWS Batch. :param kwargs: The keyword arguments passed to deregister_job_definition method. + :required kwargs: + - jobDefinition: The job definition ARN to deregister :return: The response from the deregister_job_definition method. + :raises: ValueError: If required parameters are missing + :raises: ClientError: If deregistration fails """ + if not kwargs.get('jobDefinition'): + raise ValueError("Missing required parameter: jobDefinition") + try: return self.client.deregister_job_definition(**kwargs) + except self.client.exceptions.ClientError as e: + raise ValueError(f"Failed to deregister job definition: {e}") from e - def terminate_job(self, **kwargs): + def terminate_job(self, **kwargs) -> dict: """ Terminate a job in AWS Batch. :param kwargs: The keyword arguments to pass to the terminate_job method. + :required kwargs: + - jobId: The job ID to terminate + - reason: The reason for terminating the job :return: The response from the terminate_job method. + :raises: ValueError: If required parameters are missing + :raises: ClientError: If termination fails """ + required_params = {"jobId", "reason"} + missing_params = required_params - set(kwargs.keys()) + if missing_params: + raise ValueError(f"Missing required parameters: {missing_params}") + try: return self.client.terminate_job(**kwargs) + except self.client.exceptions.ClientError as e: + raise ValueError(f"Failed to terminate job: {e}") from eLikely invalid or redundant comment.
snakemake_executor_plugin_aws_batch/batch_job_builder.py (4)
14-16: LGTM!The enum correctly defines the supported AWS Batch job definition types.
19-21: LGTM!The enum correctly defines the supported AWS Batch platform capabilities.
24-27: LGTM!The enum correctly defines the supported AWS Batch resource requirement types.
59-62:⚠️ Potential issueImplement resource validation.
The TODO comment indicates missing resource validation. AWS Batch has specific constraints for resource values:
- vCPU must be at least 1
- Memory must be positive
- GPU must be non-negative
Apply this diff to add validation:
- gpu = str(self.job.resources.get("_gpus", str(0))) - vcpu = str(self.job.resources.get("_cores", str(1))) - mem = str(self.job.resources.get("mem_mb", str(2048))) + gpu = max(0, int(self.job.resources.get("_gpus", 0))) + vcpu = max(1, int(self.job.resources.get("_cores", 1))) + mem = max(1, int(self.job.resources.get("mem_mb", 2048))) + + # Convert to strings after validation + gpu = str(gpu) + vcpu = str(vcpu) + mem = str(mem)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
snakemake_executor_plugin_aws_batch/batch_client.py (2)
40-47: 🛠️ Refactor suggestionAdd parameter validation to submit_job method.
The method should validate required parameters before submission to prevent AWS API errors.
def submit_job(self, **kwargs): """ Submit a job to AWS Batch. :param kwargs: The keyword arguments to pass to the submit_job method. + :required kwargs: + - jobName: Name of the job + - jobQueue: The job queue ARN + - jobDefinition: The job definition ARN :return: The response from the submit_job method. + :raises: ValueError: If required parameters are missing + :raises: ClientError: If job submission fails """ + required_params = {"jobName", "jobQueue", "jobDefinition"} + missing_params = required_params - set(kwargs.keys()) + if missing_params: + raise ValueError(f"Missing required parameters: {missing_params}") + + try: return self.client.submit_job(**kwargs) + except self.client.exceptions.ClientError as e: + raise ValueError(f"Failed to submit job: {e}") from e
49-56: 🛠️ Refactor suggestionAdd error handling to describe_jobs method.
Similar to submit_job, this method should validate input parameters and handle AWS errors.
def describe_jobs(self, **kwargs): """ Describe jobs in AWS Batch. :param kwargs: The keyword arguments to pass to the describe_jobs method. + :required kwargs: + - jobs: List of job IDs to describe :return: The response from the describe_jobs method. + :raises: ValueError: If required parameters are missing + :raises: ClientError: If job description fails """ + if not kwargs.get('jobs'): + raise ValueError("Missing required parameter: jobs") + + try: return self.client.describe_jobs(**kwargs) + except self.client.exceptions.ClientError as e: + raise ValueError(f"Failed to describe jobs: {e}") from esnakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
60-64: 🛠️ Refactor suggestionAdd resource validation to prevent invalid configurations.
The resource extraction lacks validation for negative or invalid values. AWS Batch requires:
- vCPU must be at least 1
- Memory must be positive
- GPU must be non-negative
- gpu = str(self.job.resources.get("_gpus", str(0))) - vcpu = str(self.job.resources.get("_cores", str(1))) - mem = str(self.job.resources.get("mem_mb", str(2048))) + gpu = max(0, int(self.job.resources.get("_gpus", 0))) + vcpu = max(1, int(self.job.resources.get("_cores", 1))) + mem = max(1, int(self.job.resources.get("mem_mb", 2048)))snakemake_executor_plugin_aws_batch/__init__.py (2)
152-153: 🛠️ Refactor suggestionImprove error handling by preserving exception context.
When re-raising exceptions, preserve the original exception context for better debugging.
except Exception as e: - raise WorkflowError(e) + raise WorkflowError(f"Failed to submit AWS Batch job: {e}") from e🧰 Tools
🪛 Ruff (0.8.2)
153-153: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
156-158: 🛠️ Refactor suggestionValidate job_info dictionary before creating SubmittedJobInfo.
The direct use of
dict(job_info)as aux data could lead to issues if required keys are missing.+ required_keys = ["jobId"] + if not all(key in job_info for key in required_keys): + raise WorkflowError(f"Missing required keys in job_info: {required_keys}") self.report_job_submission( SubmittedJobInfo( - job=job, external_jobid=job_info["jobId"], aux=dict(job_info) + job=job, + external_jobid=job_info["jobId"], + aux={k: job_info[k] for k in job_info if k != "jobId"} ) )
🧹 Nitpick comments (2)
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
49-53: Improve command construction with better escaping.The current implementation might not handle all shell special characters correctly.
def _make_container_command(self, remote_command: str) -> List[str]: """ Return docker CMD form of the command + + :param remote_command: The command to be executed in the container + :return: List of command parts suitable for docker CMD + :raises: ValueError: If command parsing fails """ - return [shlex.quote(part) for part in shlex.split(remote_command)] + try: + return [part for part in shlex.split(remote_command)] + except ValueError as e: + raise ValueError(f"Failed to parse command: {e}") from esnakemake_executor_plugin_aws_batch/__init__.py (1)
213-213: Simplify dictionary get operation.The
Nonedefault is redundant asget()returnsNoneby default.- job.aux["job_definition_arn"] = job_info.get("jobDefinition", None) + job.aux["job_definition_arn"] = job_info.get("jobDefinition")🧰 Tools
🪛 Ruff (0.8.2)
213-213: Use
job_info.get("jobDefinition")instead ofjob_info.get("jobDefinition", None)Replace
job_info.get("jobDefinition", None)withjob_info.get("jobDefinition")(SIM910)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snakemake_executor_plugin_aws_batch/__init__.py(7 hunks)snakemake_executor_plugin_aws_batch/batch_client.py(1 hunks)snakemake_executor_plugin_aws_batch/batch_job_builder.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
snakemake_executor_plugin_aws_batch/batch_client.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🪛 Ruff (0.8.2)
snakemake_executor_plugin_aws_batch/batch_client.py
29-29: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
snakemake_executor_plugin_aws_batch/__init__.py
153-153: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
213-213: Use job_info.get("jobDefinition") instead of job_info.get("jobDefinition", None)
Replace job_info.get("jobDefinition", None) with job_info.get("jobDefinition")
(SIM910)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: formatting
| except Exception as e: | ||
| raise Exception(f"Failed to initialize {self.service_name} client: {e}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling by using specific exceptions.
The generic Exception catch and re-raise pattern could mask important error details. Consider catching specific boto3 exceptions.
- except Exception as e:
- raise Exception(f"Failed to initialize {self.service_name} client: {e}")
+ except (boto3.exceptions.BotoCoreError, boto3.exceptions.ClientError) as e:
+ raise ValueError(f"Failed to initialize {self.service_name} client: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| raise Exception(f"Failed to initialize {self.service_name} client: {e}") | |
| except (boto3.exceptions.BotoCoreError, boto3.exceptions.ClientError) as e: | |
| raise ValueError(f"Failed to initialize {self.service_name} client: {e}") from e |
🧰 Tools
🪛 Ruff (0.8.2)
29-29: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
snakemake_executor_plugin_aws_batch/batch_client.py (1)
49-56: 🛠️ Refactor suggestionAdd error handling to describe_jobs method.
Similar to submit_job, this method should validate input parameters and handle AWS errors.
- def describe_jobs(self, **kwargs): + def describe_jobs(self, **kwargs) -> dict: """ Describe jobs in AWS Batch. :param kwargs: The keyword arguments to pass to the describe_jobs method. + :required kwargs: + - jobs: List of job IDs to describe :return: The response from the describe_jobs method. + :raises: ValueError: If required parameters are missing + :raises: ClientError: If job description fails """ + if not kwargs.get('jobs'): + raise ValueError("Missing required parameter: jobs") + + try: return self.client.describe_jobs(**kwargs) + except self.client.exceptions.ClientError as e: + raise ValueError(f"Failed to describe jobs: {e}") from e
🧹 Nitpick comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
249-252: Improve job cleanup error handling.The job cleanup could be more robust by checking the job definition ARN format.
- job_def_arn = job.aux.get("job_definition_arn") - if job_def_arn is not None: + job_def_arn = job.aux.get("job_definition_arn", "") + if job_def_arn and ":" in job_def_arn: # Basic ARN format validation self.logger.debug(f"de-registering Batch job definition {job_def_arn}") self.batch_client.deregister_job_definition(jobDefinition=job_def_arn) + else: + self.logger.warning(f"Invalid job definition ARN format: {job_def_arn}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snakemake_executor_plugin_aws_batch/__init__.py(7 hunks)snakemake_executor_plugin_aws_batch/batch_client.py(1 hunks)snakemake_executor_plugin_aws_batch/batch_job_builder.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
snakemake_executor_plugin_aws_batch/batch_client.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/batch_job_builder.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🪛 Ruff (0.8.2)
snakemake_executor_plugin_aws_batch/batch_client.py
29-29: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
snakemake_executor_plugin_aws_batch/__init__.py
150-150: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
212-212: Use job_info.get("jobDefinition") instead of job_info.get("jobDefinition", None)
Replace job_info.get("jobDefinition", None) with job_info.get("jobDefinition")
(SIM910)
🔇 Additional comments (7)
snakemake_executor_plugin_aws_batch/batch_client.py (2)
28-29:⚠️ Potential issueImprove exception handling.
Replace generic exception handling with specific boto3 exceptions and preserve the exception context.
- except Exception as e: - raise Exception(f"Failed to initialize {self.service_name} client: {e}") + except (boto3.exceptions.BotoCoreError, boto3.exceptions.ClientError) as e: + raise ValueError(f"Failed to initialize {self.service_name} client: {e}") from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
29-29: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
40-47: 🛠️ Refactor suggestionAdd parameter validation to submit_job method.
The method should validate required parameters before submission to prevent AWS API errors.
def submit_job(self, **kwargs): """ Submit a job to AWS Batch. :param kwargs: The keyword arguments to pass to the submit_job method. + :required kwargs: + - jobName: Name of the job + - jobQueue: The job queue ARN + - jobDefinition: The job definition ARN :return: The response from the submit_job method. + :raises: ValueError: If required parameters are missing + :raises: ClientError: If job submission fails """ + required_params = {"jobName", "jobQueue", "jobDefinition"} + missing_params = required_params - set(kwargs.keys()) + if missing_params: + raise ValueError(f"Missing required parameters: {missing_params}") + + try: return self.client.submit_job(**kwargs) + except self.client.exceptions.ClientError as e: + raise ValueError(f"Failed to submit job: {e}") from eLikely invalid or redundant comment.
snakemake_executor_plugin_aws_batch/batch_job_builder.py (3)
98-99: 🛠️ Refactor suggestionImprove error handling in build_job_definition.
The exception handling should preserve the context and provide more specific error information.
- except Exception as e: - raise WorkflowError(f"Failed to register job definition: {e}") from e + except self.batch_client.client.exceptions.ClientError as e: + raise WorkflowError( + f"Failed to register job definition {job_definition_name}: {e}" + ) from eLikely invalid or redundant comment.
60-64:⚠️ Potential issueImplement resource validation.
The TODO comment indicates missing resource validation. AWS Batch has specific constraints for resource values.
- # TODO: validate resources - gpu = str(self.job.resources.get("_gpus", str(0))) - vcpu = str(self.job.resources.get("_cores", str(1))) - mem = str(self.job.resources.get("mem_mb", str(2048))) + def validate_resource(value: int, min_value: int, name: str) -> str: + if value < min_value: + raise ValueError(f"{name} must be at least {min_value}") + return str(value) + + gpu = validate_resource( + int(self.job.resources.get("_gpus", 0)), 0, "GPU count" + ) + vcpu = validate_resource( + int(self.job.resources.get("_cores", 1)), 1, "vCPU count" + ) + mem = validate_resource( + int(self.job.resources.get("mem_mb", 2048)), 1, "Memory" + )Likely invalid or redundant comment.
115-116: 🛠️ Refactor suggestionImprove error handling in submit method.
Similar to build_job_definition, the exception handling should be more specific.
- except Exception as e: - raise WorkflowError(f"Failed to submit job: {e}") from e + except self.batch_client.client.exceptions.ClientError as e: + raise WorkflowError( + f"Failed to submit job {job_name} to queue {self.settings.job_queue}: {e}" + ) from eLikely invalid or redundant comment.
snakemake_executor_plugin_aws_batch/__init__.py (2)
149-150:⚠️ Potential issueImprove error handling in run_job method.
The exception handling should preserve the context and provide more specific error information.
except Exception as e: - raise WorkflowError(e) + raise WorkflowError(f"Failed to submit AWS Batch job: {e}") from eLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
150-150: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
153-155: 🛠️ Refactor suggestionValidate job_info dictionary before creating SubmittedJobInfo.
Direct use of dict(job_info) as aux data could lead to issues if required keys are missing.
+ required_keys = ["jobId", "jobName"] + if not all(key in job_info for key in required_keys): + raise WorkflowError(f"Missing required keys in job_info: {required_keys}") self.report_job_submission( SubmittedJobInfo( - job=job, external_jobid=job_info["jobId"], aux=dict(job_info) + job=job, + external_jobid=job_info["jobId"], + aux={k: job_info[k] for k in job_info if k != "jobId"} ) )Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Add validation for task_timeout to prevent AWS Batch API errors
The suggested validation is necessary as the current implementation doesn't enforce AWS Batch's minimum timeout requirement of 60 seconds. The proposed solution using a field validator is appropriate and will catch invalid values early before they reach the AWS Batch API.
🔗 Analysis chain
Line range hint
67-73: Add validation for task_timeout.The default task_timeout of 300 seconds might be too low for some batch jobs, and AWS Batch requires a minimum of 60 seconds. Consider adding validation to ensure the timeout is at least 60 seconds.
task_timeout: Optional[int] = field( default=300, metadata={ "help": ( "Task timeout (seconds) will force AWS Batch to terminate " "a Batch task if it fails to finish within the timeout, minimum 60" - ) + ), + "validator": lambda x: x >= 60 if x is not None else True, }, )🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for existing validation related to task_timeout rg "task_timeout.*validator" -A 5 -B 5 # Look for any field validator patterns in the codebase rg "validator.*lambda.*x.*>=" -A 2 -B 2 # Check if there are any validation utilities fd validatorLength of output: 97
Script:
#!/bin/bash # Check the imports and field usage rg "from dataclasses import" -A 5 rg "field\(" -A 2 -B 2 # Look for timeout-related code rg "timeout" -A 2 -B 2 # Check the full content of the file cat snakemake_executor_plugin_aws_batch/__init__.pyLength of output: 15185
🧹 Nitpick comments (6)
.github/workflows/ci_true_api.yml (1)
34-37: Consider adding secret existence checks.While using GitHub secrets for AWS configuration is secure, the workflow might fail silently if secrets are missing. Consider adding checks to ensure required secrets exist.
- name: Run pytest env: SNAKEMAKE_AWS_BATCH_REGION: "${{ secrets.SNAKEMAKE_AWS_BATCH_REGION }}" SNAKEMAKE_AWS_BATCH_JOB_QUEUE: "${{ secrets.SNAKEMAKE_AWS_BATCH_JOB_QUEUE }}" SNAKEMAKE_AWS_BATCH_JOB_ROLE: "${{ secrets.SNAKEMAKE_AWS_BATCH_JOB_ROLE }}" + if: ${{ secrets.SNAKEMAKE_AWS_BATCH_REGION != '' && secrets.SNAKEMAKE_AWS_BATCH_JOB_QUEUE != '' && secrets.SNAKEMAKE_AWS_BATCH_JOB_ROLE != '' }} run: poetry run pytest tests/tests_true_api.py -v + else: + run: | + echo "Error: Required AWS configuration secrets are missing" + exit 1.github/workflows/ci_mocked_api.yml (2)
91-94: Reconsider using real AWS credentials in mocked tests.Since this is the mocked API test workflow, it shouldn't require real AWS credentials. Consider using mock values for these environment variables instead.
- name: Run pytest env: - SNAKEMAKE_AWS_BATCH_REGION: "${{ secrets.SNAKEMAKE_AWS_BATCH_REGION }}" - SNAKEMAKE_AWS_BATCH_JOB_QUEUE: "${{ secrets.SNAKEMAKE_AWS_BATCH_JOB_QUEUE }}" - SNAKEMAKE_AWS_BATCH_JOB_ROLE: "${{ secrets.SNAKEMAKE_AWS_BATCH_JOB_ROLE }}" + SNAKEMAKE_AWS_BATCH_REGION: "mock-region" + SNAKEMAKE_AWS_BATCH_JOB_QUEUE: "mock-queue" + SNAKEMAKE_AWS_BATCH_JOB_ROLE: "mock-role"🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 92-92: wrong indentation: expected 10 but found 12
(indentation)
96-97: Ensure coverage report runs even if tests fail.The coverage report step should run regardless of test failures to provide insights into test coverage.
- name: Run Coverage + if: always() run: poetry run coverage report -msnakemake_executor_plugin_aws_batch/__init__.py (3)
2-2: Update copyright year.The copyright year is set to 2025, which is beyond the current date (January 2025).
-__copyright__ = "Copyright 2025, Snakemake community" +__copyright__ = "Copyright 2024, Snakemake community"
194-230: Enhance error messages in _get_job_status.The error messages could be more descriptive to help with debugging.
if job_status == "SUCCEEDED": return 0, None elif job_status == "FAILED": - reason = job_info.get("statusReason", "Unknown reason") + container_info = job_info.get("container", {}) + reason = ( + f"Status: {job_info.get('statusReason', 'Unknown')}, " + f"Container reason: {container_info.get('reason', 'Unknown')}, " + f"Exit code: {container_info.get('exitCode', 'Unknown')}" + ) return exit_code or 1, reason🧰 Tools
🪛 Ruff (0.8.2)
212-212: Use
job_info.get("jobDefinition")instead ofjob_info.get("jobDefinition", None)Replace
job_info.get("jobDefinition", None)withjob_info.get("jobDefinition")(SIM910)
212-212: Remove redundant None default in get() method.The
Nonedefault inget()is redundant as it's already the default behavior.- job.aux["job_definition_arn"] = job_info.get("jobDefinition", None) + job.aux["job_definition_arn"] = job_info.get("jobDefinition")🧰 Tools
🪛 Ruff (0.8.2)
212-212: Use
job_info.get("jobDefinition")instead ofjob_info.get("jobDefinition", None)Replace
job_info.get("jobDefinition", None)withjob_info.get("jobDefinition")(SIM910)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci_mocked_api.yml(1 hunks).github/workflows/ci_true_api.yml(1 hunks)snakemake_executor_plugin_aws_batch/__init__.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🪛 YAMLlint (1.35.1)
.github/workflows/ci_mocked_api.yml
[warning] 92-92: wrong indentation: expected 10 but found 12
(indentation)
🪛 Ruff (0.8.2)
snakemake_executor_plugin_aws_batch/__init__.py
212-212: Use job_info.get("jobDefinition") instead of job_info.get("jobDefinition", None)
Replace job_info.get("jobDefinition", None) with job_info.get("jobDefinition")
(SIM910)
🔇 Additional comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
142-156: 🛠️ Refactor suggestionImprove job_info dictionary access safety.
The job_info dictionary access is not safe and could raise KeyError if required keys are missing.
- job_info = job_definition.submit() + job_info = job_definition.submit() + required_keys = ["jobId", "jobName"] + missing_keys = [key for key in required_keys if key not in job_info] + if missing_keys: + raise WorkflowError(f"Missing required keys in job_info: {missing_keys}") log_info = { - "job_name:": job_info["jobName"], - "jobId": job_info["jobId"], + "job_name": job_info.get("jobName"), + "jobId": job_info.get("jobId"), "job_queue": self.settings.job_queue, } self.logger.debug(f"AWS Batch job submitted: {log_info}") except Exception as e: raise WorkflowError(f"Failed to submit AWS Batch job: {e}") from e self.report_job_submission( SubmittedJobInfo( - job=job, external_jobid=job_info["jobId"], aux=dict(job_info) + job=job, + external_jobid=job_info["jobId"], + aux={k: v for k, v in job_info.items() if k != "jobId"} ) )Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
snakemake_executor_plugin_aws_batch/__init__.py (1)
155-159: 🛠️ Refactor suggestionValidate job_info dictionary before creating SubmittedJobInfo.
The direct use of
dict(job_info)as aux data could lead to issues if required keys are missing.+ required_keys = ["jobId"] + if not all(key in job_info for key in required_keys): + raise WorkflowError(f"Missing required keys in job_info: {required_keys}") self.report_job_submission( SubmittedJobInfo( - job=job, external_jobid=job_info["jobId"], aux=dict(job_info) + job=job, + external_jobid=job_info["jobId"], + aux={k: job_info[k] for k in job_info if k != "jobId"} ) )
🧹 Nitpick comments (7)
tests/__init__.py (2)
35-49: Improve error handling in cleanup_test method.The current try-except-pass pattern can be replaced with
contextlib.suppressfor better readability.+from contextlib import suppress + def cleanup_test(self): import boto3 # clean up using boto3 s3c = boto3.resource( "s3", endpoint_url=self.endpoint_url, aws_access_key_id=self.access_key, aws_secret_access_key=self.secret_key, ) - try: + with suppress(Exception): s3c.Bucket(self.bucket).delete() - except Exception: - pass🧰 Tools
🪛 Ruff (0.8.2)
45-48: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
54-64: Consider making S3 configuration configurable.The S3 configuration (endpoint URL, access key, secret key) is currently hardcoded. Consider making these values configurable through environment variables or test configuration files for better flexibility.
+from os import environ + @property def endpoint_url(self): - return "http://127.0.0.1:9000" + return environ.get("MINIO_ENDPOINT_URL", "http://127.0.0.1:9000") @property def access_key(self): - return "minio" + return environ.get("MINIO_ACCESS_KEY", "minio") @property def secret_key(self): - return "minio123" + return environ.get("MINIO_SECRET_KEY", "minio123")snakemake_executor_plugin_aws_batch/__init__.py (2)
197-234: Enhance job status retrieval with better error handling.The job status retrieval can be improved by:
- Using a more descriptive error message when no jobs are found
- Adding logging for specific failure scenarios
try: response = self.batch_client.describe_jobs(jobs=[job.external_jobid]) jobs = response.get("jobs", []) if not jobs: - return None, f"No job found with ID {job.external_jobid}" + error_msg = f"Job {job.external_jobid} not found in AWS Batch. It may have been deleted or expired." + self.logger.warning(error_msg) + return None, error_msg job_info: dict = jobs[0] job_status = job_info.get("status", "UNKNOWN") # push the job_definition_arn to the aux dict for use in cleanup - job.aux["job_definition_arn"] = job_info.get("jobDefinition", None) + job.aux["job_definition_arn"] = job_info.get("jobDefinition") exit_code = job_info.get("container", {}).get("exitCode", None) if job_status == "SUCCEEDED": return 0, None elif job_status == "FAILED": reason = job_info.get("statusReason", "Unknown reason") + self.logger.error(f"Job {job.external_jobid} failed: {reason}") return exit_code or 1, reason else: log_info = { "job_name": job_info.get("jobName", "unknown"), "job_id": job.external_jobid, "status": job_status, } self.logger.debug(log_info) return None, None except Exception as e: - self.logger.error(f"Error getting job status: {e}") + error_msg = f"Error retrieving status for job {job.external_jobid}: {str(e)}" + self.logger.error(error_msg) return None, str(e)🧰 Tools
🪛 Ruff (0.8.2)
215-215: Use
job_info.get("jobDefinition")instead ofjob_info.get("jobDefinition", None)Replace
job_info.get("jobDefinition", None)withjob_info.get("jobDefinition")(SIM910)
235-248: Improve job termination error handling.The job termination error handling can be enhanced by:
- Using a more descriptive error message
- Adding proper error logging
try: - self.logger.debug(f"terminating job {job.external_jobid}") + self.logger.info(f"Terminating AWS Batch job {job.external_jobid}") self.batch_client.terminate_job( jobId=job.external_jobid, reason="terminated by snakemake", ) + self.logger.debug(f"Successfully terminated job {job.external_jobid}") except Exception as e: - job_spec = job.aux["job_params"] - self.logger.info( - f"failed to terminate Batch job definition: {job_spec} with error: {e}" + error_msg = f"Failed to terminate AWS Batch job {job.external_jobid}: {str(e)}" + self.logger.error( + error_msg, + extra={"job_params": job.aux.get("job_params", {})} )tests/docker-compose.yml (2)
4-11: Add health check for MinIO service.Consider adding a health check to ensure the MinIO service is ready before tests begin.
minio: image: minio/minio ports: - "9000:9000" environment: MINIO_ACCESS_KEY: minio MINIO_SECRET_KEY: minio123 command: server /data + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:9000/minio/health/live"] + interval: 5s + timeout: 3s + retries: 3🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
11-11: Add new line at end of file.Add a new line at the end of the file to comply with POSIX standards.
command: server /data +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/ci_true_api.yml (1)
37-43: Enhance AWS credentials configuration security.Consider adding additional security measures:
- Set
mask-aws-account-id: trueto prevent account ID leakage in logs- Add
role-duration-secondsto limit the session duration- name: Configure AWS Credentials uses: aws-actions/configure-aws-credentials@v4 with: aws-region: "${{ secrets.SNAKEMAKE_AWS_BATCH_REGION }}" role-to-assume: "${{ secrets.GH_AWS_ROLE_ARN }}" role-session-name: "GitHubActions" + mask-aws-account-id: true + role-duration-seconds: 900
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci_mocked_api.yml(2 hunks).github/workflows/ci_true_api.yml(2 hunks)snakemake_executor_plugin_aws_batch/__init__.py(7 hunks)tests/__init__.py(1 hunks)tests/docker-compose.yml(1 hunks)tests/tests_mocked_api.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tests_mocked_api.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
snakemake_executor_plugin_aws_batch/__init__.py (1)
Pattern **/*.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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.
🪛 YAMLlint (1.35.1)
tests/docker-compose.yml
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Ruff (0.8.2)
tests/__init__.py
45-48: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
snakemake_executor_plugin_aws_batch/__init__.py
215-215: Use job_info.get("jobDefinition") instead of job_info.get("jobDefinition", None)
Replace job_info.get("jobDefinition", None) with job_info.get("jobDefinition")
(SIM910)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: linting
🔇 Additional comments (4)
.github/workflows/ci_mocked_api.yml (4)
12-15: Enhanced Workflow Permissions
The addition of thepermissionsblock withid-token: writeandcontents: readis appropriate for securely enabling OIDC-based role assumptions and related actions. Ensure that these permissions follow the least privilege principle.
91-106: Docker Compose and MinIO Setup Integration
The newly added steps to install Docker Compose, start the MinIO service, install the MinIO Client CLI, and configure the MinIO client are well-aligned with the integration testing requirements.
- Consider adding a check to skip the installation of Docker Compose if it is already available on the runner to save execution time.
- For downloading the MinIO Client CLI, verifying the binary’s checksum or signature could enhance security and stability.
110-116: AWS Credentials Configuration Step
The step leveragingaws-actions/configure-aws-credentials@v4is properly set up, sourcing the AWS region and role from secrets. Double-check thatGH_AWS_ROLE_ARNis the intended secret for the role to assume.
118-121: Test Environment Variable Configuration
The environment variables for AWS Batch configuration in theRun pyteststep are correctly provided. Please ensure the YAML indentation meets linting standards—this block appears correct, but it’s always good to confirm with your YAML linter.
Big refactoring and improvements
Summary by CodeRabbit
Release Notes for Snakemake AWS Batch Executor Plugin
New Features
BatchClientfor managing AWS Batch job interactions.BatchJobBuilderto streamline job definition and submission processes.Improvements
Infrastructure
Development
.gitignoreto manage version control behavior effectively.