Skip to content

Unexpected behavior caused by _build_python_path #40

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

Open
rodifelix opened this issue May 3, 2021 · 2 comments
Open

Unexpected behavior caused by _build_python_path #40

rodifelix opened this issue May 3, 2021 · 2 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@rodifelix
Copy link
Collaborator

The procedure _build_python_path leads to some unexpected behavior (IMO) in two cases.

  1. If I start an experiment with python3 main.py config.yml -s while conda environment A is activated and config.yml contains a venv variable pointing to conda environment B, the PYTHONPATH (or more precisely the value of sys.path) of environment A is copied and used in the sbatch.sh file (Line 25). I would expect the PYTHONPATH to be exported with the PYTHONPATH (or sys.path) of the environment specified in the venv config variable. As it is now, the PYTHONPATH specified in and activated with environment B in Line 21 will be overwritten with the sbatch script in Line 25.

# -------------------------------
# Activate the virtualenv / conda environment
%%venv%%
# Export Pythonpath
%%pythonpath%%
# Additional Instructions from CONFIG.yml
%%sh_lines%%
python3 %%python_script%% %%path_to_yaml_config%% -j $SLURM_ARRAY_TASK_ID %%cw_args%%
# THIS WAS BUILT FROM THE DEFAULLT SBATCH TEMPLATE

  1. When setting experiment_copy_dst or experiment_copy_auto_dst the generated PYTHONPATH still contains references to the original working directory (and not the copy) in some cases. As defined in https://docs.python.org/3/library/sys.html#sys.path, the first entry of sys.path is the directory "containing the script that was used to invoke the Python interpreter". As the script is invoked in the "uncopied" src directory, the PYTHONPATH will contain this entry if the script (the main.py file) is in a subfolder of experiment_copy_src.
    This happens because the code below only checks for exact copies of sc["experiment_copy_src"] and not paths containing sc["experiment_copy_src"] . I think this also happens when the PYTHONPATH contains some manually added entries to subfolders of sc["experiment_copy_src"]. Changing the procedure to replace instances of sc["experiment_copy_src"] with dst would solve this issue.

cw2/cw2/cw_slurm.py

Lines 272 to 288 in 47d5eff

def _build_python_path(sc: attrdict.AttrDict) -> str:
"""clean the python path for the new experiment copy
Args:
sc (attrdict.AttrDict): slurm configuration
Returns:
str: python path bash command for slurm script
"""
pypath = sys.path.copy()
src = sc["experiment_copy_src"]
dst = sc["experiment_copy_dst"]
new_path = [x for x in pypath if x != src]
new_path.append(dst)
return "export PYTHONPATH=" + ":".join(new_path)

@LiXiling LiXiling added bug Something isn't working enhancement New feature or request labels May 4, 2021
@LiXiling
Copy link
Collaborator

LiXiling commented May 4, 2021

New commit hopefully fixes this issue. Let me know if it helped / something breaks

@rodifelix
Copy link
Collaborator Author

Sorry for getting back so late. I was a bit busy and didn't have the time to test the changes.

Looking at your changes, I noticed a case where src directory paths are written to PYTHONPATH.

return "export PYTHONPATH=$PYTHONPATH:" + ":".join(new_path)

Including export PYTHONPATH=$PYTHONPATH: will still copy the paths from my environment that lead to the src directory.

So I think there are two cases:

  1. A virtual environment is specified in the config. Then one would need to read the PYTHONPATH of that environment, change it if needed (replacing occurrences of src), and copy it to the export command. Removing the need of PYTHONPATH=$PYTHONPATH.
  2. No virtual environment is specified. All paths of the execution environment are already in sys.path, so again no PYTHONPATH=$PYTHONPATH is needed

@LiXiling LiXiling reopened this May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants