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

Don't mess with PATH #61

Closed
maresb opened this issue Dec 3, 2021 · 8 comments
Closed

Don't mess with PATH #61

maresb opened this issue Dec 3, 2021 · 8 comments

Comments

@maresb
Copy link
Collaborator

maresb commented Dec 3, 2021

I'd prefer to get rid of this line which prepends $MAMBA_ROOT_PREFIX/bin to PATH.

Here are a few reasons why I think that line is a bad idea:

  • Suppose MAMBA_ROOT_PREFIX is changed by the user. Then the default is still on the path, but the new value is not.
  • It's Mamba's job to modify the environment. (Mamba can always be triggered by SHELL or ENTRYPOINT, see Allow activation within Dockerfile via SHELL #60.)
  • In my view it's best to make minimal modifications to the base image for maximal portability and minimal surprise.
  • I can't think of any use cases for it.

Does this make sense? Is there some use case I'm missing?

@wholtz
Copy link
Member

wholtz commented Dec 3, 2021

Hi @maresb .
For this sake of this discussion, I'm working off your PR #60. If the PATH modification line is removed, then 3 tests fail.

Two of these failing tests disable the entrypoint script and I'm okay with that behavior, as people shouldn't be overriding the entrypoint and expect everything to work. Those two tests are primarily in there to ensure the behavior of the image doesn't change without the developers being aware of the implications.

The third test that fails is this one:

FROM micromamba:test
RUN micromamba install -y -n base -c conda-forge \
python=3.9.1 && \
micromamba clean --all --yes
RUN ["python", "-c", "import os; os.system('touch foobar')"]

Because that final RUN is in the "exec" form, the SHELL is not used to evaluate the RUN command. These details of how RUN and SHELL interact is likely not clear to many users. The modification to PATH was done to prevent users from getting hung up on this detail. I'm open to removing this test, but I think the documentation needs to make clear that the shell form of RUN is preferred over the exec form when executing a programmed installed to a conda environment.

@maresb
Copy link
Collaborator Author

maresb commented Dec 3, 2021

@wholtz, thanks for the detailed explanation!

In this third test, in the context of #60, could you add the line

ARG MAMBA_DOCKERFILE_ACTIVATE=1

just before the last line? I would expect that to fix this test. At least for me, this behavior is more intuitive: I don't expect anything from the environment to become available until after I have activated it.

@wholtz
Copy link
Member

wholtz commented Dec 3, 2021

No, adding ARG MAMBA_DOCKERFILE_ACTIVATE=1 does not make the 3rd failing test pass. If you read the Dockerfile docs, this is the expected behavior for the exec form of RUN.

@maresb
Copy link
Collaborator Author

maresb commented Dec 3, 2021

Ahhhhhhhhhh..... I see.

These details of how RUN and SHELL interact is likely not clear to many users.

👍

Hmm, that does seem to be a significant flaw in my proposal which I had not realized before.

@wholtz
Copy link
Member

wholtz commented Dec 3, 2021

I'm happy with PR #60 and I agree with you overall about modifying the PATH. But I'm going to re-write and expand the docs before I remove the PATH modification.

@maresb
Copy link
Collaborator Author

maresb commented Dec 3, 2021

I think the documentation needs to make clear that the shell form of RUN is preferred over the exec form when executing a programmed installed to a conda environment.

This is a really good point. I must confess that I don't understand very well the magic that happens when a Conda binary is executed outside of an activated environment. (I know in Linux there's often some clever shebang stuff.) Do you know to what extent it's equivalent to activating before running?

I suppose the way I'd fix the test case would be

RUN ["/opt/conda/bin/python", "-c", "import os; os.system('touch foobar')"] 

I'm very glad you like #60, thanks! 😃

@wholtz
Copy link
Member

wholtz commented Dec 9, 2021

Done in #63 and #64.

@wholtz wholtz closed this as completed Dec 9, 2021
@maresb
Copy link
Collaborator Author

maresb commented Dec 9, 2021

This looks great!!! Thanks so much @wholtz!

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

No branches or pull requests

2 participants