Migrate to sitrep mechanism for T5X and PAXML MGMN tests #401

hemildesai · 2023-11-29T18:36:11Z

Addresses #399, #235 and #236

Example badge:

Completed run: https://github.com/NVIDIA/JAX-Toolbox/actions/runs/7039302468

Changes

Create a reusable job for publishing sitrep for MGMN workflows
Integrate it in T5X MGMN
Integrate it in PAX MGMN

TODO

Change badge endpoint in README for T5X
Change badge endpoint in README for PAX

Closes #399
Closes #235
Closes #236

terrykong · 2023-11-30T00:03:02Z

LGTM.

CC: @yhtang's to confirm sitrep impl.

yhtang

Cool work! Please see some thoughts and suggestions below 😄

.github/workflows/_sitrep_mgmn.yaml

yhtang · 2023-11-30T07:39:42Z

.github/workflows/_test_pax.yaml

    outputs:
      TEST_STATUS:
        description: 'Summary pass/fail value indicating if results from tests are acceptable'
-        value: ${{ jobs.publish-test.outputs.STATUS }}
+        value: ${{ jobs.sitrep.outputs.STATUS }}


This is similar to our current practice, which uses a final postprocessing job to determine whether the overall MGMN test suite succeeds.

It has the problem that error feedback is delayed and made it difficult to investigate which individual tests failed.

For the new sitrep reporting system, let's do the following:

Individual jobs should fail immediately if the tests that it ran do not succeed. However, it should collect and generate test artifacts regardless of whether the tests pass or fail. The continue-on-error option can be helpful here.

The sitrep/postprocessing step should then follow regardless of the success/fail status of individual test jobs.

This helps to localize the error status for easier debugging and tracking.

If a single job fails, what should be the outcome of the entire workflow? Also, I think we still need this output for downstream jobs like triage or publish container since those depend on this overall outcome.

For the MGMN usecase, the tests are submitted to slurm via SSH. To mark an individual job as failure I can inspect the exit code and mark it accordingly. We would still need the overall status though, if there's any other recommended way to get this output, I'm happy to incorporate that.

Makes sense. How about we make individual jobs fail while also let the overall status be the Boolean AND of the individual job states?

Actually, a single job failure is already indicated. (ref: https://github.com/NVIDIA/JAX-Toolbox/actions/runs/7053172928/job/19199718748). The STATUS has to be derived not only from the job result, but also from its metrics so it is not possible to set status just based on the job states.

At face value, the specific error message from the job that you provided looks coincidental. I understand that it may be the result of the true underlying error, but the message itself does not clearly indicate that. Could we improve on it?

Also could you please add a natural language description of the logic regarding

The STATUS has to be derived not only from the job result, but also from its metrics

so that it serves as a form of documentation?

At face value, the specific error message from the job that you provided looks coincidental. I understand that it may be the result of the true underlying error, but the message itself does not clearly indicate that. Could we improve on it?

Not sure if there's a way to provide a more informative error based on the failure, might have to look at it outside the scope of this PR.

Also could you please add a natural language description of the logic regarding

The STATUS has to be derived not only from the job result, but also from its metrics

so that it serves as a form of documentation?

Done in 40aebf5

yhtang · 2023-11-30T07:42:07Z

.github/workflows/_test_pax.yaml

@@ -518,12 +481,12 @@ jobs:
          ) | tee $GITHUB_STEP_SUMMARY

  outcome:
-    needs: publish-test
+    needs: sitrep


This job would be unnecessary if we localize the error status to individual test jobs. However, there may still be situations where it is needed, i.e. if some checks use the collective results of many/all test jobs.

I think this job is still needed to publish the overall badge since that's a collection of the results of all tests.

.github/workflows/nightly-t5x-test-mgmn.yaml

hemildesai · 2023-11-30T22:39:00Z

@yhtang @terrykong I've updated to_json.sh in a4c594a to make it work better with multi-line variables.

For eg, if you do from root of repo

summary=`cat README.md`

to_json summary # fails for older version, but succeeds for newer version

Please take a look and let me know if it looks good.

hemildesai · 2023-12-04T08:22:17Z

@yhtang All feedback should be addressed now. Please take another look to see if any more changes are required.

.github/workflows/_sitrep_mgmn.yaml

yhtang · 2023-12-06T23:30:30Z

I'm working on some light final touches on this PR.

A quick question regarding

echo "SHOULD_TRIAGE=${{ ((github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success') || github.event_name == 'workflow_dispatch') }} >> $GITHUB_OUTPUT"

