test: compile thermochem kernels with the configured Fortran compiler - #1943
Open
sbryngelson wants to merge 1 commit into
Open
sbryngelson wants to merge 1 commit into
sbryngelson wants to merge 1 commit into
Conversation
test_thermochem.py hard-coded shutil.which("gfortran") and GNU flags. On
Frontier, ./mfc.sh load selects CCE (ftn) and leaves gfortran as the system
GCC 7.5, so ./mfc.sh lint failed test_precision_and_directives[dp-acc,dp-mp]:
GCC 7.5 rejects `declare target device_type(any)` and cannot link its nvptx
offload image. GitHub's ubuntu-latest has a modern gfortran, so CI never saw it.
Pick $FC, else ftn, else gfortran (as CMake does), identify the family from
--version, and use that family's flags and MFC_COMPILER define. GNU and Cray
are supported; other compilers skip with a message. CCE's OpenMP variant skips
without a craype-accel module, which declare target requires. The falloff
driver now writes with an explicit format, since Cray's list-directed output
is not parseable by np.fromstring.
The precheck hook now sets MFC_SKIP_COMPILER_TESTS=1, as it does
MFC_SKIP_RENDER_TESTS, so whether a commit passes no longer depends on the
modules loaded in the committer's shell. CI's ./mfc.sh lint still runs them.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Updates thermochemistry kernel compilation tests to use the Fortran compiler/configuration selected by the toolchain (rather than hard-coded gfortran), improving portability on systems like Frontier/CCE and making local prechecks independent of loaded modules.
Changes:
- Detect the active Fortran compiler (
$FC, elseftn, elsegfortran), infer compiler family, and apply family-specific compile/offload flags and fypp defines. - Add skip controls for compiler-dependent tests (
MFC_SKIP_COMPILER_TESTS) and special-case skipping for Cray OpenMP offload when no accelerator module is loaded. - Make thermochem driver output deterministic/parseable across compilers by switching from list-directed
print *to an explicit format.
| File | Description |
|---|---|
| toolchain/mfc/test_thermochem.py | Uses configured Fortran compiler/family flags (GNU/Cray), adds skip logic, and stabilizes driver output formatting. |
| toolchain/bootstrap/precheck.sh | Skips Fortran-compilation tests during local precheck via MFC_SKIP_COMPILER_TESTS=1. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| path = shutil.which(name) | ||
| if path is None: | ||
| continue | ||
| version = subprocess.run([path, "--version"], capture_output=True, text=True, check=False).stdout |
Comment on lines
+74
to
+85
| @functools.cache | ||
| def fortran_compiler(): | ||
| """The compiler MFC's build would use ($FC, else ftn or gfortran on PATH) and its family, or None.""" | ||
| names = [os.environ["FC"]] if os.environ.get("FC") else ["ftn", "gfortran"] | ||
| for name in names: | ||
| path = shutil.which(name) | ||
| if path is None: | ||
| continue | ||
| version = subprocess.run([path, "--version"], capture_output=True, text=True, check=False).stdout | ||
| family = "Cray" if "Cray Fortran" in version else "GNU" if "GNU Fortran" in version else None | ||
| return path, family | ||
| return None |
| FPE_TRAP: ["-ffpe-trap=invalid,zero,overflow"], | ||
| }, | ||
| # CCE enables OpenACC by default; turn it off unless asked for, as cmake/MFCTargets.cmake does. | ||
| "Cray": {"base": ["-eZ"], None: ["-hnoacc"], "acc": ["-hacc"], "mp": ["-hnoacc", "-fopenmp"], FPE_TRAP: ["-Ktrap=divz,inv,ovf"]}, |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
toolchain/mfc/test_thermochem.py(added in #1915) hard-codedshutil.which("gfortran")and GNU flags. On Frontier,source ./mfc.sh load -c f -m gselects CCE (ftn) and leavesgfortranas the system GCC 7.5, so./mfc.sh lintfailedtest_precision_and_directives[dp-acc]and[dp-mp]:dp-mp: GCC 7.5 rejects!$omp declare target device_type(any)(fromomp_macros.fpp).dp-acc: GCC 7.5 compiles but fails to link its nvptx offload image.CI never saw this because the only jobs that run
./mfc.sh lintare on GitHub-hostedubuntu-latest, which ships a modern gfortran.Changes
$FC, elseftn, elsegfortran. Identify the family from--versionand use that family's flags plus theMFC_<id>/MFC_COMPILERfypp defines, ascmake/Fypp.cmakedoes.-ffree-line-length-none(ascmake/GPU.cmake).-eZ,-hacc/-hnoacc,-fopenmp,-Ktrap=divz,inv,ovf.craype-accelmodule is loaded (CRAY_ACCEL_TARGETunset); CCE rejectsdeclare targetwithout one.print *output isn't parseable bynp.fromstring.precheck.shsetsMFC_SKIP_COMPILER_TESTS=1, like the existingMFC_SKIP_RENDER_TESTS, so whether a commit passes no longer depends on the modules loaded in the committer's shell. CI's./mfc.sh lintstill runs these tests.Testing (Frontier login node)
source ./mfc.sh load -c f -m g && ./mfc.sh lint: 767 passed (was 2 failed on master)../mfc.sh lintwithout the load: 766 passed, 1 skipped (Cray OpenMP variant).FC=gfortran(system GCC 7.5) with the load: only the acc/mp variants fail, as expected for a compiler that can't build MFC's GPU directives.Acknowledgement