-
Notifications
You must be signed in to change notification settings - Fork 35
Update CP2K ReFrame test #452
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Jean-guillaume Piccinali <[email protected]>
Co-authored-by: Jean-guillaume Piccinali <[email protected]>
Co-authored-by: Copilot <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
cscs-ci run alps-eiger-uenv;MY_UENV=cp2k/2025.1:v3 |
|
cscs-ci run alps-daint-uenv;MY_UENV=cp2k/2025.1:v2 |
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.
Pull Request Overview
This PR updates the CP2K ReFrame test suite to support CP2K version 2025.2 and improve the robustness of RPA calculations by tightening SCF convergence tolerances.
- Updates SCF tolerance (EPS_SCF) from 1.0E-6 to 1.0E-9 in input files for better RPA calculation convergence
- Adds version-specific logic to handle changes in SCF step counting introduced in CP2K 2025.2
- Updates CMake build options to reflect changes in CP2K 2025.2 defaults (MPI, LIBVORI, CUDA architectures)
- Implements xfail mechanism for known v2025.1 linking issues
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| checks/apps/cp2k/src/H2O-128-PBE-TZ.inp | Tightened SCF convergence tolerances from 1.0E-6 to 1.0E-9 for both inner and outer SCF loops |
| checks/apps/cp2k/src/H2O-128-PBE-TZ-max_scf_16.inp | New input file with MAX_SCF=16 to maintain consistent runtime behavior with CP2K 2025.2's revised SCF step counting |
| checks/apps/cp2k/cp2k_uenv.py | Updated reference runtimes, added version detection logic, updated CMake options for 2025.2, refactored wrapper setup, and added xfail decorator for v2025.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
checks/apps/cp2k/cp2k_uenv.py
Outdated
|
|
||
| # Manual compilation of v2025.1 with CMake is known to fail at link time, | ||
| # because of issues with the libxc integration. | ||
| return sn.and_(sn.assert_eq(self.job.exitcode, 0), sn.assert_ne(self.version, 'v2025.1')) |
Copilot
AI
Nov 19, 2025
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.
The xfail decorator on line 78 already marks this test as expected to fail for v2025.1. The additional check sn.assert_ne(self.version, 'v2025.1') in the sanity function is redundant. When using xfail, the test should validate its primary behavior, and the xfail mechanism will handle the expected failure case.
| return sn.and_(sn.assert_eq(self.job.exitcode, 0), sn.assert_ne(self.version, 'v2025.1')) | |
| return sn.assert_eq(self.job.exitcode, 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 is not true. The test fails precisely because of this assertion. The tests that really fails is the following compilation test, but since it fails at the linking phase it would be wasteful to run it in full. Therefore, I made the download fail. One alternative could be to simply remove the archive from JFrog.
|
|
||
|
|
||
| def version_from_uenv(): | ||
| return os.environ['UENV'].split('/')[1].split(':')[0] |
Copilot
AI
Nov 19, 2025
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.
The version_from_uenv() function could raise unclear errors if the UENV environment variable format is unexpected (e.g., IndexError if the splits don't produce enough elements). Consider adding error handling or validation, such as:
def version_from_uenv():
try:
return os.environ['UENV'].split('/')[1].split(':')[0]
except (KeyError, IndexError) as e:
raise ValueError(f"Unable to parse version from UENV environment variable: {os.environ.get('UENV', 'not set')}") from e| return os.environ['UENV'].split('/')[1].split(':')[0] | |
| try: | |
| return os.environ['UENV'].split('/')[1].split(':')[0] | |
| except (KeyError, IndexError) as e: | |
| raise ValueError(f"Unable to parse version from UENV environment variable: {os.environ.get('UENV', 'not set')}") from e |
|
|
||
| # CP2K CMake changed default values from 2025.2 onwards | ||
| # CMake options below are chosen depending on the version of CP2K | ||
| version = float(version_from_uenv()) |
Copilot
AI
Nov 19, 2025
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.
Converting the version to float may cause precision issues. For example, float('2025.2') equals 2025.2, but float('2025.10') would be 2025.1, which could lead to incorrect version comparisons. Consider using string comparison or a proper version comparison library like packaging.version.Version, or compare the version as a tuple of integers. For example:
version_str = version_from_uenv()
major, minor = map(int, version_str.split('.'))
if (major, minor) > (2025, 1):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.
While this is true, the assumption is that CP2K releases only twice a year (with one or two bug fixes releases if they are really important). The function can definitely be improved, and also needs to be made more robust since it doesn't seem to work when releasing a new uenv (see eth-cscs/alps-uenv#259).
| version = float(version_from_uenv()) | ||
| if version > 2025.1: |
Copilot
AI
Nov 19, 2025
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.
Converting the version to float may cause precision issues. For example, float('2025.2') equals 2025.2, but float('2025.10') would be 2025.1, which could lead to incorrect version comparisons. Consider using string comparison or a proper version comparison library like packaging.version.Version, or compare the version as a tuple of integers. For example:
version_str = version_from_uenv()
major, minor = map(int, version_str.split('.'))
if (major, minor) > (2025, 1):| version = float(version_from_uenv()) | |
| if version > 2025.1: | |
| version_str = version_from_uenv() | |
| major, minor = map(int, version_str.split('.')) | |
| if (major, minor) > (2025, 1): |
Update CP2K ReFrame test in preparation for eth-cscs/alps-uenv#259.
xfailto mark known failuresReframe-HPC>=4.9