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

add mamba support #47

Merged
merged 4 commits into from
Jun 10, 2020
Merged

add mamba support #47

merged 4 commits into from
Jun 10, 2020

Conversation

jaimergp
Copy link
Member

@jaimergp jaimergp commented Jun 9, 2020

As discussed per #14.

This is my first TS GitHub Action, so I might have done things in a weird way. Also, I don't know how to test locally, so let's see if it works.

Please let me know how I can improve the TS idioms too!

@jaimergp
Copy link
Member Author

jaimergp commented Jun 9, 2020

Welp, seems to work! Unfortunately, there's an issue in how environments are installed in this action.

Mamba does not support mamba env update yet. In this action, the environment file is treated in a different way than the usual conda env create -f <file.yml>. Instead, it first creates an empty environment with only python <python-version> and then it updates it with the YML. Is there any reason behind this alternative strategy? Looks a bit redundant because you need two solving steps instead of a single one.

@goanpeca
Copy link
Member

goanpeca commented Jun 9, 2020

@jaimergp thanks for the PR. The reason was to simplify the logic but then id mamba has that limitation then we should probably change it.

@jaimergp
Copy link
Member Author

jaimergp commented Jun 9, 2020

@jaimergp thanks for the PR. The reason was to simplify the logic but then id mamba has that limitation then we should probably change it.

Seems that they might "look into it" today. Let's see :)

@jaimergp
Copy link
Member Author

jaimergp commented Jun 9, 2020

In the meantime, I have given it some thought (and this is probably out of the scope of this PR, so feel free to branch the discussion into a separate issue).

Right now, there are several ways a new environment gets initialized with this action:

  • base gets initialized by default, optionally changing the default python version. This can be the builtin Miniconda or an externally provided one.
  • A custom env gets created if an environment file is provided (or is a custom name is passed?). The way this works is by creating an empty environment first, then installing Python, then updating that with the environment definitions. However, this might have (mostly harmless) side effects, such as having Python in the environment even if the user didn't specify that in the YML file. It also leads to longer wait times because there are two (or three, depending on .condarc) solving steps: (empty env), Python installation, environment update.

I think environment creation from a file should be decoupled from the Custom env + Python of your choosing setup. It's less problematic, faster and the code shouldn't be much more complicated.

What do you think? This could be enforced with errors and warnings too. For example if both python-version and environment-file are specified, it should be an error IMO, since the Python version of your choice should be specified in the YML anyway. For matrix builds, this is solved with multiple env files if needed or other conda magics.

@goanpeca
Copy link
Member

goanpeca commented Jun 9, 2020

I think environment creation from a file should be decoupled from the Custom env + Python of your choosing setup. It's less problematic, faster and the code shouldn't be much more complicated.

Yes, and no

What do you think? This could be enforced with errors and warnings too. For example if both python-version and environment-file are specified, it should be an error IMO, since the Python version of your choice should be specified in the YML anyway. For matrix builds, this is solved with multiple env files if needed or other conda magics.

This is precisely the reason why it is split into steps, you could have a single yaml file with python specified, but not the actual version. A lot of projects need to test with different versions and doing this way is what allows for that.

On these points

base gets initialized by default, optionally changing the default python version. This can be the builtin Miniconda or an externally provided one.

I don't understand what you mean by this.

However, this might have (mostly harmless) side effects, such as having Python in the environment even if the user didn't specify that in the YML file.

Not exactly true, as explained above is python is a requirement but several versions will be tested the python in the env yaml should have no version.

It also leads to longer wait times because there are two (or three, depending on .condarc) solving steps: (empty env), Python installation, environment update.

Not really. An empty env has no solver, so the waiting time is practically zero. A python installation takes no time to solve since it will be placed on an empty env and uses well-established dependencies. environment update-> In here the Python version will be used as a constraint for the new environment and that is the reason why python version will not be changed at all.

It is not possible to have a single env create step and be able to provide a python version, since the command that runs that is condo env not conda, so something like:

conda end create -f someenv.yml python=$PYTHON_VERSION will not work.

And yes this is a separate issue and a separate thread and exposed before, the only viable change for current workflows (that use the possibility of changing the python version without hardcoding it on the env.yaml) is to reduce going from 3 steps to just two so that creating the empty env and adding python to it is a single step. In reality, the gain in that is like 1 second, so not really worth the trouble.

@jaimergp
Copy link
Member Author

jaimergp commented Jun 9, 2020

