Skip to content

Conversation

@hyeoksu-lee
Copy link
Contributor

@hyeoksu-lee hyeoksu-lee commented Jun 26, 2025

User description

Description

This PR introduces DoD Carpenter Cray modules.

Type of change

Please delete options that are not relevant.

  • Something else

Scope

  • This PR comprises a set of related changes with a common goal

If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.

How Has This Been Tested?

MFC compiles ok on Carpenter with Cray modules and test suite passes.

Test Configuration:

  • What computers and compilers did you use to test this: Carpenter Cray

Checklist

  • I have added comments for the new code
  • I added Doxygen docstrings to the new code
  • I have made corresponding changes to the documentation (docs/)
  • I have added regression tests to the test suite so that people can verify in the future that the feature is behaving as expected
  • I have added example cases in examples/ that demonstrate my new feature performing as expected.
    They run to completion and demonstrate "interesting physics"
  • I ran ./mfc.sh format before committing my code
  • New and existing tests pass locally with my changes, including with GPU capability enabled (both NVIDIA hardware with NVHPC compilers and AMD hardware with CRAY compilers) and disabled
  • This PR does not introduce any repeated code (it follows the DRY principle)
  • I cannot think of a way to condense this code and reduce any introduced additional line count

PR Type

Enhancement


Description

  • Add Carpenter Cray module configuration for DoD system

  • Update module selection interface to distinguish Cray/GNU compilers

  • Create new batch template for Carpenter Cray environment

  • Modify module loading logic to handle Cray-specific paths


Changes walkthrough 📝

Relevant files
Configuration changes
modules.sh
Update module selection for Carpenter Cray support             

