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

qcengine info psi4 harness raises exception if system uses pyenv #292

Closed
coltonbh opened this issue Mar 10, 2021 · 7 comments · Fixed by #418
Closed

qcengine info psi4 harness raises exception if system uses pyenv #292

coltonbh opened this issue Mar 10, 2021 · 7 comments · Fixed by #418

Comments

@coltonbh
Copy link
Contributor

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.

@loriab
Copy link
Collaborator

loriab commented Mar 10, 2021

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.

@coltonbh
Copy link
Contributor Author

coltonbh commented Mar 11, 2021

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.

@Flamefire
Copy link

Flamefire commented Dec 5, 2022

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 added a commit to Flamefire/QCEngine that referenced this issue Dec 5, 2022
When `psi4` binary is found but the Python package is not we should not overwrite the `$PYTHONPATH` before importing `psi4` as that may cause other required packages to be not found anymore.
Hence prepend it.

See MolSSI#292
@loriab
Copy link
Collaborator

loriab commented Aug 8, 2023

I think #418 solves @Flamefire 's issues but not @coltonbh 's original one from #292 (comment) . 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?

@coltonbh
Copy link
Contributor Author

coltonbh commented Aug 8, 2023

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

@Flamefire
Copy link

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?

@loriab
Copy link
Collaborator

loriab commented Aug 12, 2023

MolSSI/QCElemental#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants