-
Notifications
You must be signed in to change notification settings - Fork 29
fix: sshare testing as an alternative to sacctmgr account tests #178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
sshare
whensacctmgr
fails is a good improvement for robustness. However, there are a few improvements that can be made:
- The exception should be chained using
raise ... from e
to 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
except
clause, raise exceptions withraise ... from err
orraise ... from None
to 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_TOKEN
toMASTODONBOT
. Please ensure this secret is properly configured in the repository settings.
@johanneskoester might give it a check? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
except
clause, raise exceptions withraise ... from err
orraise ... from None
to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
sshare
as a fallback whensacctmgr
fails 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
except
clause, raise exceptions withraise ... from err
orraise ... from None
to 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