fix: sshare testing as an alternative to sacctmgr account tests #178
Conversation
WalkthroughThe changes in this pull request enhance the error handling logic within the Changes
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
snakemake_executor_plugin_slurm/__init__.py
566-566: 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
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
636-650: Improve error handling in account validationThe implementation of a fallback mechanism to
ssharewhensacctmgrfails is a good improvement for robustness. However, there are a few improvements that can be made:
- The exception should be chained using
raise ... from eto maintain the error context- The error messages could be more concise based on past feedback
Apply this diff to improve the implementation:
- sacctmgr_report = ( - "Unable to test the validity of the given or guessed " - f"SLURM account '{account}' with sacctmgr: {e.stderr}." - ) - try: - cmd = "sshare -U --format Account --noheader" - accounts = subprocess.check_output( - cmd, shell=True, text=True, stderr=subprocess.PIPE - ) - except subprocess.CalledProcessError as e2: - sshare_report = ( - "Unable to test the validity of the given or guessed" - f" SLURM account '{account}' with sshare: {e2.stderr}." - ) - raise WorkflowError(sacctmgr_report + " - and - " + sshare_report) + try: + cmd = "sshare -U --format Account --noheader" + accounts = subprocess.check_output( + cmd, shell=True, text=True, stderr=subprocess.PIPE + ) + except subprocess.CalledProcessError as e2: + raise WorkflowError( + f"Failed to validate account '{account}' with sacctmgr and sshare" + ) from e🧰 Tools
🪛 Ruff (0.8.2)
650-650: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/post_to_mastodon.yml(1 hunks)snakemake_executor_plugin_slurm/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
snakemake_executor_plugin_slurm/__init__.py
650-650: 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 (2)
- GitHub Check: testing
- GitHub Check: linting
🔇 Additional comments (1)
.github/workflows/post_to_mastodon.yml (1)
24-24: Verify the MASTODONBOT secret is configuredThe secret token for Mastodon authentication has been renamed from
MASTODON_ACCESS_TOKENtoMASTODONBOT. Please ensure this secret is properly configured in the repository settings.
|
@johanneskoester might give it a check? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
645-650: Consider adding exception chaining for better error tracking.The error handling is good, but could be improved by adding exception chaining to preserve the original error context.
Apply this diff to add exception chaining:
- raise WorkflowError(f"{sacctmgr_report} {sshare_report}") + raise WorkflowError(f"{sacctmgr_report} {sshare_report}") from e🧰 Tools
🪛 Ruff (0.8.2)
650-650: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
snakemake_executor_plugin_slurm/__init__.py
650-650: 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
🔇 Additional comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
636-639: LGTM! Clear and informative error message.The error message for sacctmgr failure is well-formatted and provides useful debugging information.
640-644: LGTM! Good fallback mechanism.The implementation of sshare as a fallback for account validation is a robust solution that improves reliability.
|
This has an infinite loop now. |
…share will fail to test the account
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
636-653: LGTM! The fallback mechanism is a good addition.The implementation of
sshareas a fallback whensacctmgrfails improves the robustness of account validation. The error messages are clear and informative.Consider using proper exception chaining to maintain the traceback information:
- raise WorkflowError( - f"The 'sacctmgr' reported: '{sacctmgr_report}' " - f"and likewise 'sshare' reported: '{sshare_report}'." - ) + raise WorkflowError( + f"The 'sacctmgr' reported: '{sacctmgr_report}' " + f"and likewise 'sshare' reported: '{sshare_report}'." + ) from e🧰 Tools
🪛 Ruff (0.8.2)
650-653: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
snakemake_executor_plugin_slurm/__init__.py
650-653: 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: testing
🤖 I have created a release *beep* *boop* --- ## [0.12.1](v0.12.0...v0.12.1) (2025-01-15) ### Bug Fixes * sshare testing as an alternative to sacctmgr account tests ([#178](#178)) ([38fa919](38fa919)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Addresses issue #177
Summary by CodeRabbit