Skip to content

Add a loss comparison script #2029

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

Merged
fegin merged 15 commits into from
Nov 19, 2025
Merged

Add a loss comparison script #2029

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
546847a
Update (base update)
fegin Nov 12, 2025
dd979ef
Update
fegin Nov 12, 2025
0e78d18
Update (base update)
fegin Nov 12, 2025
4b4e547
Update
fegin Nov 12, 2025
fa43b28
Update (base update)
fegin Nov 12, 2025
4f2b8bd
Update
fegin Nov 12, 2025
cbec465
Update (base update)
fegin Nov 13, 2025
74f9fb7
Update
fegin Nov 13, 2025
0cfde37
Update (base update)
fegin Nov 17, 2025
9bb1b33
Update
fegin Nov 17, 2025
e393a13
Update
fegin Nov 17, 2025
a53269f
Update
fegin Nov 17, 2025
88f37f6
Update
fegin Nov 19, 2025
67708b9
Update
fegin Nov 19, 2025
1425d8a
Update
fegin Nov 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/integration_test_8gpu_features.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,11 @@ jobs:
export TEST_WITH_ROCM=$([[ "${{ matrix.gpu-arch-type }}" == "rocm" ]] && echo 1 || echo 0)
python -m tests.integration_tests.run_tests --test_suite features $RUNNER_TEMP/artifacts-to-be-uploaded --ngpu 8

# Verify the accuracy.
export baseline_options="--parallelism.data_parallel_replicate_degree=1"
export test_options="--parallelism.data_parallel_replicate_degree=4"
Copy link
Copy Markdown
Contributor

@tianyu-l tianyu-l Nov 18, 2025

Choose a reason for hiding this comment

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

Why comparing these two? FSDP 8 vs. HSDP 4, 2

Also we are not using the assert_equal flag, so what happens if the losses are different? Are you analyzing them with some other scripts?

I thought the idea was we should compare main and PR submit, but for that it may be easier to assert with a fixed loss number.

Copy link
Copy Markdown
Contributor Author

@fegin fegin Nov 18, 2025

Choose a reason for hiding this comment

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

Yes, I should add --assert_equal—I'll update it.

I originally planned to compare the current PR with the main branch. The problem is that CI mounts the folder as read-only, so I couldn't check out the "main" version. I took a step back and decided to verify the minimum guarantees: at least some parallelism variations should have the same losses. Since this check is quick, I think it doesn't hurt to add it (we can also verify TP). This way, we ensure the minimum guarantees and also validate the script.

For comparing with the "main" branch, one way to achieve this is to pre-record the results and add an option to the tool to read them. Both baseline and test runs must match the pre-recorded losses when --assert_equal is set. The downside is that this approach is less stable than simply checking out during CI, since the pre-recorded method can't tolerate machine or library changes.

I prefer to do this in another PR.

Copy link
Copy Markdown
Contributor

@tianyu-l tianyu-l Nov 18, 2025

Choose a reason for hiding this comment

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

since the pre-recorded method can't tolerate machine or library changes.

Good point, but here are two cases if numerics change?

  1. not "error", just differences between machines / libraries
  2. library introduces a bug which causes both main and branch on all parallelisms to be the same and simultaneously incorrect.

I guess we could say the latter should be captured by upstream library?

Copy link
Copy Markdown
Contributor Author

@fegin fegin Nov 19, 2025

Choose a reason for hiding this comment

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

Sure, we can do this. Will do this in another PR.

python3 scripts/loss_compare.py . . --baseline-options="${baseline_options}" --test-options="${test_options}" --steps=10 --job-dump-folder="${RUNNER_TEMP}/artifacts-to-be-uploaded/accuracy_comparison_outputs"

# Cleanup the checkpoints so that we don't waste network bandwidth and time.
rm -rf $RUNNER_TEMP/artifacts-to-be-uploaded/*/checkpoint
rm -rf artifacts-to-be-uploaded/*/checkpoint
Loading
Loading