use streak.llnl.gov as runner for GHA #2873

rrsettgast · 2023-12-07T00:42:42Z

This PR uses streak to execute one CUDA build and execute unit tests on the GPU. Two tests are disabled testLifoStorage and testExternalSolvers are disabled on streak due to failure of those tests on GPU with unified memory disabled. Follow up PR's will be needed to fix and reenable those tests.

…rRunner

codecov · 2024-01-26T20:35:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5c88b35) 51.44% compared to head (931b4e2) 51.34%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2873      +/-   ##
===========================================
- Coverage    51.44%   51.34%   -0.11%     
===========================================
  Files          972      972              
  Lines        87067    82389    -4678     
===========================================
- Hits         44789    42300    -2489     
+ Misses       42278    40089    -2189

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

rrsettgast · 2024-02-06T02:38:04Z

src/coreComponents/constitutive/solid/SolidBase.hpp

@@ -98,7 +98,7 @@ class SolidBaseUpdates
  arrayView1d< real64 const > const m_thermalExpansionCoefficient;

  /// Flag to disable inelasticity
-  const bool & m_disableInelasticity;
+  const bool m_disableInelasticity;


This was a bug on heterogeneous memory systems. @klevzoff This was the problem with the constitutive unit tests from the remove array1d<string>

is there any way to automatically check for this kind of bugs? Any way to at least get a warning whenever a reference is captured by value in a raja forAll?

rrsettgast · 2024-02-06T02:38:29Z

src/coreComponents/linearAlgebra/unitTests/testLinearAlgebraUtils.hpp