If I use a different mechanism to cancel the workflow if its upstream worfklow did not succeed, do we still need the logic above at all? It appears that we always want to run triage as long as 1) upstream workflow succeeded or 2) the workflow is manually run via web GUI dispatch.

@hemildesai

hmonishN · 2023-12-06T23:43:25Z

Listing the requirements for perf data dump here for tracking purpose @yhtang

Job name to be dumped in artifacts (as part of sitrep.json file explaining the acronyms i.e TP, DP, FSDP, etc for pax and G, N for t5x, will be helpful if we add pax/t5x at the beginning of the job name)
_metric.json files to be dumped in the artifacts.

hemildesai · 2023-12-06T23:54:34Z

I'm working on some light final touches on this PR.

A quick question regarding
echo "SHOULD_TRIAGE=${{ ((github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success') || github.event_name == 'workflow_dispatch') }} >> $GITHUB_OUTPUT"
If I use a different mechanism to cancel the workflow if its upstream worfklow did not succeed, do we still need the logic above at all? It appears that we always want to run triage as long as 1) upstream workflow succeeded or 2) the workflow is manually run via web GUI dispatch.

@hemildesai

@yhtang Yeah then it should be fine to remove this condition.

yhtang · 2023-12-07T02:49:54Z

@hemildesai I made some changes to the two nightly-*-mgmn.yaml files. Could you please verify whether the changes are good?

yhtang · 2023-12-07T07:31:29Z

Test nightly workflows:
https://github.com/NVIDIA/JAX-Toolbox/actions/runs/7123351301/job/19395825884
https://github.com/NVIDIA/JAX-Toolbox/actions/runs/7123352749/job/19395829302

hemildesai · 2023-12-07T07:53:06Z

@yhtang Yes the changes look good. Thanks for the updates.

yhtang · 2023-12-07T12:45:59Z

A latest job https://github.com/NVIDIA/JAX-Toolbox/actions/runs/7127530028/job/19408352254 failed with the following error:

2023-12-07 12:39:40.366819: F external/xla/xla/stream_executor/cuda/cuda_dnn.cc:7393] Failed to generate cuDNN execution plan for opGraph Mul_Matmul_Add_Reduction_Sub_Exp_Reduction_Div_Matmul_. Status of final plan: CUDNN_STATUS_NOT_SUPPORTED

Although it is not caused by this PR, the fact that the job status is green seems to defeat our expectation where individual job states should reflect the SLURM job exit states.

The job status was:

$ sacct -j 23766
JobID           JobName  Partition    Account  AllocCPUS      State ExitCode
------------ ---------- ---------- ---------- ---------- ---------- --------
23766        T5X-71275+    compute                   256     FAILED      6:0
23766.batch       batch                              128     FAILED      6:0
23766.extern     extern                              256  COMPLETED      0:0
23766.0      test-t5x.+                              256     FAILED      6:0

Our job did successfully retrieved this information, but did not convert it into the status of the job step:

SLRUM Job 23766 finished.
SLURM Job state is FAILED
SLURM Job exit code is 6

hemildesai · 2023-12-07T18:42:51Z

@yhtang This is because the status is retrieved after the job step is complete. Not sure if there's a way to retroactively set the job status. Either ways, I think the exit code of individual jobs is probably outside the scope of this PR. I'm happy to look into it and work on it in a separate PR (to prevent this PR from getting too verbose)

hemildesai force-pushed the hemil/fix-badge-mgmn-tests branch from 5cd10b8 to 0a2958b Compare November 29, 2023 21:49

terrykong requested review from yhtang and terrykong November 29, 2023 22:59

hemildesai marked this pull request as ready for review November 29, 2023 23:18

yhtang requested changes Nov 30, 2023

View reviewed changes

hemildesai force-pushed the hemil/fix-badge-mgmn-tests branch 3 times, most recently from df917d4 to a4c594a Compare November 30, 2023 22:33

hemildesai added 20 commits December 1, 2023 14:10

Migrate to sitrep mechanism for T5X MGMN tests

6d709be

Fix missing output file

187cee2

Fix missing output dir

3328f46

Upload artifacts

b0fdebf

bash fix

6263408

Download artifacts in sitrep

ba4f05e

Source to_json

cf6574b

Checkout repo

325277e

change order

31b5809

bash fix

dfd1a34

Write output at end

9d42afb

Fix sitrep output

fcaba7f

Finalize

550e490

