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

add mambaforge installer #22

Merged
merged 3 commits into from
Jan 17, 2022
Merged

add mambaforge installer #22

merged 3 commits into from
Jan 17, 2022

Conversation

jameslamb
Copy link
Collaborator

Contributes to microsoft/LightGBM#4948.

This PR proposes adding support for using mamba instead of conda to install conda packages.

How I tested this

cd dockers/ubuntu-14.04
docker build \
    -t lightgbm/vsts-agent:local \
    -f ./Dockerfile \
    .

Notes for Reviewers

I'd like to push these changes to the dev branch, and test pulling in the new image lightgbm/vsts-agent:ubuntu-14.04-dev pushed to Docker Hub on microsoft/LightGBM#4953.

Per microsoft/LightGBM#4948 (comment), I don't have permissions to create branches here.

@StrikerRUS could you create a dev branch in this repo so I can target this PR at it?

@guolinke if you see this, could you please give me "write" permissions on this repo so I can add branches myself in the future?

@@ -0,0 +1,2 @@
*
!start_azure.sh
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

adding a .dockerignore isn't strictly necessary to add support for mamba, but I think it's generally a good idea and thought now was a good time, given that we're about to test this new image anyway.

For details on this file, you can see https://codefresh.io/docker-tutorial/not-ignore-dockerignore-2/.

Benefits:

  • smaller build context = faster builds
  • reduced risk of accidentally including unnecessary files
    • smaller images
    • less risk of including secret / sensitive information

@StrikerRUS
Copy link
Collaborator

could you create a dev branch in this repo so I can target this PR at it?

Done!

@jameslamb jameslamb changed the base branch from master to dev January 17, 2022 17:49
@jameslamb
Copy link
Collaborator Author

Thank you very much! I just updated this to dev, so I think it's ready for review. My proposal is the following:

  1. merge this change to dev, so that a new image lightgbm/vsts-agent:ubuntu-14.04-dev will be pushed to Docker Hub
  2. in [ci] use conda-forge in Linux and macOS CI jobs microsoft/LightGBM#4953, change .vsts-ci.yml to reference that dev image
  3. once it's approved, merge [ci] use conda-forge in Linux and macOS CI jobs microsoft/LightGBM#4953
  4. come back here and merge dev into master
  5. follow-up PR in LightGBM to change the image reference in .vsts-ci.yml to the non-dev tag (lightgbm/vsts-agent:ubuntu-14.04)

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thank you! Agree with your plan. Please consider checking my suggestion about paths and filenames renaming below.

README.md Outdated Show resolved Hide resolved
@@ -84,11 +84,10 @@ RUN curl -sLk https://sourceforge.net/projects/swig/files/swig/swig-4.0.2/swig-4
&& rm -rf swig-4.0.2

# Install Miniconda
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't make a suggestion for all these lines, so describing my suggestion just in words.

Let's be explicit here and don't call Mambaforge installation as "Miniconda"/"conda".
Please update lines 86-95 making it clear we are working with mamba[forge]. The only public entrypoint here is CONDA env variable, so don't change its name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, works for me!

updated in f75b7a4 and tested that building the image still works

@jameslamb jameslamb requested a review from StrikerRUS January 17, 2022 19:13
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM!

@StrikerRUS StrikerRUS merged commit 0c041e7 into guolinke:dev Jan 17, 2022
@jameslamb jameslamb deleted the mamba branch January 17, 2022 19:53
@StrikerRUS
Copy link
Collaborator

dev has been published:

image

@guolinke
Copy link
Owner

Thank you @jameslamb , I will give you the write permission.

@jameslamb
Copy link
Collaborator Author

thanks very much @guolinke !

StrikerRUS added a commit that referenced this pull request Feb 11, 2022
* Add mambaforge installer (#22)

* add mambaforge installer

* Update README.md

Co-authored-by: Nikita Titov <[email protected]>

* change paths to mamba

Co-authored-by: Nikita Titov <[email protected]>

* Replace mambaforge with miniforge (#24)

* Replace mambaforge with miniforge

* Update Dockerfile

Co-authored-by: James Lamb <[email protected]>
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

Successfully merging this pull request may close these issues.

3 participants