-
Notifications
You must be signed in to change notification settings - Fork 48
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
support for multiple CVMFS repositories in eessi_container.sh
#618
support for multiple CVMFS repositories in eessi_container.sh
#618
Conversation
Instance
|
Instance
|
eessi_container.sh
eessi_container.sh
Tried the container with @trz42's instructions from slack # Step 1: checkout repo (and PR)
git clone https://github.com/EESSI/software-layer
cd software-layer
git fetch origin pull/618/head:PR618
git checkout PR618
# Step 2: launch interactive job, for example
srun --time=60 --pty bash
# Step 3: launch container with read access to software.eessi.io and write access to dev.eessi.io
./eessi_container.sh -r software.eessi.io -r dev.eessi.io,access=rw
# Step 4: init EESSI and build environment
source /cvmfs/software.eessi.io/versions/2023.06/init/bash
export EESSI_PROJECT_INSTALL=/cvmfs/dev.eessi.io/dev_test/versions/2023.06/software/linux/${EESSI_SOFTWARE_SUBDIR}
module load EESSI-extend/2023.06-easybuild
# Step 5: build some software (e.g., recently merged imbalanced-learn-0.12.3-gfbf-2023a.eb)
eb --robot --from-pr 20848 imbalanced-learn-0.12.3-gfbf-2023a.eb
# Step 6: show, load and test module
module show imbalanced-learn/0.12.3-gfbf-2023a
module load imbalanced-learn/0.12.3-gfbf-2023a
python -c "import imblearn" Works out of the box! The only thing extra I had to do was: Otherwise I get:
Once I first created the directory then I was able to load I might be missing a step, but if not perhaps there is a way to take care of creating the necessary subdirectories automatically (i.e., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting my message from slack:
I can't see anything wrong in the code, but it might be a good idea to have someone who is more familiar with setting up CVMFS repositories than I am to also take a look since some changes may have implications that I'm not aware of. Beyond that, looks good to me, and works!
…-layer into multiple_cvmfs_repos_eessi_container
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Tested it with various scenarios, and it worked like a charm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The "tests for eessi_container.sh script / eessi_container_script (listrepos_custom)" CI was actually already broken: it was configuring a "EESSI/20AB.CD" and "EESSI/20HT.TP", but then running a grep for ""[EESSI/2023.02]". Not only is that version not available, also the regex itself is wrong: the square brackets should have been escaped, otherwise they will just try to match any of those characters. Because of the wrong regex, the check still passed with this wrong EESSI version. I've fixed it in aa9895e. Also, the other check now doesn't look for "EESSI" anymore (which used to be the default repo name), but instead for "software.eessi.io". This was needed because of the changes in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regex is fixed and the CI passed without issues 🎉
BIND_PATHS="${BIND_PATHS},${EESSI_TMPDIR}/repos_cfg/${src}:${target}" | ||
done | ||
export EESSI_VERSION_OVERRIDE=${repo_version} | ||
export EESSI_CVMFS_REPO_OVERRIDE="/cvmfs/${repo_name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this was important, it shouldn't have been removed.
I'm now seeing errors like this in the build job for #632:
ERROR: software.eessi.io/versions/2023.06/compat/linux/aarch64 does not exist!
Note the missing /cvmfs/
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe we're missing a fix in bot/build.sh
, which now has this:
export EESSI_CVMFS_REPO_OVERRIDE=$(cfg_get_value "repository" "repo_name")
I guess this should be:
export EESSI_CVMFS_REPO_OVERRIDE=/cvmfs/$(cfg_get_value "repository" "repo_name")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, good catch, looks like that needs to be changed (and I think it makes more sense that these things are (only) set there and not in the container script).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support for multiple CVMFS repositories is needed for
dev.eessi.io
.Ready for review. Example test:
Partially fixes #616 (doesn't provide repository-specific bind mounts for
host_injections
).