-
Notifications
You must be signed in to change notification settings - Fork 15
Allow for CUDA compute capability fallbacks when initialising EESSI #5
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
Conversation
|
bot: build repo:eessi.io-2023.06-software instance:eessi-bot-mc-aws arch:x86_64/amd/zen2 |
|
New job on instance
|
|
Saw the test step failing in #11 (comment) and want to check that it is not as a result of merging that PR. bot: build repo:eessi.io-2023.06-software instance:eessi-bot-mc-aws arch:x86_64/amd/zen2 |
|
New job on instance
|
|
Something is going wrong with the new setup, untouched files are changing |
|
bot: build repo:eessi.io-2023.06-software instance:eessi-bot-mc-aws arch:x86_64/amd/zen2 |
|
New job on instance
|
| show_msg "archdetect found supported accelerator for CPU target ${EESSI_ACCEL_SOFTWARE_SUBDIR}: ${EESSI_ACCEL_SUBDIR}" | ||
| if [ ! -d $EESSI_ACCEL_SOFTWARE_PATH/${EESSI_ACCEL_SUBDIR} ]; then | ||
| # We should try to use the fallback compute capability | ||
| EESSI_ACCELERATOR_TARGET="${EESSI_ACCEL_SUBDIR::-1}0" |
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.
This makes the assumption that one can always replace the last digit with a zero. While that may be true, it might be better to define a function for this ... and add a comment about it (maybe even a link to some documentation).
In case the naming does not imply compatibility in the same way as now, one could then also later change the function instead of changing the variable expansion/setting.
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.
It is not enough to make a function just here, the same also needs to apply to the EESSI module. That feels overly complicated for now and could be done in a follow-up PR?
I can add a CI test that verifies that the fallback is as expected, that should make a follow-up PR easier.
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.
With 39272de it should at least be more obvious that this is being checked, so should help sanity checking a follow-up PR
|
Rebuild is not necessary, but why not... bot: build repo:eessi.io-2023.06-software instance:eessi-bot-mc-aws arch:x86_64/amd/zen2 |
|
New job on instance
|
lorisercole
left a comment
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.
I'm not an expert of EESSI, but this looks reasonable. Looking forward to test it!
bedroge
left a comment
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
|
Staging PR merged, tarball ingested. |
Replaces EESSI/software-layer#1115