-
Notifications
You must be signed in to change notification settings - Fork 704
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}[gompi/2022a,gompi/2022b,gompi/2023a] CubeLib v4.8.2, CubeWriter v4.8.2, Score-P v8.3 w/ CUDA 11.7.0, CUDA 12.0.0, ... #19524
base: develop
Are you sure you want to change the base?
Conversation
Test report by @Flamefire |
Test report by @Flamefire |
@Flamefire PAPI-7.1.0 was released in December 2023. Shall I upgrade the dependencies which include PAPI in this PR? |
7a2f942
to
93a50b0
Compare
This PR doesn't add PAPI so we already have a PAPI in the ECs. Adding another version isn't a good idea unless there is a really pressing reason to make an exception. Also I found some issues with PAPI 7.1 at least in the tests: icl-utk-edu/papi#160 |
@Flamefire I recently created a PR for PAPI 7.1.0 and have pasted the issue there. We required it because it also introduces support for |
Test report by @Flamefire |
I just noticed that while I updated Score-P here to 8.4 as requested by @Thyre in #20146 I didn't change the PAPI version used and simply used the existing 7.0.x versions. as per the reasoning of @casparvl we might want to use PAPI 7.1 here too introducing a second version into those toolchains, but we already needed to add additional versions of the Cube* ECs (required newer versions by Score-P) |
Test report by @Flamefire |
Ok, I'm a bit confused now with all the version discussion :D
Do you mean here that your intention is to get this PR merged and have another PR with newer PAPI 7.1? Or do you plan to bump the PAPI version used in this PR and make a second PR? |
So basically: Some people need PAPI 7.1. This adds ECs using existing PAPI 7.0 but at the same time adds a 2nd version of another dependency (Cube*) So we can either keep this as-is or add PAPI 7.1 to the toolchains used here and update this PR to use PAPI 7.1 instead of 7.0 as dependencies. So yes: Another PR with PAPI 7.1 and then bump PAPI in this PR before it gets merged. |
@casparvl To get this resolved I updated the used PAPI versions here to 7.1. While we IIRC already have another version in some toolchains Score-P is the only EC using it from our side. So IMO this is the best option |
…Ccore-12.3.0.eb, CubeWriter-4.8.2-GCCcore-11.3.0.eb, CubeWriter-4.8.2-GCCcore-12.3.0.eb, Score-P-8.4-gompi-2022a-CUDA-11.7.0.eb, Score-P-8.4-gompi-2022a.eb, Score-P-8.4-gompi-2022b-CUDA-12.0.0.eb, Score-P-8.4-gompi-2023a-CUDA-12.1.1.eb, Score-P-8.4-gompi-2023a.eb
51d3266
to
97ab723
Compare
CI doesn't like when I use PAPI 7.1.0 for 2022a, 2022b or 2023a. So either we add an exception to the CI similar to the Cube* one or accept that zen4 won't be supported until 2023b |
Test report by @Flamefire |
Test report by @Flamefire |
Updated software
|
Test report by @Flamefire |
The patch should not be necessary with easybuilders/easybuild-easyblocks#3496 I think. |
63fbf75
to
97ab723
Compare
This reverts commit 63fbf75.
You are right. I reverted that commit so if anyone runs into the error
might be able to find it. @bedroge Can you take a look at this PR? |
I agree. However, looking at the initial patch again, there was also a tiny mistake, that may cause unintended side-effects diff -ur scorep-8.0-orig/build-backend/configure scorep-8.0/build-backend/configure
--- scorep-8.0-orig/build-backend/configure 2024-11-28 13:53:42.539876288 +0100
+++ scorep-8.0/build-backend/configure 2024-11-28 13:58:04.917444170 +0100
@@ -37776,7 +37776,7 @@
esac ;; #(
set2set3) :
case ${with_libbfd_lib}${with_libbfd_include} in #(
- *yes*|*no*) :
+ yes,*|no,*|*,yes|*,no) :
as_fn_error $? "Both, --with-libbfd-lib and --with-libbfd-include require a <path>." "$LINENO" 5 ;; #(
*) : The -case ${with_libbfd_lib}${with_libbfd_include} in #(
+case ${with_libbfd_lib},${with_libbfd_include} in #( Same for the others. We came across the same issue in the EasyBlock PR: easybuilders/easybuild-easyblocks#3496 (comment). But to be honest, such tiny things are hard to spot 😄 |
Well the intention of the patch was to NOT match the case anymore. The (faulty) patch achieves that ;-) It just never matches now. But Bert just notified me of the same when I copied this patch to our internal repo which doesn't have the easyblock fix yet. I corrected it there but as the patch is removed here I think it doesn't make sense to update it again. |
@boegelbot please test @ generoso |
@bedroge: Request for testing this PR well received on login1 PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2513906340 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot |
Test report by @bedroge |
(created using
eb --new-pr
)