Thanks for the detailed answer!

Is it possible then to have an environment set up without Python? Let's say, we have this file:

name: test
channels:
- conda-forge
- defaults
packages:
- cmake
- clang-dev

@goanpeca
Copy link
Member

goanpeca commented Jun 9, 2020

Is it possible then to have an environment set up without Python? Let's say, we have this file:

Yes, if python-version is not set or if it is not on the yaml file, and it is not a dependency of the listed requirements, there is no reason why python would get installed (unless there is a bug :-p )

See: https://github.com/goanpeca/setup-miniconda/blob/master/src/setup.ts#L759

@jaimergp
Copy link
Member Author

jaimergp commented Jun 9, 2020

Awesome! Thanks for the explanation again, then my main concerns were based on misconceptions :) Apologies for the time taken 😬

@jaimergp
Copy link
Member Author

jaimergp commented Jun 9, 2020

Getting back to this PR, are you comfortable with the mamba-version: "latest" naming? This is what installs mamba without any restrictions (e.g. conda install mamba vs conda install mamba={mambaVersion}). I was thinking whether the wording is confusing, because depending on the packages installed in the existing environment, the installed version might not be strictly the latest. Other alternatives would be:

  • auto
  • default

@goanpeca
Copy link
Member

goanpeca commented Jun 9, 2020

Hmmm good points

Since mamba is a package and conda does not have the notion of latest (miniconda installers do) I would be inclined in having to provide an actual version number. So I am still not 100% on either of the suggestions, since that would hold for python-version: "latest" for instance.

Mamba is an "advanced" feature so you should be explicit about the version you want if you are already on that path.

What do you think @bollwyvl ?

Maybe "*" is an actual thing that condo could use?

conda install mamba=* ?

@jaimergp
Copy link
Member Author

jaimergp commented Jun 9, 2020

You are right, conda happily takes * as a version specifier. No need to use default or auto or anything.

@jaimergp
Copy link
Member Author

jaimergp commented Jun 9, 2020

Seems that they might "look into it" today. Let's see :)

Btw, mamba devs fixed this! mamba-org/mamba#327 They are also using your GH Action ;)

@goanpeca
Copy link
Member

goanpeca commented Jun 9, 2020

Haha awesome and great to hear

@wolfv
Copy link

wolfv commented Jun 9, 2020

I have just released mamba 0.3.2
Once this is green conda-forge/mamba-feedstock#48 you could give it another try :)

@goanpeca
Copy link
Member

goanpeca commented Jun 9, 2020

Awesome! Thanks @wolfv !

@goanpeca
Copy link
Member

goanpeca commented Jun 9, 2020

@jaimergp is everything working as expected? Do you need to make an update to the examples to check for the fix?

@jaimergp
Copy link
Member Author

jaimergp commented Jun 9, 2020

Looks like the new mamba is indeed applied with mamba env update!

I am a bit confused because of how the downloads look, though. In a regular mamba install they look like this, but with mamba env update they look like the regular conda downloads. In a nutshell, looks like solving is done with mamba but not downloading. Is this intentional @wolfv?

@wolfv
Copy link

wolfv commented Jun 10, 2020

HI @jaimergp I think you're right. With mamba env the downloads are still done through conda. I can fix that

@goanpeca
Copy link
Member

Awesome! Thanks for the explanation again, then my main concerns were based on misconceptions :) Apologies for the time taken 😬

Not at all, no need to apologize, thanks for the help @jaimergp

Is this good to go? I can make a release and get this in the wild and see how it goes :-)

@goanpeca
Copy link
Member

Ok merging, will make a release soon :-p

@goanpeca goanpeca merged commit f5af664 into conda-incubator:master Jun 10, 2020
@goanpeca goanpeca mentioned this pull request Jun 10, 2020
@jaimergp
Copy link
Member Author

Thank you so much!

@wolfv
Copy link

wolfv commented Jun 11, 2020

hey, mamba 0.3.3 (it's in the process of being released to conda-forge) has an important fix for an issue that manifested itself in CI environments sometimes.

Just wanted to make sure that you're not stuck on 0.3.2!

Cheers & thanks for your work here!

@goanpeca
Copy link
Member

Thanks again @jaimergp and @wolfv :-)

@goanpeca
Copy link
Member

Ok version 1.6 is out 🥳 !

@jaimergp
Copy link
Member Author

Yayy thanks everybody!!

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.

3 participants