-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix image build and bump base image version #197
Conversation
docker/py/requirements.txt
Outdated
@@ -6,5 +6,4 @@ jupyterlab~=3.0.0 | |||
papermill~=2.3.0 | |||
powerline-shell~=0.7.0 | |||
requests>=2.20.0 | |||
setuptools==57.5.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove this? it will be globally available and would make sure that the user's environment uses the same setuptools as the renku venv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could also pin the pip and wheel versions in fact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I had pinned it only very recently when replacing pipx with a handmade virtual env, believing that this would determine also the version used inside the virtual env and thus prevent the 2-to-3 bug when installing renku.
This assumption was wrong, therefore I'm removing this line again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see, it always uses the latest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to go even further and strictly pin everything in the users environment, but for now I just wanted to fix the build and not pin a package version based on a renku requirement in an env that doesn't install renku.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sure which version it will use for the venv if nothing is specified, but you can determine it through the virtualenv command (which I didn't know before but I was pleased to find out).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok - is setuptools 57.5.0 still needed for renku to install properly? In that case I would say we should pin it also in the global environment in case the user wants to pip install it to use renku as a library in python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is still needed. good point, I added it to the user python env.
I can help out with a review. I notice that the py-batch build fails - should this be looked into first? |
Yes, that would be very much appreciated. Thanks a lot! 🙏 |
@TaoSunVoyage do we need to replicate pinning the Renku version in the batch Docker? I.e. like this: renkulab-docker/docker/py/Dockerfile Lines 65 to 71 in 771cc49
instead of renkulab-docker/docker/batch/Dockerfile Lines 23 to 28 in 771cc49
|
Thanks @gavin-k-lee, it looks nice. just wondering if it's possible to simply copy the folder of |
I think that could work! Do you want to try implementing that fix? |
@TaoSunVoyage any chance you could give it a try with the fix that you and @gavin-k-lee have discussed. We should try and get this fix to the users asap. |
The simple copy doesn't work as the python executable in the venv is a symbolic link to the conda python. So, I reinstalled it in batch image instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @TaoSunVoyage feel free to approve and merge.
* fix: pin setuptools, pip, wheel and renku in renku virtualenv * fix: solve certificate problem by upgrading base image * chore: pin setuptools also in user python env * fix: renku install * fix: typo in base arg Co-authored-by: Tao Sun <[email protected]>
* fix: pin setuptools, pip, wheel and renku in renku virtualenv * fix: solve certificate problem by upgrading base image * chore: pin setuptools also in user python env * fix: install renku with venv in batch image * fix: typo in base arg Co-authored-by: Tao Sun <[email protected]>
This bumps the notebooks base image from
lab-3.0.16
tolab-3.2.1
, so a good look from the @SwissDataScienceCenter/poc squad to check if that breaks something would be appreciated.