Fix message and summary

4dcf31b

Publish badge

4dbe474

Remove publish-completion

8d73989

Refactor sitrep generation

da3f66b

Comment out publish

f882f2f

Change name

cd90c6a

Revert changes to jobs

7af36b0

yhtang mentioned this pull request Dec 4, 2023

Adds test cases for t5x finetuning+eval with and without TE #359

Open

PR feedback

40aebf5

hemildesai added 4 commits December 5, 2023 10:57

Merge branch 'main' into hemil/fix-badge-mgmn-tests

276ac33

Fix

ae25d1e

Extract out FW name

21cf73b

Fix indent

00d0e00

hemildesai requested a review from ashors1 December 5, 2023 19:14

ashors1 reviewed Dec 5, 2023

View reviewed changes

.github/workflows/_sitrep_mgmn.yaml Outdated Show resolved Hide resolved

hemildesai added 3 commits December 5, 2023 12:20

Use correct field in metrics summary

827a6ce

Merge branch 'main' into hemil/fix-badge-mgmn-tests

c89367d

Fix

5f9f401

yhtang added 2 commits December 7, 2023 02:31

Merge branch 'main' into hemil/fix-badge-mgmn-tests

70bea03

simplify job dependency and conditional execution

9f5d230

yhtang previously approved these changes Dec 7, 2023

View reviewed changes

fix typo

339a228

yhtang dismissed their stale review via 339a228 December 7, 2023 03:20

Merge branch 'main' into hemil/fix-badge-mgmn-tests

b496f90

Merge branch 'main' into hemil/fix-badge-mgmn-tests

1e2389e

yhtang approved these changes Dec 8, 2023

View reviewed changes

yhtang merged commit fe2b422 into main Dec 8, 2023
96 of 99 checks passed

yhtang deleted the hemil/fix-badge-mgmn-tests branch December 8, 2023 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to sitrep mechanism for T5X and PAXML MGMN tests #401

Migrate to sitrep mechanism for T5X and PAXML MGMN tests #401

hemildesai commented Nov 29, 2023 •

edited by yhtang

Loading

terrykong commented Nov 30, 2023

yhtang left a comment

yhtang Nov 30, 2023 •

edited

Loading

hemildesai Nov 30, 2023

hemildesai Nov 30, 2023

yhtang Dec 1, 2023

hemildesai Dec 1, 2023

yhtang Dec 4, 2023

yhtang Dec 4, 2023 •

edited

Loading

hemildesai Dec 4, 2023

hemildesai Dec 4, 2023 •

edited

Loading

yhtang Nov 30, 2023 •

edited

Loading

hemildesai Nov 30, 2023

yhtang Dec 1, 2023

hemildesai commented Nov 30, 2023

hemildesai commented Dec 4, 2023

yhtang commented Dec 6, 2023

hmonishN commented Dec 6, 2023

hemildesai commented Dec 6, 2023 •

edited

Loading

yhtang commented Dec 7, 2023

yhtang commented Dec 7, 2023

hemildesai commented Dec 7, 2023

yhtang commented Dec 7, 2023 •

edited

Loading

hemildesai commented Dec 7, 2023

Migrate to sitrep mechanism for T5X and PAXML MGMN tests #401

Migrate to sitrep mechanism for T5X and PAXML MGMN tests #401

Conversation

hemildesai commented Nov 29, 2023 • edited by yhtang Loading

Changes

TODO

terrykong commented Nov 30, 2023

yhtang left a comment

Choose a reason for hiding this comment

yhtang Nov 30, 2023 • 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

yhtang Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemildesai Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

yhtang Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemildesai commented Nov 30, 2023

hemildesai commented Dec 4, 2023

yhtang commented Dec 6, 2023

hmonishN commented Dec 6, 2023

hemildesai commented Dec 6, 2023 • edited Loading

yhtang commented Dec 7, 2023

yhtang commented Dec 7, 2023

hemildesai commented Dec 7, 2023

yhtang commented Dec 7, 2023 • edited Loading

hemildesai commented Dec 7, 2023

hemildesai commented Nov 29, 2023 •

edited by yhtang

Loading

yhtang Nov 30, 2023 •

edited

Loading

yhtang Dec 4, 2023 •

edited

Loading

hemildesai Dec 4, 2023 •

edited

Loading

yhtang Nov 30, 2023 •

edited

Loading

hemildesai commented Dec 6, 2023 •

edited

Loading

yhtang commented Dec 7, 2023 •

edited

Loading