ENH: Activate/deactivate conda environment for build duration

[jakirkham]

TL;DR: Tries to simply activate the conda environment (for a conda build only) and deactivate after the build ends, but before sending out notifications.

Detail: This takes advantage of the fact that the builder will read information from the os.environ to use for its base environment. As a result, simply seed os.environ with what conda needs to recognize the environment. Simply strip this information out afterwards. Note that PATH had to be used here as the builder reads it instead of looking at sys.path (unless I have missed something).

Todo:

• Activate the conda environment
• Deactivate the conda environment
• Test conda environment is active

Related:

• Original conda support - Add support for Conda as a build environment #1849
• Absorbed style fix - STY: Fix PEP8 - E712 linting error (fixes tests) #1920
• Conda support - Support conda for builds #857

Attn: @ericholscher

[ericholscher]

So this isn't the correct approach, because we're running the doc builders inside of an environment (Docker in production) -- so this is setting the environment on the build machine, not inside the docker container.

I need to look a bit more at how we're doing environment setup, but @agjohnson might be able to talk more about it.

[jakirkham]

Alright, yeah, I skimmed the code and this seemed ok initially, but if this is happening outside a VM or docker machine that is another question. At what point is the docker machine created relative to the conda installation and relative to the documentation building?

[ericholscher]

https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/projects/tasks.py#L110-L143 is the main logic. I believe we don't currently have a way to pass the proper environment into docker currently, it would likely need to be passed into the container creation here: https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/doc_builder/environments.py#L600-L601

[ericholscher]

Think we need to add an environment argument to the BuildEnvironment, and make sure it gets passed in to the create_container call, and then we can set it when we create it in the tasks.py.

[jakirkham]

The docker image you are using already has conda installed in some form, right? Also, is the conda env create step is happening in a container? I just want to make sure we don't activate the environment too early. Maybe we can just pass something like a USE_CONDA environment variable to the docker container so it can construct and set the environment variables internally.

[ericholscher]

Yep, conda is installed, and all commands are run in the container. I don't want to put that logic in the container, the goal is for the containers to be quite simple and just an image, and the code will have all the logic it needs to be able to execute.

[jakirkham]

What if we share a file of environment variables that can be sourced right before building the docs? Would that work for you? That should avoid having conda specific logic enter the container and could be used anytime you would want to add these environment variables.

In the worst case, we could try passing the environment variables as you have proposed. I don't expect any bad behavior from conda as long as we are explicit about which environment we want by using -n. It is just a bit worrisome as we are effectively activating an environment before it is created.

[jakirkham]

This is happening in the container, correct? ( https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/projects/tasks.py#L232-L246 ) Maybe we could just add an activate method that gets called at the end?

[jakirkham]

So, I think I may be confused. Is everything proceeding through one docker run command or do some things go through docker exec? If you are using docker exec, which things use it?

[ericholscher]

We run it all with docker exec: https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/doc_builder/environments.py#L198-L203

[ericholscher]

The main issue here is that Docker only allows environment creation on container creation, not per command. Historically we shelled out to commands with the normal python subprocess module, which does support this. We've hacked per-command environment stuff into the Docker system by passing them as a prefix to the command (eg. PATH=foo:bar command) -- but we should change to just establishing all environment settings at the environment (BuildEnvironment) creation.

There will be a bit of refactoring required because the BuildEnvironment is currently created before the config is read, so we'll need to move the config reading up (so we know if conda is activated), and do appropriate exception handling of the config reading there. I can work on this in the next couple days if this is too much to ask of you, but feel free to take a swing at this if you want :)

[jakirkham]

The main issue here is that Docker only allows environment creation on container creation, not per command.

Right, I was going to write something about this. Basically, I don't think this is a problem per se. We can just set the environment variables at container creation as it looks like those variables are predetermined by things like project name and such so we should have some way to always know them. This also makes sure all exec'd containers have an active conda environment (or possibly virtual environment). We can try that as is. However, we would want to keep on our radar that we have activated an environment before it exists (at install time) and should figure out how to get rid of that. Possibly they can be unset, but that sounds tricky.

I can work on this in the next couple days if this is too much to ask of you, but feel free to take a swing at this if you want :)

That's fine. Yeah, my understanding of the code is not nearly as good as yours so this may be less than productive. I just saw people asking for the last 1% and thought it shouldn't be that bad, but the last 1% is always the hardest. :)

[jakirkham]

Though I hope this environment stuff gives you some insight into my perspective. This is why, with docker, I just dump everything in the default environment and move on.

[ericholscher]

#1924 is my work to refactor how this works a bit more so it makes sense.

[agjohnson]

Closing this, I believe #1924 supersedes this work.

@jakirkham thanks a ton for taking a swing at this! 👍

[jakirkham]

I was just about to do that. Thanks @agjohnson. Sorry this didn't go far enough.