CSCfi / hpc-container-wrapper

Tool to wrap installations into a container designed for use on HPC systems
MIT License
27 stars 8 forks source link

Replace miniconda by miniforge to avoid licensing issues #25

Closed xavierabellan closed 1 month ago

xavierabellan commented 3 months ago

Using miniconda from Anaconda implies accepting its license. This may not be obvious to the user using this tool since it is done automatically, and they may be indadvertedly in breach of such license.

Replacing miniconda by miniforge is a safer alternative, addressing this licensing concern while virtually keeping all the functionality intact.

Nortamo commented 3 months ago

Overall looks good, I'll have to think if it's better to nuke miniconda all together or then default to miniforge and have an option for using miniconda while removing the -b as to show the license to the user

Nortamo commented 1 month ago

Looks good, I'll leave it to later to possibly re-implement the support for using miniconda. Before merging:

Whats the specific reason for the empty condarc, as that now breaks the functionality that we disable $HOME/.conda/pkgs (if the file is empty but exists the user pkgs setting will be inserted, which is not what we want)

xavierabellan commented 1 month ago

In conda 24.7.1 of conda (at least the one from miniforge, I have not tried with miniconda) something has changed, and the installation itself fails if the pkgs directory defined in the .condarc does not exist. Quick workaround was then to just remove that setting from the .condarc.

I believe you cannot pre-create the directory because then the installation also fails since it detects that the installation directory exists and it is not empty. How would you propose to address it to maintain the original behaviour?

Nortamo commented 1 month ago

I tested, and we can solve it with an environment variable:

 # By default we want to disable the user condarc as that
 # might interfere with the installation
 if [[ ! ${CW_ENABLE_CONDARC+defined} ]]; then
-    echo "" > $PWD/_inst_dir/condarc_override
-    SINGULARITY_BIND="$SINGULARITY_BIND,$PWD/_inst_dir/condarc_override:$(readlink -f $HOME/.condarc)"
+    export CONDA_PKGS_DIRS=$CW_INSTALLATION_PATH/miniforge/pkgs
 fi
 export SINGULARITY_BIND

So if you do that change / if it's ok for me to push to your PR, we can merge after the fix

xavierabellan commented 1 month ago

Thanks, that's a very good idea. I have committed and pushed the proposed change.

xavierabellan commented 1 month ago

I also realised that the environments created within the container are leaked into the user's ~/.conda/environments.txt outside the container. If conda is used outside, all those environments are listed but are, of course, not there.

I would propose to add the following dummy overlay to address that problem using SINGULARITY_BIND:

$ git diff
diff --git a/create_inst.sh b/create_inst.sh
index 47a5ab2..a5ccc83 100755
--- a/create_inst.sh
+++ b/create_inst.sh
@@ -42,6 +42,8 @@ SINGULARITY_BIND="$SINGULARITY_BIND,/tmp"
 if [[ ! ${CW_ENABLE_CONDARC+defined} ]]; then
     export CONDA_PKGS_DIRS=$CW_INSTALLATION_PATH/miniforge/pkgs
 fi
+mkdir -p $CW_BUILD_TMPDIR/.conda_override
+SINGULARITY_BIND="$SINGULARITY_BIND,$CW_BUILD_TMPDIR/.conda_override:$(readlink -f $HOME/.conda)"
 export SINGULARITY_BIND
 echo "export install_root=$CW_INSTALLATION_PATH" >> _extra_envs.sh
 echo "export install_root=$CW_INSTALLATION_PATH" >> _vars.sh

If you agree, I can also push this change (or open a separate pull request for it)

Nortamo commented 1 month ago

I also realised that the environments created within the container are leaked into the user's ~/.conda/environments.txt outside the container. If conda is used outside, all those environments are listed but are, of course, not there.

I would propose to add the following dummy overlay to address that problem using SINGULARITY_BIND:

$ git diff
diff --git a/create_inst.sh b/create_inst.sh
index 47a5ab2..a5ccc83 100755
--- a/create_inst.sh
+++ b/create_inst.sh
@@ -42,6 +42,8 @@ SINGULARITY_BIND="$SINGULARITY_BIND,/tmp"
 if [[ ! ${CW_ENABLE_CONDARC+defined} ]]; then
     export CONDA_PKGS_DIRS=$CW_INSTALLATION_PATH/miniforge/pkgs
 fi
+mkdir -p $CW_BUILD_TMPDIR/.conda_override
+SINGULARITY_BIND="$SINGULARITY_BIND,$CW_BUILD_TMPDIR/.conda_override:$(readlink -f $HOME/.conda)"
 export SINGULARITY_BIND
 echo "export install_root=$CW_INSTALLATION_PATH" >> _extra_envs.sh
 echo "export install_root=$CW_INSTALLATION_PATH" >> _vars.sh

If you agree, I can also push this change (or open a separate pull request for it)

It can be part of this PR, so you can push the changes here.

xavierabellan commented 1 month ago

Done thanks!