Skip to content
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

CI: move PyPI deps installation to conda #565

Merged
merged 43 commits into from
Feb 28, 2024
Merged

Conversation

gcroci2
Copy link
Collaborator

@gcroci2 gcroci2 commented Feb 6, 2024

With this PR:

  • I moved all the package's dependencies installations to conda, so now they can all live in a YML file (env/deeprank.yml) which is used for building the environment. Both the CI and the user's installations are much smoother and cleaner now.
  • We use a new conda-incubator action for creating a conda env in the CI which allows you to provide a YML file for building the env.
    • This is much more convenient now that we use the YML file for almost everything. The action uses mamba - a drop-in replacement for conda that is generally faster and better at resolving dependencies. See here for more details about it.
  • For the Docker installation, I added two completely separated files in env/ (deeprank2-docker.yml and requirements-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 the Dockerfile we 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!
  • For testing (not required, I did it already, but a double check doesn't hurt)
    • Follow the instructions for the Docker container
    • Try to setup the env with the new procedure and run the tests locally

@gcroci2 gcroci2 self-assigned this Feb 6, 2024
@coveralls
Copy link

coveralls commented Feb 6, 2024

Coverage Status

coverage: 83.613% (+0.1%) from 83.499%
when pulling 43fb72d on 559_conda_migration_gcroci2
into 39bceae on main.

@gcroci2 gcroci2 linked an issue Feb 6, 2024 that may be closed by this pull request
6 tasks
@gcroci2 gcroci2 mentioned this pull request Feb 19, 2024
6 tasks
@gcroci2 gcroci2 changed the base branch from main to dev February 21, 2024 10:52
@gcroci2 gcroci2 requested a review from DaniBodor February 22, 2024 14:07
Copy link
Collaborator

@DaniBodor DaniBodor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
docs/installation.md Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't the python version be specified here as well (as it is in deeprank2-docker.yml)?

Copy link
Collaborator Author

@gcroci2 gcroci2 Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go for it!

README.md Outdated Show resolved Hide resolved
@gcroci2 gcroci2 merged commit b7a7d05 into dev Feb 28, 2024
5 checks passed
@gcroci2 gcroci2 deleted the 559_conda_migration_gcroci2 branch February 28, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conda migration
3 participants