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

Added mamba support #22

Merged
merged 10 commits into from
Nov 11, 2021
Merged

Added mamba support #22

merged 10 commits into from
Nov 11, 2021

Conversation

GabrielDougherty
Copy link
Contributor

@GabrielDougherty GabrielDougherty commented Nov 8, 2021

This implements the ability to use mamba, in accordance with our design discussion.

Design considerations

I commented out the call to _update_conda. I found it confusing that we specify the version of conda in the build file, but then ask conda to update itself. This seems to go against the "Bazel way" (if I can call it that) of specifying the exact versions for every input to the build process. Can I remove the _update_conda function?

Testing

I manually tested this by running the test in tests/ with the following modifications:

  • conda then mamba w/o clean -> success
  • mamba then conda w/o clean -> success
  • conda then mamaba w/ clean -> success
  • mamba then conda w/ clean -> success

To elaborate on what these bullet points mean, for the first bullet point I would do the following:

cd tests/
bazel clean --expunge
# edit WORKSPACE to set use_mamba = True and clean = False
nvim WORKSPACE
bazel test test
# edit WORKSPACE to set use_mamba = False and clean = False
nvim WORKSPACE
bazel test test

I would repeat this process, manually changing the value of use_mamba and clean for each bullet point. In between each bullet point, I run bazel clean --expunge. Now I would like to make an automated version of these tests, but the only idea I have for that is to make a bunch of directories under tests/ with their own WORKSPACE and BUILD files and change the use_mamba and clean flags to the matrix of combinations I specified above. Then there would be a test script that goes to each directory and runs bazel test //.... The problem is that that approach would duplicate a lot of files, just to change two flags. Do you have any better idea for structuring the tests?

TODO

Before merging this PR, I would like to:

  • Add documentation for the new flags
  • Add tests

example/WORKSPACE Outdated Show resolved Hide resolved
conda.bzl Outdated Show resolved Hide resolved
@spietras spietras added the feature New feature or request label Nov 8, 2021
@spietras spietras linked an issue Nov 8, 2021 that may be closed by this pull request
Closed
example/WORKSPACE Outdated Show resolved Hide resolved
@spietras spietras mentioned this pull request Nov 9, 2021
conda.bzl Outdated Show resolved Hide resolved
conda.bzl Outdated Show resolved Hide resolved
conda.bzl Outdated Show resolved Hide resolved
conda.bzl Outdated Show resolved Hide resolved
conda.bzl Show resolved Hide resolved
env.bzl Outdated Show resolved Hide resolved
env.bzl Outdated Show resolved Hide resolved
env.bzl Outdated Show resolved Hide resolved
env.bzl Outdated Show resolved Hide resolved
Copy link
Owner

@spietras spietras left a comment

Choose a reason for hiding this comment

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

Great job so far! Added more suggestions, this time for rather smaller things

env.bzl Outdated Show resolved Hide resolved
example/WORKSPACE Outdated Show resolved Hide resolved
env.bzl Outdated Show resolved Hide resolved
env.bzl Outdated Show resolved Hide resolved
docs/docs/usage/example.md Outdated Show resolved Hide resolved
@spietras
Copy link
Owner

Please rebase after #21

docs/docs/usage/api.md Outdated Show resolved Hide resolved
env.bzl Outdated Show resolved Hide resolved
Copy link
Owner

@spietras spietras left a comment

Choose a reason for hiding this comment

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

Wonderful, we are almost there, only final touches now. Ping me when you think it's ready to merge.

@GabrielDougherty
Copy link
Contributor Author

@spietras ready to merge, thanks for your prompt code reviews!

Copy link
Owner

@spietras spietras left a comment

Choose a reason for hiding this comment

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

Awesome, great job 💯

Thanks for contributing to rules_conda 💚

@spietras spietras changed the title Add option to use mamba (#2) Added mamba support Nov 11, 2021
@spietras spietras merged commit a79182d into spietras:main Nov 11, 2021
@spietras spietras mentioned this pull request Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mamba
3 participants