Skip to content
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

Use try/except to catch error #917

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Changes from 6 commits
Commits
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
49 changes: 40 additions & 9 deletions src/aiidalab_qe/setup/codes.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import json
import subprocess
from pathlib import Path
from shutil import which
Expand Down Expand Up @@ -44,18 +45,48 @@ def get_qe_env():


def qe_installed():
import json
"""Check if Quantum Espresso (QE) is installed in the specified conda environment.

env_exist = get_qe_env().exists()
proc = subprocess.run(
["conda", "list", "-n", f"{get_qe_env().name}", "--json", "--full-name", "qe"],
check=True,
capture_output=True,
)
Returns:
bool: True if the environment exists and QE is installed; False otherwise.
"""
try:
# Verify if the specified conda environment exists
env_exist = get_qe_env().exists()

if not env_exist:
return False
Comment on lines +55 to +58
Copy link
Member Author

Choose a reason for hiding this comment

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

Because we check if the conda env exists here, I didn't add the code to run the code cmd using the subprocess.


# Run the conda list command to check for the QE package
proc = subprocess.run(
[
"conda",
"list",
"-n",
f"{get_qe_env().name}",
"--json",
"--full-name",
"qe",
],
check=True,
capture_output=True,
)

# Load and interpret the JSON output
info = json.loads(proc.stdout.decode())

info = json.loads(str(proc.stdout.decode()))[0]
# Check if 'qe' is listed in the environment
for package in info:
if package.get("name") == "qe":
return True
return False # noqa: TRY300

return env_exist and "qe" == info["name"]
except subprocess.CalledProcessError:
# Handle cases where the conda list command fails
return False
Copy link
Member

@unkcpz unkcpz Nov 8, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the comment! I didn't consider docker when modifying the code. Could we have a short zoom chat on this?

Copy link
Member

Choose a reason for hiding this comment

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

At 2 pm?

except (json.JSONDecodeError, IndexError):
# Handle cases where the JSON output is invalid or missing expected data
return False


def install_qe():
Expand Down
Loading