venv replacement - Githubissues

cedadev / ceda-notebooks

Jupyter Notebooks demonstrating CEDA data and tools

BSD 2-Clause "Simplified" License

6 stars 12 forks source link

venv replacement #30

Closed Mahir-Sparkess closed 2 years ago

Mahir-Sparkess commented 2 years ago

Reformatting:

moved ceda-notebooks/notebooks/mngmnt/ to ceda-notebooks/scripts
moved gridlines.py to scripts

venv replacement

changed the venv_utils.py so that it works without venv and activate_this.py
changed the methods and required parameters.

notebooks

changed how venvs are created in the notebooks to utilise the script instead.
proofed the notebooks using the jasmin notebook service.

Mahir-Sparkess commented 2 years ago

Thanks Ag, changed the string to format the python version now. 👍

agstephens commented 2 years ago

Hi @Mahir-Sparkess, another quick (I hope) change request. When setting up the imports of the venv_utils library, I prefer:

import os
import sys
sys.path.append('../../../')

from scripts.utils import venv_utils

instead of:

os.chdir('../../..')

I think it is less confusing if we avoid changing the working directory - which could impact where outputs are written further down the notebooks.

Mahir-Sparkess commented 2 years ago

Hi @Mahir-Sparkess, another quick (I hope) change request. When setting up the imports of the venv_utils library, I prefer:
import os
import sys
sys.path.append('../../../')

from scripts.utils import venv_utils
instead of:
os.chdir('../../..')
I think it is less confusing if we avoid changing the working directory - which could impact where outputs are written further down the notebooks.

Yep, just an easy find and replace. Thanks @agstephens, makes sense for file creation which I wouldn't catch in the pytest.

amanning9 commented 2 years ago

@Mahir-Sparkess @agstephens quick poke to maybe take another look at this when you get a chance, please: the way this tutorial creates a virtual environment is currently broken, which stops the notebook from working!

agstephens commented 2 years ago

@Mahir-Sparkess How confident are you that the latest fix solves the problem of the venv creation? It's tricky to review because the notebook format obscures the changes.

Mahir-Sparkess commented 2 years ago

@Mahir-Sparkess How confident are you that the latest fix solves the problem of the venv creation? It's tricky to review because the notebook format obscures the changes.

@agstephens The venv creation has been solved for a while and works on the notebooks service + ssh vscode. 👍

I do have a discrepancy where some notebooks fail the pytest run as it can't find certain packages that should be in jaspy, unrelated to the venv. However, they all run fine on the notebooks service.

agstephens commented 2 years ago

@amanning9: Please test this out on master and let us know if the broken notebook is working again for you.

@Mahir-Sparkess: is it worth us trying to capture the dependencies from each notebook and put them all in a conda environment.yml file so that we know they will all run. The tests would be slower but would be comprehensive.

amanning9 commented 2 years ago

@agstephens @Mahir-Sparkess

The notebook I had a user query about (https://github.com/cedadev/ceda-notebooks/blob/master/notebooks/training/intro/notebook-tour-part-6.ipynb) seems to create venvs successfully and install things to them OK, so on that front you can count it as fixed, thanks!

However, I'll just highlight that I do get the following telling-off from pip:

Perhaps not currently important, but worth noting!