JudeWells / chainsaw

MIT License
27 stars 2 forks source link

Remove hard-coded python environment variable in renum_pdb_file() #18

Closed JudeWells closed 1 year ago

JudeWells commented 1 year ago

The following code block will not run if eg. user's environment has python=python version 2

def renum_pdb_file(pdb_path, output_pdb_path):
    pdb_reres_path = REPO_ROOT / 'src/utils/pdb_reres.py'
    with open(output_pdb_path, "w") as output_file:
        subprocess.run(['python', str(pdb_reres_path), pdb_path],
                       stdout=output_file,
                       check=True,
                       text=True)
sillitoe commented 1 year ago

What does the error look like?

Are we trying to make this backwards compatible with python 2?

JudeWells commented 1 year ago

Not trying to make it backwards compatible with python3 but someone told me they had this problem at ISMB. The problem is that we are essentially hard-coding the bash command: python path/to/pdb_reres.py ex01.pdb For some users who have python2 and python3 installed on their machine python is going to be an alias for python version 2. And the correct command for them to run this would be: python3 path/to/pdb_reres.py ex01.pdb

Possible ways to fix:

  1. call the functions in pdb_reres.py without using subprocess
  2. allow user to specify the python path (to guarantee it points to python3)
  3. look up which python install is running the main script and make sure the same python install is called in this subprocess (is this possible?)
sillitoe commented 1 year ago

Ah right, thanks.

  1. Rename: python -> python3 ?

Not perfect, but should solve the original problem.

JudeWells commented 1 year ago

On some windows installs i think python3 doesn't get defined maybe this is the best fix: 'python' -> sys.executable (I think this should always point to to python install that is running the main script).

sillitoe commented 1 year ago

Yup, much nicer solution

JudeWells commented 1 year ago

Fixed with commit 35e2bb86fa313ef0c7cb4eb7768ef3850efe23cf