@@ -192,6 +192,8 @@ void compute2DLaplaceOperator( MPI_Comm comm,

  // Construct the 2D Laplace matrix
  laplace2D.create( matrix.toViewConst(), matrix.numRows(), comm );
+
+//  laplace2D.write( "matrix.txt", LAIOutputFormat::NATIVE_ASCII);


keep this because it will be needed when we fix this test on GPU.

rrsettgast · 2024-02-06T02:39:27Z

scripts/ci_build_and_test_in_container.sh

@@ -232,7 +252,11 @@ fi

 # Run the unit tests (excluding previously ran checks).
 if [[ "${RUN_UNIT_TESTS}" = true ]]; then
-  or_die ctest --output-on-failure -E "testUncrustifyCheck|testDoxygenCheck"
+  if [[ ${HOSTNAME} == 'streak.llnl.gov' ]]; then
+    or_die ctest --output-on-failure -E "testUncrustifyCheck|testDoxygenCheck|testLifoStorage|testExternalSolvers"


This is to exclude the two failing unit tests on GPU systems so we can merge this and follow up with fix PR's.

Can you create an issue pointing at this line though? If someone other than you fixes the tests I am pretty sure they'll forget to remove this.

rrsettgast · 2024-02-06T02:39:52Z

scripts/ci_build_and_test_in_container.sh

+    update-ca-certificates 
+    # gcloud config set core/custom_ca_certs_file cert.pem'
+
+    NPROC=8


this is hard coded right now for streak.

TotoGaz · 2024-02-06T06:02:13Z

.github/workflows/build_and_test.yml

+        if [ ${{ inputs.RUNS_ON }} == 'self-hosted' ]; then
+          RUNNER_CERTIFICATES_DIR=/etc/pki/ca-trust/source/anchors/
+          mkdir -p ${GITHUB_WORKSPACE}/certificates
+          cp ${RUNNER_CERTIFICATES_DIR}/*.crt* ${GITHUB_WORKSPACE}/certificates
+        fi


Could we use the DOCKER_RUN_ARGS input to add -v /etc/pki/ca-trust/source/anchors/.../bundle.crt:/etc/pki/ca-trust/.../llnl.bundle.crt:ro.

That way the certificate would be at the proper location in read-only, with no copy required from within the container.

Then inside of the docker container simply update the certificates list (we can do it whatever the case : it's fast and could do not harm?). Or if we really want it, we can add an option.

We certainly can...and probably should. Would you mind if we did this in a follow up PR. We are getting another workstation to run integratedTests on the CPU, and we will require a generic treatment at that time. I would rather just get this merged as is, and in separate PR's:

clean up and generalize for the new workstation,

fix the failing unit tests on GPU, and

get integrated tests diffs worked out.

we can do all of these steps independent of each other IMO.

OK, I'm good with it. We need to be sure we actually have time to revisit this.

.github/workflows/build_and_test.yml

TotoGaz · 2024-02-06T06:04:21Z

.github/workflows/build_and_test.yml

+        #rm -rf ${{ github.workspace }}/*
+        #docker rm -f ${CONTAINER_NAME}


So do you still need to clean up?

Then can you add a small comment about it?

TotoGaz · 2024-02-06T06:20:14Z

scripts/ci_build_and_test_in_container.sh

+# Cleaning the build directory.
+or_die ninja clean


This is most likely superfluous since the build dir is in /tmp which is washed away with the --rm option of docker run. You can keep it if you want, since this may change.

I think you are right.

TotoGaz · 2024-02-06T06:21:54Z

scripts/ci_build_and_test_in_container.sh

@@ -256,8 +280,14 @@ if [[ "${RUN_INTEGRATED_TESTS}" = true ]]; then
  or_die tar cfM ${DATA_EXCHANGE_DIR}/${DATA_BASENAME_WE}.tar --directory ${GEOS_SRC_DIR}    --transform "s/^integratedTests/${DATA_BASENAME_WE}\/repo/" integratedTests
  or_die tar rfM ${DATA_EXCHANGE_DIR}/${DATA_BASENAME_WE}.tar --directory ${GEOSX_BUILD_DIR} --transform "s/^integratedTests/${DATA_BASENAME_WE}\/logs/" integratedTests
  or_die gzip ${DATA_EXCHANGE_DIR}/${DATA_BASENAME_WE}.tar
+
+  # want to clean the integrated tests folder to avoid polluting the next build.
+  or_die integratedTests/geos_ats.sh -a clean


With @cssherman 's patch to build the baselines out of source, cleaning the integrated tests will surely not be mandatory anymore, since they disappear with the docker run --rm.
Also, we will be able to mount the source read-only!

I think this was before the --rm command. I can try without.

Oh, I think @cssherman 's PR has not been merged already. Am I wrong?

clean not veryclean ? I think it will still be better to clean up the integratedTests directory. One may update the version of the baselines... and the clean also removes the tests results which I think we do want to remove IMO. Unless the rm options cleans all that for us.

An option can be to mount the workspace read-only in the container, then copy it to a temporary and work in it (waiting for an out-of-source integrated tests which should solve the last item which touches the src dir).
Then everything would be cleaned by the docker run --rm.

Eventually, we'll be able to skip the workspace copy, but not right now.

TotoGaz · 2024-02-06T06:30:13Z

scripts/ci_build_and_test_in_container.sh

@@ -232,7 +252,11 @@ fi

 # Run the unit tests (excluding previously ran checks).
 if [[ "${RUN_UNIT_TESTS}" = true ]]; then
-  or_die ctest --output-on-failure -E "testUncrustifyCheck|testDoxygenCheck"
+  if [[ ${HOSTNAME} == 'streak.llnl.gov' ]]; then
+    or_die ctest --output-on-failure -E "testUncrustifyCheck|testDoxygenCheck|testLifoStorage|testExternalSolvers"


I'd add a EXCLUDE_UNIT_TESTS_RE input in GHA, defaulting to testUncrustifyCheck|testDoxygenCheck, so we do not have to hardcode anything.
Combined to giving the number of CPUs as a CLI arguments as well (NPROC can be provided through GHA, or defaulted to the results of nproc is absent), then we'll remove the need to hard code the name of the self hosted runner.

I would, but I actually don't think we should be excluding unit tests once they are fixed. This is just a hack to get us to the next step (passing tests), after witch this goes away. I actually do want to make it as hard as possible to exclude specific tests 😁

This makes sense. I kind of prefer having an easy way to do thing with a Cerberus at the code review, but it's OK.
What happens, is that the people maintaining the CI are not the same maintaining the tests or code. So the CI guys gets some ugly code he's not responsible for.

well....that is a good enough reason.

.github/workflows/ci_tests.yml

sframba · 2024-02-06T19:13:09Z

@rrsettgast as for the testLifoStorage, we plan on fixing it on CUDA in the not-too-distant future. It's supposed to work but it's broken right now.
Btw we had testExternalSolvers fail on Pangea3 too, with one single test failing (Hypre/SolverTestLaplace2D/0.GMRES_ILU, see this discussion with @victorapm) . We are available to do more tests if needed

* use pip_system_certs...this time in right context * add run args for docker image * clean workspace from inside the docker container * add nvidia runtime to docker args

attempt to transfer modifications from old branch feature/useStreakFo…

50a62e4

…rRunner

rrsettgast added ci: run CUDA builds Allows to triggers (costly) CUDA jobs DO NOT MERGE ! labels Dec 7, 2023

rrsettgast self-assigned this Dec 7, 2023

rrsettgast added 21 commits December 6, 2023 18:18

Merge branch 'develop' into feature/useStreakForRunner2

b491777

test

e3593b1

test

3428f15

test

e6994d1

test

a485937

test

318d833

test

6cf1dbf

test

9574bbd

test

3b9d75a

test

8c8ac20

test

abde69d

test

513e7b8

test

1ddcb6a

test

c24a25d

test

0fd6b8b

test

9d89343

test

95d6455

test

1dfd2ee

test

7f759b4

test

521ee05

test

57bad0e

rrsettgast removed the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Dec 8, 2023

rrsettgast added 4 commits December 8, 2023 13:33

mostly works for ubuntu. fails on python gsutil push

dc94a52

try to use pip_system_certs

498f96c

try to use pip_system_certs...this time in right context

eea10cf

output some information for debugging the issues with gcp

cdad4be

rrsettgast added 5 commits January 26, 2024 15:15

try to set NPROC for streak case

8738cd7

try to set NPROC for streak case

0b5fefe

try to set NPROC for streak case

336519b

update google-cloud actions

a0d23df

Merge branch 'develop' into feature/useStreakForRunner2

9751403

rrsettgast force-pushed the feature/useStreakForRunner2 branch from d13ab2f to 9751403 Compare February 4, 2024 21:01

rrsettgast added 2 commits February 5, 2024 10:55

fix bug involing a reference data member in SoidBaseUpdates

54b474d

Merge branch 'develop' into feature/useStreakForRunner2

f627583

rrsettgast force-pushed the feature/useStreakForRunner2 branch from 1bd45aa to f627583 Compare February 5, 2024 18:56

rrsettgast added 2 commits February 5, 2024 16:55

disable SolverTestLaplace2D/GMRES_ILU test

bd0505b

exclude failing unit tests on streak cuda

0fb6af5

rrsettgast requested review from TotoGaz and CusiniM February 6, 2024 02:36

rrsettgast added 2 commits February 5, 2024 18:43

update LvArray submodule

2996020

update submodules

346ea5a

rrsettgast commented Feb 6, 2024

View reviewed changes

rrsettgast requested a review from bmhan12 February 6, 2024 02:53

rrsettgast removed the DO NOT MERGE ! label Feb 6, 2024

TotoGaz reviewed Feb 6, 2024

View reviewed changes

CusiniM approved these changes Feb 6, 2024

View reviewed changes

CusiniM and others added 5 commits February 6, 2024 11:43

Merge branch 'develop' into feature/useStreakForRunner2

52c72ee

Merge branch 'develop' into feature/useStreakForRunner2

2dc1ce7

re-enable specificaiton of container name

a25def1

revert integrated tests to 2 core

80c2441

run integrated tests for non-pull request

931b4e2

rrsettgast merged commit cb4e99b into develop Feb 7, 2024
42 of 43 checks passed

rrsettgast deleted the feature/useStreakForRunner2 branch February 7, 2024 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use streak.llnl.gov as runner for GHA #2873

use streak.llnl.gov as runner for GHA #2873

rrsettgast commented Dec 7, 2023 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading

rrsettgast Feb 6, 2024 •

edited

Loading

CusiniM Feb 6, 2024

rrsettgast Feb 6, 2024

rrsettgast Feb 6, 2024

CusiniM Feb 6, 2024

rrsettgast Feb 6, 2024

TotoGaz Feb 6, 2024 •

edited

Loading

rrsettgast Feb 6, 2024

TotoGaz Feb 6, 2024

TotoGaz Feb 6, 2024

rrsettgast Feb 6, 2024

TotoGaz Feb 6, 2024

TotoGaz Feb 6, 2024

rrsettgast Feb 6, 2024

TotoGaz Feb 6, 2024

rrsettgast Feb 6, 2024

TotoGaz Feb 6, 2024 •

edited

Loading

CusiniM Feb 6, 2024 •

edited

Loading

TotoGaz Feb 6, 2024 •

edited

Loading

TotoGaz Feb 6, 2024

rrsettgast Feb 6, 2024

TotoGaz Feb 6, 2024

rrsettgast Feb 6, 2024

sframba commented Feb 6, 2024 •

edited

Loading

		#rm -rf ${{ github.workspace }}/*
		#docker rm -f ${CONTAINER_NAME}

use streak.llnl.gov as runner for GHA #2873

use streak.llnl.gov as runner for GHA #2873

Conversation

rrsettgast commented Dec 7, 2023 • edited Loading

codecov bot commented Jan 26, 2024 • edited Loading

Codecov Report

rrsettgast Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotoGaz Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotoGaz Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

CusiniM Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

TotoGaz Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sframba commented Feb 6, 2024 • edited Loading

rrsettgast commented Dec 7, 2023 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading

rrsettgast Feb 6, 2024 •

edited

Loading

TotoGaz Feb 6, 2024 •

edited

Loading

TotoGaz Feb 6, 2024 •

edited

Loading

CusiniM Feb 6, 2024 •

edited

Loading

TotoGaz Feb 6, 2024 •

edited

Loading

sframba commented Feb 6, 2024 •

edited

Loading