-
Notifications
You must be signed in to change notification settings - Fork 705
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}[GCCcore/12.3.0] PAPI v7.1.0 #20081
{perf}[GCCcore/12.3.0] PAPI v7.1.0 #20081
Conversation
icl-utk-edu/papi#160 reported by @Flamefire |
Additionally there already is a PAPI EC for 12.3.0 |
@Flamefire we wanted support for |
I see. The general approach, at least for the ECs of this official repo, is to have exactly 1 software version per toolchain to avoid conflicts (aka dependency hell) That just for explanation. From what I can see there currently is no PAPI for 13.1 or 13.2 so you could add that for this toolchain. Although we'll have trouble at our site installing this as the test will fail. |
You're close, but not quite: we have no restrictions on having versions of a software in a given toolchain. E.g. having two GROMACS versions in the same toolchain is totally fine. So in that sense, this PR is totally fine. What we do not allow (at least not by default) is using multiple different versions of a software as a dependency. Our CI also checks for this, and fails if you try to do this. We do make exceptions if there are good reasons: sometimes, your software just needs a newer version of a dependency, because it will not work otherwise. Other justified reasons for using a second dependency version is if there is a proven/substantial performance benefit. In those cases, the PR that contributes an EasyConfig that depends on a 2nd version has to also include an exception for the CI tests, to make those pass. See e.g. here. I'd say the fact that It should all be seen in the context of why we try to avoid this: if software A and B both depend on C, but on different versions, you cannot load A and B at the same time. One caveat here is that from this I understand that Then again: I'm not the most independent reviewer here, since Satish and I are both at the same company :) I'll ping on the maintainers chat for an second opinion ;-) |
@Flamefire and @casparvl I have added the file for |
@satishskamath could you explain a bit more please? Not sure I follow. |
I think this can be closed since @Flamefire has already pushed for PAPI 7.1.0 in 13.2.0 which I did here as well. |
Ah yes, I added it as part of a larger PR including Score-P and forgot about that. I've seen the issue (in the test) pop up randomly there too but not in the test report where it happened to succeed. |
(created using
eb --new-pr
)