CI: move PyPI deps installation to conda#565
Conversation
paper: add minor fixes for acceptance
release: patch 3.0.1
DaniBodor
left a comment
There was a problem hiding this comment.
The installation from env file doesn't work for me. I got the following problem Collecting package metadata (repodata.json): / Killed, which after some googling seems to be a OOM issue. I do get this more often when trying to install envs from yml files, so could be related to my system specifically.
Given that we will migrate to conda soon anyway and it works on the CI, it's probably ok for now. Perhaps it would make sense to just ask someone who hasn't had prior problems with yml-env installations to test it as well?
Note: I also haven't tested the updated docker installation, but was only minor changes, so should be ok.
I will approve this now and you can proceed as you see fit (also re: README comments).
There was a problem hiding this comment.
shouldn't the python version be specified here as well (as it is in deeprank2-docker.yml)?
There was a problem hiding this comment.
Here it is specified. Maybe you were referring to the environment.yml file? There it is not specified since the action controls the python version. Otherwise we can't use the same yml file for different python versions builds on the CI (in principle we want to test both python 3.10 and 3.11, see my PR's more extensive comment on top)
There was a problem hiding this comment.
Sorry, I indeed commented on the wrong file, I meant to comment on deeprank2.yml (environment.yml was deleted in this PR).
I understand your point about the CI, but I was primarily thinking of users. Don't we need to ensure that the python version in their env is also 3.10 or 3.11?
There was a problem hiding this comment.
Yes sorry, indeed I was also referring to the deeprank2.yml file. Very good point :) I added python==3.10 specification in the yml file, which then will switch to python>=3.10,<3.12 when #540 will be fixed (I've already added a comment about that in the issue). At that point, it will use whatever python version the user has between 3.10 and 3.11. And the CI will be free to set different python versions in the different builds.
I've also done the installation (with the new python specification in the yml file) on Snellius following the readme instructions, and all tests pass locally. So I think I can merge, just first give me a signal here @DaniBodor
Co-authored-by: Dani Bodor <d.bodor@esciencecenter.nl>
With this PR:
env/deeprank.yml) which is used for building the environment. Both the CI and the user's installations are much smoother and cleaner now.deeprank2-docker.ymlandrequirements-docker.txt). The issue was that while in the CI we want to be able to set up the python version from outside the YML file, to test multiple versions (see also issue Fix build for Python 3.11 #540). But in theDockerfilewe need to create the env and install the dependencies (python included) in one go, and I didn't find any way to include both the python version in the mamba/conda command AND the YML input file for the env. If I create an environment specifying python 3.10 for example, it gets then very tricky to activate it during the creation of the image itself for updating it and installing the deps listed in the YML (I concluded that it's not possible or at least not the standard way to go). I had to do the same for the TXT file since in the Docker env creation we need to have installed deeprank2 as well (but in the CI we want to decouple that as well). If this is not clear enough or you have ideas for improving we can have a chat of course!