Skip to content

Respect conda channels in mulled docker container building #13755

Merged
mvdbeek merged 3 commits intogalaxyproject:devfrom
bernt-matthias:fix_mulled_conda
Apr 22, 2022
Merged

Respect conda channels in mulled docker container building #13755
mvdbeek merged 3 commits intogalaxyproject:devfrom
bernt-matthias:fix_mulled_conda

Conversation

@bernt-matthias
Copy link
Contributor

@bernt-matthias bernt-matthias commented Apr 19, 2022

DEFAULT_MULLED_CONDA_CHANNELS is now respected when building mulled docker containers.

In addition the change to CondaInDockerContext allows to use local packages.

Ideally planemo users would be able to set the channels via a command line option, but I could not find a way. Also this should be done for singularity container building?

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

@github-actions github-actions bot added this to the 22.05 milestone Apr 19, 2022
@bernt-matthias bernt-matthias force-pushed the fix_mulled_conda branch 2 times, most recently from 74d1764 to 682b67e Compare April 19, 2022 20:12
@mvdbeek mvdbeek changed the title make mulled docker container building respect conda channels Respect conda channels in mulled docker container building Apr 20, 2022
for channel in ensure_channels:
if channel.startswith("file://"):
bind_path = channel[7:]
binds.extend(["-v", f"/{bind_path}:/{bind_path}"])
Copy link
Member

Choose a reason for hiding this comment

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

How come you're prefixing the bind paths with /, and wouldn't you want to to hardcode the bind destination to what the container expects ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How come you're prefixing the bind paths with /,

I took inspiration from here:

binds.append(f"/{bind_path}:/{bind_path}")

But you are right the / is redundant. I will remove it at both places.

and wouldn't you want to to hardcode the bind destination to what the container expects ?

This should be what the container expects, i.e. the same value is given to the -c / --channel argument of conda search.

Copy link
Member

Choose a reason for hiding this comment

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

I see, that makes sense.

DEFAULT_CHANNELS = os.environ["DEFAULT_MULLED_CONDA_CHANNELS"].split(",")
else:
DEFAULT_CHANNELS = ["conda-forge", "bioconda"]
DEFAULT_CHANNELS = default_mulled_conda_channels_from_env() or ["conda-forge", "bioconda"]
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to add this to the command line parser as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you mean this?

Copy link
Member

Choose a reason for hiding this comment

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

There's an argparse section below, you'd want to be able to set the channels there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change should only alter the default of the --channels argument, users can still overwrite the value.

bernt-matthias added a commit to bernt-matthias/planemo that referenced this pull request Apr 20, 2022
to the value given by `--conda_channels`

xref galaxyproject/galaxy#13755
bernt-matthias added a commit to bernt-matthias/planemo that referenced this pull request Apr 20, 2022
to the value given by `--conda_channels`

xref galaxyproject/galaxy#13755
@bernt-matthias
Copy link
Contributor Author

bernt-matthias commented Apr 20, 2022

Maybe galaxyproject/planemo@a3cd29a (added here galaxyproject/planemo#1227) is even a better solution?

Still, it might be good to respect the env variable and certainly it's necessary to set the binds correctly.

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Looks good, you can fix the linting with make format (or by installing pre-commit and copying .pre-commit-config.yaml.sample to .pre-commit-config.yaml).

bernt-matthias added a commit to bernt-matthias/planemo that referenced this pull request Apr 20, 2022
to the value given by `--conda_channels`

xref galaxyproject/galaxy#13755
DEFAULT_MULLED_CONDA_CHANNELS is now respected when building
mulled docker containers.

In addition the change to CondaInDockerContext allows to use
local packages.

Ideally planemo users would be able to set the channels via
a command line option, but I could not find a way.
@mvdbeek mvdbeek merged commit ed5dd14 into galaxyproject:dev Apr 22, 2022
@bernt-matthias bernt-matthias deleted the fix_mulled_conda branch April 22, 2022 11:44
bernt-matthias added a commit to bernt-matthias/galaxy that referenced this pull request May 18, 2022
respecting conda channels on disk: follow up on galaxyproject#13755

- CondaInDockerContext needs the list of channels and not the joined str
- get_file_from_recipe should use local data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants