Use try/except to catch error #917
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #917 +/- ##
==========================================
- Coverage 67.06% 66.97% -0.10%
==========================================
Files 51 51
Lines 4715 4723 +8
==========================================
+ Hits 3162 3163 +1
- Misses 1553 1560 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Very funny thing: the |
src/aiidalab_qe/setup/codes.py
Outdated
| return env_exist and "qe" == info["name"] | ||
| except subprocess.CalledProcessError: | ||
| # Handle cases where the conda list command fails | ||
| return False |
There was a problem hiding this comment.
This may not correct, if the function return from exception, the exception will not propagate up.
The exception was handled by where the function _install who is calling qe_installed().
The change here will shadow docker build to show the exception message when it is failed.
I agree here we need capture the except case, but instead of return, the except is better to propagate up to handle, since that means the subprocess call is problematic.
There was a problem hiding this comment.
Thanks for the comment! I didn't consider docker when modifying the code. Could we have a short zoom chat on this?
|
As suggested by @unkcpz , if the cmd to inspect the env fails, we should raise the error and stop the installation. |
| env_exist = get_qe_env().exists() | ||
|
|
||
| if not env_exist: | ||
| return False |
There was a problem hiding this comment.
Because we check if the conda env exists here, I didn't add the code to run the code cmd using the subprocess.
| return False # noqa: TRY300 | ||
| except Exception as error: | ||
| raise RuntimeError( | ||
| "Failed to check if Quantum Espresso is installed." | ||
| ) from error |
There was a problem hiding this comment.
| return False # noqa: TRY300 | |
| except Exception as error: | |
| raise RuntimeError( | |
| "Failed to check if Quantum Espresso is installed." | |
| ) from error | |
| except Exception as exc: | |
| raise RuntimeError( | |
| "Failed to check if Quantum Espresso is installed." | |
| ) from exc | |
| else: | |
| return False # noqa: TRY300 |
Also for the catched exception, I think there are two types of exceptions we know which are SubprocessCallError and JsonParseError. You can catch those two and for uncatched exceptions, use raise to propagated it up.
There was a problem hiding this comment.
For TRY300, you can move it to else block, https://docs.astral.sh/ruff/rules/try-consider-else/
What you mean |
| return False # noqa: TRY300 | ||
| except Exception as error: | ||
| raise RuntimeError( | ||
| "Failed to check if Quantum Espresso is installed." | ||
| ) from error |
There was a problem hiding this comment.
For TRY300, you can move it to else block, https://docs.astral.sh/ruff/rules/try-consider-else/
Fix #916
The error from 916 is because the setup process stopped because the code failed to inspect the qe env. In this PR, we handle this gracefully so that it returns False when failed to inspect qe env.