[Bug] Raise error when pysqa is not available #677
Conversation
WalkthroughThe changes introduce explicit imports of the Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #677 +/- ##
==========================================
- Coverage 96.82% 96.75% -0.08%
==========================================
Files 29 29
Lines 1292 1294 +2
==========================================
+ Hits 1251 1252 +1
- Misses 41 42 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jan-janssen
force-pushed
the
import_pysqa
branch
from
eaa6864 to
99cc720
Compare
for more information, see https://pre-commit.ci
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
executorlib/executor/flux.py(1 hunks)executorlib/executor/slurm.py(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
executorlib/executor/slurm.py
[warning] 154-154: executorlib/executor/slurm.py#L154
Added line #L154 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_win
- GitHub Check: notebooks_integration
| import pysqa # noqa | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap pysqa import in try/except and surface a clear installation hint
A bare import pysqa will raise ModuleNotFoundError, but the default traceback does not help users understand why the executor fails. Provide a short, actionable message so the failure is self-explanatory.
- import pysqa # noqa
+ try:
+ import pysqa # noqa
+ except ModuleNotFoundError as exc:
+ raise ModuleNotFoundError(
+ "pysqa is required for SlurmClusterExecutor. "
+ "Install it with `pip install pysqa`."
+ ) from excReplicating the guard used for optional dependencies elsewhere in the codebase will improve UX and prevent support noise.
📝 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.
| import pysqa # noqa | |
| try: | |
| import pysqa # noqa | |
| except ModuleNotFoundError as exc: | |
| raise ModuleNotFoundError( | |
| "pysqa is required for SlurmClusterExecutor. " | |
| "Install it with `pip install pysqa`." | |
| ) from exc |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 154-154: executorlib/executor/slurm.py#L154
Added line #L154 was not covered by tests
🤖 Prompt for AI Agents
In executorlib/executor/slurm.py around lines 154 to 155, the import of pysqa is
done without error handling, which causes a ModuleNotFoundError with an unclear
traceback if the module is missing. Wrap the import statement in a try/except
block catching ModuleNotFoundError, and in the except block raise a new
ImportError with a clear, actionable message instructing the user to install
pysqa to use this executor. This pattern matches the existing optional
dependency guards in the codebase and improves user experience.
| import pysqa # noqa | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the safeguarded import pattern used for SLURM executor
Same rationale as in SlurmClusterExecutor—surface a concise hint when pysqa is absent.
- import pysqa # noqa
+ try:
+ import pysqa # noqa
+ except ModuleNotFoundError as exc:
+ raise ModuleNotFoundError(
+ "pysqa is required for FluxClusterExecutor. "
+ "Install it with `pip install pysqa`."
+ ) from excThis keeps behaviour consistent across back-ends and provides an immediate, informative error.
🤖 Prompt for AI Agents
In executorlib/executor/flux.py around lines 337 to 338, the import of pysqa
should follow the safeguarded pattern used in SlurmClusterExecutor to handle
missing dependencies gracefully. Modify the import to try importing pysqa inside
a try-except block, and if ImportError occurs, raise an ImportError with a
clear, concise message indicating that pysqa is required for this executor. This
ensures consistent behavior across back-ends and provides an immediate,
informative error when pysqa is not installed.
Summary by CodeRabbit