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

perf: update to latest miniforge, since mambaforge is deprecated and miniforge nowadays contains both mamba and conda #200

Merged
merged 5 commits into from
Oct 16, 2024

Conversation

johanneskoester
Copy link
Contributor

No description provided.

…miniforge nowadays contains both mamba and conda
@mbargull
Copy link
Contributor

mbargull commented Oct 8, 2024

It seems like the task runs the installer with -u:

- /tmp/mambaforge.sh
- -b
- -u
- -p
- "{{ mamba_home }}"

IIRC, the "update" functionality of (Conda-)constructor-created installers is not very sophisticated and does not uninstall previously installed packages, i.e., the environment is not very "clean" afterwards.

With an update from Miniforge <24.5 we'd, e.g., end up with 2 different python installs since we updated python to 3.12 in Miniforge 24.5.0.

This is at least how I remember things (haven't looked into constructor for a long time).
So, you definitely want to make sure to test this out on one/a couple of nodes before rolling it out generally.

If it indeed works as pictured above, we may want to change that task's step to always do a clean install via something along the lines of

if [ -e '{{ mamba_home }}' ]; then
  mv '{{ mamba_home }}' '{{ mamba_home }}'.old
  if ! { /tmp/mambaforge.sh -bp '{{ mamba_home }}' && '{{ mamba_home }}/condabin/conda' --version | grep -F '{{ mambaforge_conda_version }}'; }; then
    if [ -e '{{ mamba_home }}' ]; then
        mv '{{ mamba_home }}' '{{ mamba_home }}'.deleteme
        rm -rf '{{ mamba_home }}'.deleteme
    fi
    mv '{{ mamba_home }}'.old '{{ mamba_home }}'
    exit 1
  else
    rm -rf '{{ mamba_home }}'.old
  fi
else
  /tmp/mambaforge.sh -bp '{{ mamba_home }}' 
fi

(N.B.: The above is untested/just quickly written down and might not be without fault (e.g., using trap would be cleaner).)

If the installed version differs from the target version, the existing
installation directory is wiped before proceeding.
@enasca
Copy link
Member

enasca commented Oct 8, 2024

Following Marcel's comment, I've added a commit which deletes the current installation if it's different from the version being installed. It doesn't implement the revert logic from the suggested script, but it takes into account that Ansible stops on failure, leaving cleanup to the user in case of a failure in the middle of the run. I think it's enough for normal operation, but it can be expanded if needed.

@enasca
Copy link
Member

enasca commented Oct 8, 2024

I've replied to the comments but I'm afraid I can't make changes right now. I can come back to this later tonight. In the meantime @mbargull feel free to make any changes without my approval.

* If no existing installation is detected, install.
* If an existing installation is detected and the version is the same
  as the target, skip the installation.
* If an existing installation is detected and the version differs
  from the target, wipe and then install.
@enasca
Copy link
Member

enasca commented Oct 9, 2024

@mbargull I've added a commit taking your feedback into account. From your table above, I'm handling the "matches" and "differs" cases explicitly. The "unknown" case is handled implicitly by the installer given that it fails if the installation directory already exists.

@enasca enasca requested a review from mbargull October 9, 2024 11:16
@enasca
Copy link
Member

enasca commented Oct 9, 2024

If the logic looks ok to you, Johannes can go ahead and test it.

@mbargull
Copy link
Contributor

This has been deployed to almost all nodes.
Thanks everyone!

@mbargull mbargull merged commit 6a81156 into main Oct 16, 2024
1 check passed
@mbargull mbargull deleted the perf/miniforge branch October 16, 2024 12:44
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