toolchain/bootstrap/modules.sh

  • Update system selection menu to include Carpenter Cray (cc) option
  • Modify module reset logic to exclude both Carpenter variants
  • Update Cray library path handling for GNU Carpenter only
  • +4/-4     
    modules
    Define Carpenter Cray module configuration                             

    toolchain/modules

  • Add new cc configuration for Carpenter Cray modules
  • Include Cray-specific modules like PrgEnv-cray and cray-fftw
  • Rename existing Carpenter entry to specify GNU compiler
  • +5/-1     
    Enhancement
    carpenter-cray.mako
    Add Carpenter Cray batch execution template                           

    toolchain/templates/carpenter-cray.mako

  • Create new PBS batch template for Carpenter Cray system
  • Configure MPI execution with proper node/task allocation
  • Set up module loading with Cray-specific parameters
  • +48/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @hyeoksu-lee hyeoksu-lee requested a review from a team as a code owner June 26, 2025 07:05
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 26, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 7801bbe)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Inconsistent Syntax

    Mixed use of '!=' and '!=' operators in conditional statements. Line 71 uses '!=' while line 100 uses '!=' which may cause shell compatibility issues or unexpected behavior.

    if [ "$u_c" '!=' 'cc' ] && [ "$u_c" '!=' 'c' ]; then
        module reset > /dev/null 2>&1
        code="$?"
    
        # Purge if reset is not available
        if [ "$code" -ne '0' ]; then
            module purge > /dev/null 2>&1
        fi
    else
        module purge > /dev/null 2>&1
    fi
    
    ELEMENTS="$(__extract "$u_c-all") $(__extract "$u_c-$cg")"
    MODULES=`echo "$ELEMENTS" | tr ' ' '\n' | grep -v = | xargs`
    VARIABLES=`echo "$ELEMENTS" | tr ' ' '\n' | grep = | xargs`
    
    log " $ module load $MODULES"
    if ! module load $MODULES; then
        error "Failed to load modules."
    
        return
    fi
    
    if [ $(echo "$VARIABLES" | grep = | wc -c) -gt 0 ]; then
        log " $ export $(eval "echo $VARIABLES")"
        export $(eval "echo $VARIABLES") > /dev/null
    fi
    
    # Don't check for Cray paths on Carpenter, otherwise do check if they exist
    if [ ! -z ${CRAY_LD_LIBRARY_PATH+x} ] && [ "$u_c" '!=' 'c' ]; then
    Missing Tests

    New batch template for Carpenter Cray system lacks corresponding test cases to verify proper functionality, module loading, and job submission behavior.

    #!/usr/bin/env bash
    
    <%namespace name="helpers" file="helpers.mako"/>
    
    % if engine == 'batch':
    #PBS -l select=${nodes}:ncpus=192:mpiprocs=${tasks_per_node}
    #PBS -N "${name}"
    #PBS -l walltime=${walltime}
    % if partition:
    #PBS -q ${partition}
    % endif
    % if account:
    #PBS -A ${account}
    % endif
    % if email:
    #PBS -M ${email}
    #PBS -m abe
    % endif
    #PBS -o "${name}.out"
    #PBS -e "${name}.err"
    #PBS -V
    % endif
    
    ${helpers.template_prologue()}
    
    ok ":) Loading modules:\n"
    cd "${MFC_ROOT_DIR}"
    . ./mfc.sh load -c cc -m ${'g' if gpu else 'c'}
    cd - > /dev/null
    echo
    
    % for target in targets:
        ${helpers.run_prologue(target)}
    
        % if not mpi:
            (set -x; ${profiler} "${target.get_install_binpath(case)}")
        % else:
            (set -x; ${profiler}                              \
                mpirun -np ${nodes*tasks_per_node}            \
                       "${target.get_install_binpath(case)}")
        % endif
    
        ${helpers.run_epilogue(target)}
    
        echo
    % endfor
    
    ${helpers.template_epilogue()}

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 26, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix inequality operator syntax

    The inequality operator should be != instead of '!='. The quotes around the
    operator make it a literal string comparison rather than a proper inequality
    test.

    toolchain/bootstrap/modules.sh [71]

    -if [ "$u_c" '!=' 'cc' ] && [ "$u_c" '!=' 'c' ]; then
    +if [ "$u_c" != 'cc' ] && [ "$u_c" != 'c' ]; then
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a critical bug. In bash's test ([) command, quoting the inequality operator '!=' causes it to be treated as a literal string instead of an operator, which breaks the conditional logic. This fix is essential for the script to function as intended.

    High
    • More

    @hyeoksu-lee hyeoksu-lee marked this pull request as draft June 26, 2025 08:52
    @hyeoksu-lee hyeoksu-lee marked this pull request as draft June 26, 2025 08:52
    @codecov
    Copy link

    codecov bot commented Jun 26, 2025

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 45.77%. Comparing base (a34f0ca) to head (7801bbe).
    Report is 4 commits behind head on master.

    Additional details and impacted files
    @@           Coverage Diff           @@
    ##           master     #903   +/-   ##
    =======================================
      Coverage   45.77%   45.77%           
    =======================================
      Files          68       68           
      Lines       18664    18664           
      Branches     2251     2251           
    =======================================
      Hits         8543     8543           
      Misses       8763     8763           
      Partials     1358     1358           

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    🚀 New features to boost your workflow:
    • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

    @sbryngelson
    Copy link
    Member

    does it work? when it compiles cmake prints the compilers it finds (it's one of the first things it does), can you put the output here so I can see?

    @hyeoksu-lee hyeoksu-lee marked this pull request as ready for review June 26, 2025 22:56
    @hyeoksu-lee
    Copy link
    Contributor Author

    hyeoksu-lee commented Jun 26, 2025

    @sbryngelson It works. Here's the outputs:

    Not searching for unused variables given on the command line.
    -- The C compiler identification is Clang 18.1.6
    -- The CXX compiler identification is Clang 18.1.6
    -- The Fortran compiler identification is Cray 18.0.0
    -- Cray Programming Environment 2.7.32 C
    -- Detecting C compiler ABI info
    -- Detecting C compiler ABI info - done
    -- Check for working C compiler: /opt/cray/pe/craype/2.7.32/bin/cc - skipped
    -- Detecting C compile features
    -- Detecting C compile features - done
    -- Cray Programming Environment 2.7.32 CXX
    -- Detecting CXX compiler ABI info
    -- Detecting CXX compiler ABI info - done
    -- Check for working CXX compiler: /opt/cray/pe/craype/2.7.32/bin/CC - skipped
    -- Detecting CXX compile features
    -- Detecting CXX compile features - done
    -- Cray Programming Environment 2.7.32 Fortran
    -- Detecting Fortran compiler ABI info
    -- Detecting Fortran compiler ABI info - done
    -- Check for working Fortran compiler: /opt/cray/pe/craype/2.7.32/bin/ftn - skipped
    -- Checking whether /opt/cray/pe/craype/2.7.32/bin/ftn supports Fortran 90
    -- Checking whether /opt/cray/pe/craype/2.7.32/bin/ftn supports Fortran 90 - yes
    -- Performing Test SUPPORTS_MARCH_NATIVE
    -- Performing Test SUPPORTS_MARCH_NATIVE - Failed
    -- Performing Test SUPPORTS_MCPU_NATIVE
    -- Performing Test SUPPORTS_MCPU_NATIVE - Failed
    -- IPO / LTO is NOT available
    -- Found MPI_Fortran: /opt/cray/pe/craype/2.7.32/bin/ftn (found version "3.1")
    -- Found MPI: TRUE (found version "3.1") found components: Fortran
    -- Found SILO: /p/global/hyeoksu/MFC/carpenter-cray/build/install/silo/lib/libsiloh5.a
    -- Found HDF5: /opt/cray/pe/hdf5/1.14.3.1/CRAY/18.0/lib/libhdf5.so
    -- Found FFTW: /opt/cray/pe/fftw/3.3.10.8/x86_rome/lib/libfftw3.so
    -- Configuring done
    -- Generating done
    -- Build files have been written to: /p/global/hyeoksu/MFC/carpenter-cray/build/staging/dd3ec89703

    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix inequality operator syntax

    The inequality operator should be != instead of '!='. The quotes around the
    operator make it a literal string comparison rather than a proper inequality
    test.

    toolchain/bootstrap/modules.sh [71]

    -if [ "$u_c" '!=' 'cc' ] && [ "$u_c" '!=' 'c' ]; then
    +if [ "$u_c" != 'cc' ] && [ "$u_c" != 'c' ]; then
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a critical bug. In bash's test ([) command, quoting the inequality operator '!=' causes it to be treated as a literal string instead of an operator, which breaks the conditional logic. This fix is essential for the script to function as intended.

    High
    • More

    @sbryngelson
    Copy link
    Member

    fantastic, merging

    @sbryngelson sbryngelson merged commit 6d8ecdd into MFlowCode:master Jun 27, 2025
    23 of 25 checks passed
    @hyeoksu-lee hyeoksu-lee deleted the carpenter-cray branch June 27, 2025 03:44
    prathi-wind pushed a commit to prathi-wind/MFC-prathi that referenced this pull request Jul 13, 2025
    Co-authored-by: Hyeoksu Lee <[email protected]>
    Co-authored-by: Hyeoksu Lee <[email protected]>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Development

    Successfully merging this pull request may close these issues.

    2 participants