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 mamba and conda-libmamba-solver to Miniforge (Mambaforge is now identical) #277

Merged
merged 7 commits into from
Aug 20, 2023

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Mar 21, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@isuruf isuruf requested a review from a team as a code owner March 21, 2022 19:37
@isuruf
Copy link
Member Author

isuruf commented Mar 21, 2022

cc @jaimergp, @jezdez, @wolfv

scripts/build.sh Outdated
@@ -22,7 +22,7 @@ fi
if [[ "${TARGET_PLATFORM}" == win-* ]]; then
conda install -y "nsis=3.01" -c conda-forge --override-channels
fi
pip install git+git://github.com/chrisburr/constructor@64ebd6d34f0f18684c76c0bebcfab41c38d55083#egg=constructor --force --no-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably why the last tag was failing.

Copy link
Member

Choose a reason for hiding this comment

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

If we merge conda-forge/constructor-feedstock#61 we don't need this pip install override, right?

@@ -28,6 +28,8 @@ specs:

{% if name.startswith("Mambaforge") %}
- mamba 0.22.1
{% else %}
- conda-libmamba-solver 22.3.0
Copy link
Member

Choose a reason for hiding this comment

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

This will pull libmambapy plus its deps so I wonder if just the presence of mamba as a command is enough to justify the existence of Mambaforge. Like, if we merge, Miniforge and Mambaforge are almost the same. IIRC, Mambaforge was provided separately because it shipped more deps.

scripts/build.sh Outdated
@@ -22,7 +22,7 @@ fi
if [[ "${TARGET_PLATFORM}" == win-* ]]; then
conda install -y "nsis=3.01" -c conda-forge --override-channels
fi
pip install git+git://github.com/chrisburr/constructor@64ebd6d34f0f18684c76c0bebcfab41c38d55083#egg=constructor --force --no-deps
pip install git+https://github.com/conda/constructor@3.3.1#egg=constructor --force --no-deps
Copy link
Member

Choose a reason for hiding this comment

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

conda-forge/constructor-feedstock#61 was merged, so we can remove this in a couple hours.

Suggested change
pip install git+https://github.com/conda/constructor@3.3.1#egg=constructor --force --no-deps

Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

A few notes on this:

Some context first: The new solver integration was released as a separate package as a means for users to "opt-in" into the experimental phase of the engineering effort, basically for us to be able to collect feedback from real world environments. The goal is to gain the confidence that our integration layer with libmamba indeed works as closely as possible to conda's default solver. It's not just a straight port of libmamba but does extra steps to be closer to what users expect from conda. The extra installation step was a design decision to make sure we're not accidentally cargo-cult the experimental solver into "production environments" such as conda-forge.

The miniconda 4.12.0 installer will not automatically install conda-libmamba-solver. It largely depends on the real world user feedback, but I'm expecting it to be added in the next release of miniconda at the end of Q2, start of Q3 time frame instead, when we hope to have finalized the experiment.

For conda-forge I would strongly suggest to wait for at least one or two bugfix releases before adding conda-libmamba-solver to miniforge, simply because it would make it easier for Jaime, Wolf and me to ship breaking changes in conda-libmamba-solver when they become necessary following user feedback.

Regarding mambaforge and how conda-libmamba-solver fits into this, I want to encourage us to think about this before we make decisions (like merging this PR here) before we regret it afterwards. mambaforge/miniforge and mamba are reasonable alternatives to miniconda and conda and I'd rather gain more experience with conda-libmamba-solver before we change the installers that a lot of users use.

The alternative to this PR right now is: conda install conda-libmamba-solver.

Could we maybe put this PR on hold for a bit?

@jezdez
Copy link
Member

jezdez commented Mar 23, 2022

I take the merging of #280 as a yes :) Thanks @isuruf!

@jakirkham
Copy link
Member

IIUC sounds like we are now good to include conda-libmamba-solver ( #284 (comment) )

Would we like to revitalize this PR or start a new one?

@jaimergp
Copy link
Member

@jakirkham - I added a comment on 284 to figure out how we want to move forward! Thanks for the reminder!

@jaimergp
Copy link
Member

Updated PR as discussed in #284.

Note that I am not copying the installers yet. The name of the installer, as taken by constructor will also determine the default install location, so some people might be expecting Mambaforge there.

I suggest we advertise this in the release notes and maybe in other channels (blog post, Twitter, Element, etc), and then change it to a copy in the next release or two.

@jaimergp jaimergp changed the title update to 4.12.0 and add conda-libmamba-solver Add mamba and conda-libmamba-solver to Miniforge (Mambaforge is now identical) Aug 10, 2023
@jaimergp
Copy link
Member

This is ready for review @conda-forge/miniforge. Thanks!

@jakirkham
Copy link
Member

FWIW LGTM. Thanks Jaime! 🙏

@jakirkham
Copy link
Member

@hmaarrfk could you please take a look?

@jakirkham
Copy link
Member

Friendly nudge @hmaarrfk 😉

@hmaarrfk
Copy link
Contributor

Sorry, trying to enable boa compatibility tests on miniforge too.

@hmaarrfk hmaarrfk merged commit 7b6dc5f into conda-forge:main Aug 20, 2023
24 checks passed
@jakirkham
Copy link
Member

Thanks everyone! 🙏

@hmaarrfk
Copy link
Contributor

Release is now out. Give it a try!

@jaimergp
Copy link
Member

Thanks Mark! Looks like it's gone live in macOS and Windows and no one has complained in Element yet :D I have sent a PR to change the base Miniforge install in our Linux Docker images at conda-forge/docker-images#239

I also updated the release description a bit to add a couple extra details. Hope that's ok!

@jakirkham
Copy link
Member

Noticed the tag is 23.3.1. Am wondering if it should be 23.8.0

Comment on lines 33 to +35
- conda {{ version.split("-")[0] }}
- conda-libmamba-solver {{ conda_libmamba_solver_version }}
- mamba {{ mamba_version }}
Copy link
Member

Choose a reason for hiding this comment

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

Ah nvm looks like the version is tied to how Conda & friends are versioned. Submitted PR ( #480 ) to address this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants