QCEngine
QCEngine copied to clipboard
qcengine info psi4 harness raises exception if system uses pyenv
Describe the bug
Running qcengine info raises the following error if a system has installed python versions using pyenv.
❯ qcengine info
>>> Version information
QCEngine: v0.18.0+1.g02aa5ef
QCElemental: v0.18.0
>>> Program information
Traceback (most recent call last):
File "/Users/cbh/dev/mtzgroup/QCEngine/env/bin/qcengine", line 33, in <module>
sys.exit(load_entry_point('qcengine', 'console_scripts', 'qcengine')())
File "/Users/cbh/dev/mtzgroup/QCEngine/qcengine/cli.py", line 162, in main
info_cli(args)
File "/Users/cbh/dev/mtzgroup/QCEngine/qcengine/cli.py", line 125, in info_cli
info_programs()
File "/Users/cbh/dev/mtzgroup/QCEngine/qcengine/cli.py", line 84, in info_programs
avail_progs = list_available_programs()
File "/Users/cbh/dev/mtzgroup/QCEngine/qcengine/programs/base.py", line 97, in list_available_programs
if p.found():
File "/Users/cbh/dev/mtzgroup/QCEngine/qcengine/programs/psi4.py", line 64, in found
sys.path.append(exc["stdout"].split()[-1])
IndexError: list index out of range
To Reproduce
- Install pyenv
- Install a miniconda/anaconda version
- Create a conda env and install psi4 inside it
- Run
qcengine infowhen this environment is not activated
Expected behavior
I would expect qcengine to simply omit psi4 from the available programs.
Additional context
The issue is raised due to the exc variable having the following value:
{'proc': <Popen: returncode: 127 args: ['/Users/cbh/.pyenv/shims/psi4', '--module']>, 'stdout': '', 'stderr': "pyenv: psi4: command not found\n\nThe `psi4' command exists in these Python versions:\n miniconda3-4.7.12/envs/p4env\n\nNote: See 'pyenv help global' for tips on allowing both\n python2 and python3 to be found.\n"}
which does not include a stdout value that can be split.
Thanks for the detailed analysis. Is https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/psi4.py#L55 coming back True such that the psi4 --module block even gets triggered? That seems odd.
Yes. Because if you use pyenv to manage your python environments (very common in software engineering land--perhaps less common in science land given I'm the first to raise this issue!) you get the following. This is from running the debugger on line 56.
(Pdb) which("psi4")
'/Users/cbh/.pyenv/shims/psi4'
pyenv knows where this executable is--it's in one of the python installs it manages (in this case inside a conda env created from a miniconda install), but that environment is not activated. Hence the output seen above.
I've a reported similar issue: https://gist.github.com/sassy-crick/f5b058bc9c4fd7be44d730eb17ec4e6d
In this case the psi4 binary does not exist (in PATH) but the module does
It's from the test suite of PSI4 and the relevant log part is:
File "/dev/shm/easybuild/PSI4/1.6.1/foss-2021b/easybuild_obj/stage/lib/psi4/driver/task_base.py", line 161, in compute
self.result = qcng.compute(
File "/home/sassy/apps/software/qcengine/0.24.1-foss-2021b/lib/python3.9/site-packages/qcengine/compute.py", line 76, in compute
executor = get_program(program)
File "/home/sassy/apps/software/qcengine/0.24.1-foss-2021b/lib/python3.9/site-packages/qcengine/programs/base.py", line 79, in get_program
ret.found(raise_error=True)
File "/home/sassy/apps/software/qcengine/0.24.1-foss-2021b/lib/python3.9/site-packages/qcengine/programs/psi4.py", line 72, in found
os.environ["PATH"] += os.pathsep + exc["stdout"].split()[-1]
IndexError: list index out of range
I'm wondering why the code is seemingly overly complicated: https://github.com/MolSSI/QCEngine/blob/v0.24.1/qcengine/programs/psi4.py#L67-L72
- Check if you can/could
import psi4 - Replace
PYTHONPATHto runpython -c "import psi4" - Don't check any errors and try to parse the output
So I'd strongly suggest to add any kind of error checking first before accessing exc["stdout"]
Additionally fully replacing $PYTHONPATH may make importing the module fail.
So why not simply import psi4 and directly access psi4.executable?
EDIT: Overwriting PYTHONPATH is a major issue. Other packages (dependencies of psi4) like NumPy might be included via PYTHONPATH so putting only the single path there will make the import of numpy and hence psi4 fail (which is the case here)
I think #418 solves @Flamefire 's issues but not @coltonbh 's original one from https://github.com/MolSSI/QCEngine/issues/292#issuecomment-796343813 . I've read over pyenv's documentation, and it sounds like pyenv is a $PATH manipulator like conda, hooray. I think the place to address this is in qcelemental/util/importing.py which. Since psi4 is found in a pyenv shim but not the active shim, I propose adding a check that if the shutil.which path contains shims, then execute pyenv which and if that returns a path, then the fn returns found. If it doesn't return a path, then it's not the active shim, and fn returns not found. Does this sound like a good strategy?
Here's a function that does a quick fix for the pyenv situation I built for myself in another repo. Fix is pretty easy; hope this helps :)
def prog_available(program: str) -> bool:
"""Check if a program is available on the system.
Args:
program: The program to check.
Returns:
True if the program is available, False otherwise.
"""
path = shutil.which(program)
if not path:
return False
# Perform secondary checks
if ".pyenv/shims" in path:
# Executable is in non activated pyenv environment; often a conda env
print(
f"Program {program} is in a pyenv shim. It may become available if you "
f"run 'conda activate {path.split('/')[-1]}'"
)
return False
return True
I think #418 already does solve this by https://github.com/MolSSI/QCEngine/pull/418/files#diff-d66842b418a1c03f41438563803c987c9defd1811ea0af131e285ba540c8c0d6R70
According to the initial description stderr isn't empty -> Hence it will (correctly) error out, won't it?
However it might be that https://github.com/MolSSI/QCEngine/pull/418/files#diff-d66842b418a1c03f41438563803c987c9defd1811ea0af131e285ba540c8c0d6R69 is wrong treating an empty stdout as valid which it never is, is it?
So checking the return code and/or stdout/stderr should also already solve the pyenv/condaenv issues, doesn't it?
https://github.com/MolSSI/QCElemental/pull/322 should address the original issue now, regardless of stderr empty or not.
The case with empty stdout turning into Path(".") (which passes exist()) is now avoided in #418.
I'll plan on closing this issue with 418. Please let me know if you spy any more real or potential problems.