Skip to content

[Bug] Raise error when pysqa is not available #677

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.

Sign up for GitHub

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

Merged
jan-janssen merged 3 commits into from
Jun 14, 2025
Merged

[Bug] Raise error when pysqa is not available #677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d056d8
[Bug] Raise error when pysqa is not available
jan-janssen Jun 14, 2025
99cc720
ignore # noqa
jan-janssen Jun 14, 2025
15f8bb4
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions executorlib/executor/flux.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,8 @@ def __init__(
{k: v for k, v in default_resource_dict.items() if k not in resource_dict}
)
if not plot_dependency_graph:
import pysqa # noqa

Comment on lines +337 to +338
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 exc

This 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.

from executorlib.task_scheduler.file.task_scheduler import (
create_file_executor,
)
Expand Down
2 changes: 2 additions & 0 deletions executorlib/executor/slurm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@
{k: v for k, v in default_resource_dict.items() if k not in resource_dict}
)
if not plot_dependency_graph:
import pysqa # noqa

Check warning on line 154 in executorlib/executor/slurm.py

View check run for this annotation

Codecov / codecov/patch

executorlib/executor/slurm.py#L154 

Added line #L154 was not covered by tests

Comment on lines +154 to +155
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 exc

Replicating 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.

Suggested change
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.

from executorlib.task_scheduler.file.task_scheduler import (
create_file_executor,
)
Expand Down
Loading