QCEngine icon indicating copy to clipboard operation
QCEngine copied to clipboard

qcengine info psi4 harness raises exception if system uses pyenv

Open coltonbh opened this issue 4 years ago • 2 comments

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

  1. Install pyenv
  2. Install a miniconda/anaconda version
  3. Create a conda env and install psi4 inside it
  4. Run qcengine info when 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.

coltonbh avatar Mar 10 '21 21:03 coltonbh

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.

loriab avatar Mar 10 '21 21:03 loriab

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.

coltonbh avatar Mar 11 '21 01:03 coltonbh

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 PYTHONPATH to run python -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)

Flamefire avatar Dec 05 '22 13:12 Flamefire

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?

loriab avatar Aug 08 '23 18:08 loriab

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

coltonbh avatar Aug 08 '23 22:08 coltonbh

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?

Flamefire avatar Aug 09 '23 08:08 Flamefire

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.

loriab avatar Aug 12 '23 02:08 loriab