Skip to content

Conversation

@weihuang-jedi
Copy link
Contributor

Description

Make GW be able to run with container has spack-stack 1.9.2 inside.

change includes:
Add new module files, make detect-machine, module-setup can detect and setup for container.
Add new scripts to shell-in container compiling GW,
Add new scripts to generate run cases using container.
Add wrapper and other scripts to run GW with container.

Resolves #3963

Type of change

  • Bug fix (fixes something broken)
  • [x ] New feature (adds functionality)
  • Maintenance (code refactor, clean-up, new CI test, etc.)

Change characteristics

How has this been tested?

  • Clone and build on Ursa, AWS.

Checklist

  • Any dependent changes have been merged and published
  • My code follows the style guidelines of this project
  • [x ] I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have documented my code, including function, input, and output descriptions
  • My changes generate no new warnings
  • New and existing tests pass with my changes
  • This change is covered by an existing CI test or a new one has been added
  • Any new scripts have been added to the .github/CODEOWNERS file with owners
  • I have made corresponding changes to the system documentation if necessary

@weihuang-jedi weihuang-jedi self-assigned this Oct 2, 2025
Copy link
Contributor

@WalterKolczynski-NOAA WalterKolczynski-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still doesn't address a lot of the concerns about containing the containerization code. Please consider if this can all be done modifying only things run up-front before the j-job is called (*.env script, modulefiles), without using alternate scripts or altering the main flow of scripts.

#
###############################################################################
#
source "${HOMEgfs}/dev/ush/load_gw_run_modules.sh"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not be loading modules in the middle of a job here or in the ush scripts like you have added in many places. All modules need to be loaded up front before the j-job begins.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, I have not think of a better way to handle this.
So, there are quite few scripts are doing this.
I was thinking add this to preamble.sh, what do you think?

# overwrite MACHINE_ID if in container
if [[ -v SINGULARITY_CONTAINER ]]; then
# We are in a container
MACHINE_ID=container
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think you are going to be more successful if container is just a separate setting and not a MACHINE_ID. There are times we will still need to know what hardware we are on when running from container.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need more time to think about this. Please update if you have something to share.

echo "${nm} ${line}" >> "${mpmd_cmdfile}"
((nm=nm+1))
done < "${cmdfile}"
# Redirect output from each process to its own stdout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there such drastic changes here? It is designed to detect the scheduler type so it can form the MPMD script properly. This removes all that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run_mpmd.sh is little complicated. Need more time to think and re-check.

Comment on lines 27 to 31
if [[ "${RUN_WITH_CONTAINER}" == "YES" ]]; then
"${HOMEgfs}/exec/run_python.sh" "${GLOBALARCHIVESH:-${SCRgfs}/exglobal_archive_vrfy.py}" -c -v
else
${GLOBALARCHIVESH:-${SCRgfs}/exglobal_archive_vrfy.py}
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really do not want to add blocks like this all over the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried a different way. Please check and comment.

chmod +x "${exec_python_script}"
chmod +x "${run_python_script}"

for item in JGLOBAL_WAVE_INIT

Check warning

Code scanning / shellcheck

This loop will only ever run once. Bad quoting or missing glob/expansion? Warning

This loop will only ever run once. Bad quoting or missing glob/expansion?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use container with spack-stack 1.9.2

3 participants