Skip to content

Updated Bridges2 Modules (CPU/GPU) #905

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 27, 2025
Merged

Updated Bridges2 Modules (CPU/GPU) #905

merged 3 commits into from
Jun 27, 2025

Conversation

@Malmahrouqi3
Copy link
Collaborator

@Malmahrouqi3 Malmahrouqi3 commented Jun 27, 2025 •
edited
Loading

User description

Description

Concerning (#614),

Updated modules for Bridges2, tested on --qos=cpuinteract/gpuinteract. MFC compiles and builds neatly. To address the outdated python version issue, anaconda3 is loaded as it houses a very recent version. Foreseen concern would be dependency conflict with the co-existing py venv & conda env. Therefore, please keep conda as-is and do not attempt to install any dependency on it at all. To run tests, append -- --binary mpirun or according to the official MFC docs -- -c bridges2 is the preferred way.

PR Type

Enhancement

Description

  • Updated Bridges2 GPU modules configuration

  • Added anaconda3 and hdf5 to common modules

  • Reordered GPU module loading sequence

  • Added CUDA compute capability and home path settings

Changes walkthrough 📝

Relevant files
Configuration changes
modules
Updated Bridges2 module configuration                                       

toolchain/modules

  • Added anaconda3 and hdf5 to common Bridges2 modules
  • Reordered GPU module loading sequence (nvhpc/22.9, cuda/11.7, openmpi)
  • Added MFC_CUDA_CC=70,75,80 and NVHPC_CUDA_HOME=$CUDA_HOME environment
    variables
  • Updated CUDA module version from generic to specific cuda/11.7
    		• +3/-3     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    Copilot AI review requested due to automatic review settings June 27, 2025 02:11
    @Malmahrouqi3 Malmahrouqi3 requested a review from a team as a code owner June 27, 2025 02:11
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 27, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    614 - PR Code Verified

    Compliant requirements:

    • Use openmpi/4.0.5-nvhpc22.9, python/3.8.6, cuda/11.7.1
    • Set environment variables FC=nvfortran, CC=nvc, CXX=nvc++

    Requires further human verification:

    • Verify modules actually work on Bridges2 system
    • Test MFC compilation and execution with new modules

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

    Module Conflicts

    Adding both python/3.8.6 and anaconda3 to common modules may cause Python environment conflicts or version mismatches that could affect dependency resolution

     
    b-all python/3.8.6 hdf5 anaconda3
b-cpu allocations/1.0 gcc/10.2.0 openmpi/4.0.5-gcc10.2.0
    Missing Validation

    The CUDA compute capability values and NVHPC_CUDA_HOME environment variable are added without validation that they work correctly on the target system

     
    b-gpu MFC_CUDA_CC=70,75,80 NVHPC_CUDA_HOME=$CUDA_HOME CC=nvc CXX=nvc++ FC=nvfortran

    Copilot AI reviewed Jun 27, 2025
    View reviewed changes
    Copy link
    Contributor

    Copilot AI left a 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 Bridges2 modules for both CPU and GPU configurations as part of addressing issue #614. The key changes include:

    • Adding "hdf5" and "anaconda3" to the Python module configuration.
    • Updating GPU-related module definitions with a newer CUDA version and additional environment variables.
    • Reordering GPU module definitions for clarity.
    Comments suppressed due to low confidence (1)

    toolchain/modules:17

    • Consider including a brief comment or documentation reference in the file to indicate why 'anaconda3' was added alongside python/3.8.6, given the PR description caution regarding dependency conflicts.
    b-all python/3.8.6 hdf5 anaconda3

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 27, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add fallback for environment variable

    The NVHPC_CUDA_HOME=$CUDA_HOME assignment may fail if $CUDA_HOME is not set when
    this line executes. Consider using a more robust path or verifying the variable
    exists before assignment.

    toolchain/modules [20]

    -b-gpu MFC_CUDA_CC=70,75,80 NVHPC_CUDA_HOME=$CUDA_HOME CC=nvc CXX=nvc++ FC=nvfortran
+b-gpu MFC_CUDA_CC=70,75,80 NVHPC_CUDA_HOME=${CUDA_HOME:-/opt/packages/cuda/11.7} CC=nvc CXX=nvc++ FC=nvfortran
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies that the $CUDA_HOME variable might not be set. Adding a fallback value using ${CUDA_HOME:-...} is a good defensive practice that improves the script's robustness.

    		Low
    • More

    @sbryngelson
    Copy link
    Member

    sbryngelson commented Jun 27, 2025

    Why this part

    To run tests, append -- --binary mpirun.

    it should run tests with -- -c bridges2 (this is the MFC way, at least)

    @Malmahrouqi3
    Copy link
    Collaborator Author

    Malmahrouqi3 commented Jun 27, 2025 •
    edited
    Loading

    I edited the description.

    Edit: just confirming, either way works fine.

    @sbryngelson sbryngelson merged commit 07d8f1f into MFlowCode:master Jun 27, 2025
    17 checks passed
    @sbryngelson sbryngelson mentioned this pull request Jul 2, 2025
    Closed
    prathi-wind pushed a commit to prathi-wind/MFC-prathi that referenced this pull request Jul 13, 2025
    @Malmahrouqi3 @prathi-wind
    Updated Bridges2 Modules (CPU/GPU) (MFlowCode#905)
    2786f5a
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    Copilot code review Copilot Copilot left review comments

    @sbryngelson sbryngelson sbryngelson approved these changes

    Assignees

    No one assigned

    Labels

    Review effort 2/5

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @Malmahrouqi3 @sbryngelson