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

Append core requirements to Conda environment file #5956

Merged
merged 4 commits into from Jul 23, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jul 18, 2019

We run 3 steps when a project depends on conda.

  1. create the whole environment based on user's YAML file
  2. run conda install with our own dependencies
  3. run pip install with some of our dependencies that are not
    published on conda repositories.

This commit changes this to make it in just one step (at environment
creation). To do this, it appends our own requirements to the
environment.yml file under dependencies and dependencies.pip
config (see https://docs.conda.io/projects/conda/en/latest/user-guide/tasks/manage-environments.html#create-env-file-manually)

It also shows the resulting environment.yml in the build output.

This behavior is added behind a feature flag so we can test it first.

This allow users to be able to pin their dependencies as they want
without us upgrading them in the second step. Also, this should
improve the build time, since dependencies resolution is done just
once.

Related to

We run 3 steps when a project depends on conda.

1. create the whole environment based on user's YAML file
2. run `conda install` with our own dependencies
3. run `pip install` with some of our dependencies that are not
published on conda repositories.

This commit changes this to make it in just one step (at environment
creation). To do this, it appends our own requirements to the
`environment.yml` file under `dependencies` and `dependencies.pip`
config (see https://docs.conda.io/projects/conda/en/latest/user-guide/tasks/manage-environments.html#create-env-file-manually)

It also shows the resulting `environment.yml` in the build output.

This behavior is added behind a feature flag so we can test it first.

This allow users to be able to pin their dependencies as they want
without us upgrading them in the second step. Also, this should
improve the build time, since dependencies resolution is done just
once.

Related to

* #3829
* #5631
@humitos humitos requested a review from Jul 18, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 19, 2019

I believe we tried to do this in the past and it caused conflicts with version resolution in conda.

@@ -472,8 +543,19 @@ def install_core_requirements(self):
pip_requirements.append('mkdocs')
else:
pip_requirements.append('readthedocs-sphinx-ext')
requirements.extend(['sphinx', 'sphinx_rtd_theme'])
conda_requirements.extend(['sphinx', 'sphinx_rtd_theme'])
Copy link
Member

@ericholscher ericholscher Jul 19, 2019

I believe we explicitly don't add versions here just to confirm that they are installed without specifying a version or having them be upgraded. Has this changed? I don't know how this logic would be upgrading users versions.

Copy link
Member Author

@humitos humitos Jul 19, 2019

Once the environment is created with conda env create if the user pin sphinx==1.6 it will be upgraded (in the current production code) in the step conda install sphinx --even if we don't specify any version.

There was an attempt to add an attribute in conda itself called --satisfied-skip-solve but does not work as we need unfortunately :( . See #5631

@humitos
Copy link
Member Author

@humitos humitos commented Jul 19, 2019

I believe we tried to do this in the past and it caused conflicts with version resolution in conda.

Conda has changed a lot on how they solve the version resolution. I think it worth to give a try to this approach.

All the tests that I've done locally worked properly, and at the moment, is the only working solution that we have.

@humitos humitos requested a review from ericholscher Jul 22, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 22, 2019

All the tests that I've done locally worked properly, and at the moment, is the only working solution that we have.

OK. Though I'm pretty sure this will blow up in some random new way. Is there someone in the conda community we can ask if this is the right approach?

@humitos
Copy link
Member Author

@humitos humitos commented Jul 22, 2019

Though I'm pretty sure this will blow up in some random new way.

This is probably true. Although, this will give us some context on where it fails and we can research how to fix it from there if we want.

I'm not too worry about this because it's behind a feature flag, though. So, we can enable on request and if it does not work, we just disable it.

Is there someone in the conda community we can ask if this is the right approach?

I don't personally known anyone. There were already some attempts to add this feature on the conda community, but there is no solution yet. As I mention, --satisfied-skip-solve was one attempt, but it only works when all the dependencies are satisfied and it does not work for our use case. See conda/conda#6845 (comment)

As far as I know, unfortunately, what we need it's not currently possible on conda.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 22, 2019

If this is a better approach, I'd like to remove the feature flag eventually and just ship it. I don't fully understand it though :/

@humitos
Copy link
Member Author

@humitos humitos commented Jul 23, 2019

@ericholscher this is a better approach, yes. It solves a problem that we have for what there is not current workaround. conda does not provide a "correct way of doing it" as far as I know.

I personally prefer to ship this under a feature flag, though. Why? Because it's easier to start testing this out and be able to go one step back if we found that it does not work for some cases that I may not considered in my tests. Then, after some time we can remove the flag and make it the default behavior, which won't involve a lot of work to remove the flag.

@ericholscher ericholscher merged commit c6bacb0 into master Jul 23, 2019
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/append-reqs-conda-environment branch Jul 23, 2019
@keewis keewis mentioned this pull request Aug 9, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants