Repository navigation
test(3d): Paint 2.1 UNet reference lane on CUDA, with the GPU numbers - #788
Conversation
…BACKEND, HARTSY_PAINT21_HP); GPU numbers in the parity doc
|
@claude review PR |
|
Claude finished @kalebbroo's task in 56s —— View job PR review: Paint 2.1 UNet reference lane on CUDA
Overall this is a small, clean change. Assertions only run on the CPU, logging on the GPU doesn't stop at the first failing stage, and adding the worst relRMS of the unlogged blocks helps. One problem affects the doc row as written, and there are a few smaller items. 1.
|
…estores it, asserts a loose bound at the end, compares the backend name case-insensitively
|
@claude review PR |
|
Claude finished @kalebbroo's task in 57s —— View job PR review: Paint 2.1 UNet reference lane on CUDA (re-review after
|
| # | Point | Status |
|---|---|---|
| 1 | ExplicitPath was ignored because it was set after the first knob read |
✅ Fixed in code. KnobScope sets the path and calls KnobFile.Reload(). It is created before Backend(), so CudaBackend now reads NoTf32/SdpaNoTf32 from the file. |
| 2 | On a GPU the test could not fail | ✅ Fixed. Failures are collected in _gpuFailures and AssertGpuBound() checks them at the end of both tests (relRMS < 1e-2, corr > 0.999). |
| 3 | The settings path stayed set after the test | ✅ Fixed. Dispose restores the old path and reloads. using declarations dispose in reverse order, so the backend is disposed before the knobs are restored. |
| 4 | Case-sensitive "cpu" check, duplicate env read |
✅ Fixed (BackendName, OrdinalIgnoreCase). |
I also checked that nothing in the Paint model reads knobs during LoadWeights (no knob reads under Models/Hunyuan3D/*Paint*). Creating KnobScope after the weights load is therefore fine.
1. The doc row still reports the old settings-file result (needs fixing)
docs/Checklists/PARITY_VERIFICATION.md:267 still says:
A settings file (
profile: reference,numerics.noTf32) did not change the lane's flags or numbers, so only theHighPrecisionGemmrun is a second configuration; attention stayed TF32.
The new commit shows why: the file was never applied. So the sentence describes a test-harness bug, not something the engine does. The PR body has the same claim, and so does the sentence "the remaining difference is probably F16 activations". Neither can be supported until the HighPrecisionGemm + numerics.sdpaNoTf32 (or profile: reference) configuration has actually run. That is the configuration that took Sonata from 3.6e-2 to 7.1e-5.
You have two options:
- Rerun with
HARTSY_PAINT21_SETTINGSpointing at areferenceprofile file, check that the backend banner showsTf32Gemm=False Tf32Sdpa=False, and add that row. - Or reword the row as follows until you have the numbers: "the settings-file runs did not apply the file (a harness bug, fixed); TF32 attention has not been measured off". Also remove the F16-activations guess.
2. OnGpu comes from the env var, not from the backend that was created (minor)
BackendFactory.Create also accepts auto, cuda:N and selectors with surrounding spaces (it calls Trim().ToLowerInvariant(), BackendFactory.cs:384). With HARTSY_PAINT21_BACKEND=auto on a machine without a GPU, Create returns a CpuBackend. OnGpu is still true, so the CPU run silently drops to the loose 1e-2 bound. The same thing happens with " cpu". Basing the check on the backend itself avoids this, for example by setting a field _onGpu = backend is not CpuBackend right after Backend().
3. KnobFile.Reload() is global and clears every override (worth knowing)
Reload() calls KnobStore.ResetOverrides() (KnobFile.cs:94). The constructor and Dispose therefore both clear all programmatic overrides in the process, not only the ones from the file. The test project has no DisableTestParallelization, and only the Trellis2 tests share a collection. If another test class sets a knob while this lane runs in parallel, it would lose its override, or would see reference values. This only happens when HARTSY_PAINT21_SETTINGS is set, which is a manual GPU run, so it's acceptable. A short note in the KnobScope doc comment saying "run alone or with -parallel none" would be enough.
KnobProfileScope (used in KnobStore.Resolve, KnobStore.cs:65) might let you scope the reference profile without touching global state. However, CudaBackend reads the knobs at construction, so it only helps if the backend is created inside that scope. Not required.
Nits
- The
<summary>onBackend()says "Create it inside aKnobScope, which the knobs are read through". "Create it inside aKnobScopeso the backend reads the scoped knobs at construction" would be clearer. - The PR body's "Not verified" list could add "TF32-off attention on the GPU" until the rerun.
Summary
All four code points from the first review are fixed correctly. The only remaining blocker is the documentation: the doc row and the PR body still say the settings file had no effect, but the new commit shows it was never applied. Either rerun with HighPrecisionGemm and sdpaNoTf32 and record the numbers, or reword the row. #2 and #3 are small robustness points.
I didn't build or run the tests: this runner has no GPU and no weights.
…the same 7.8e-4, TF32 is not the limit
|
@claude review PR |
|
Claude finished @kalebbroo's task in 41s —— View job PR review: Paint 2.1 UNet reference lane on CUDA (third pass, after
|
| Point | Status |
|---|---|
ExplicitPath set after the knob file had loaded |
✅ Fixed (9c180d2): KnobScope sets the path and calls Reload() |
| On a GPU the test could not fail | ✅ Fixed: _gpuFailures plus AssertGpuBound() at the end of both tests |
| The settings path stayed set after the test | ✅ Fixed: Dispose restores the path and reloads |
Case-sensitive "cpu" check, duplicate env read |
✅ Fixed |
| Doc row reported a result from the harness bug | ✅ Fixed (766f83e): rerun, and the row is rewritten with the new numbers |
OnGpu comes from the env var, not from the backend that was created |
⏳ Still open (minor, see 1) |
KnobFile.Reload() clears every override in the process |
⏳ Still open (just needs a comment, see 2) |
1. OnGpu should come from the backend (minor)
HunyuanPaintUnet21ReferenceTests.cs:39: when HARTSY_PAINT21_BACKEND=auto on a machine with no GPU, BackendFactory returns a CPU backend. OnGpu is still true, so a CPU run gets only the loose 1e-2 bound. IBackend.Device already exists, so the check can use the backend that was actually created:
using IBackend backend = Backend();
_onGpu = backend.Device.Type != DeviceType.Cpu;Then Report and the else branch for the reference features would read _onGpu.
2. KnobScope changes global state (doc note only)
Reload() calls KnobStore.ResetOverrides(), so creating and disposing the scope clears every programmatic knob override in the process. This only matters when HARTSY_PAINT21_SETTINGS is set, which is a manual GPU run. I'd add one line to the KnobScope summary, such as "clears all knob overrides; run this lane alone (-parallel none)".
3. The "F16 weights and activations" explanation is a guess (wording)
The doc row and PR body say the remaining gap to the CPU (7.8e-4 vs 4.9e-4) is "probably F16 weights and activations". Both lanes load the same pickle tensors, and I found no F16 path in Models/Hunyuan3D/Paint/HunyuanPaintUnet.cs. If there is a difference in weight precision, it must come from how the CUDA backend stores or converts the matrices, and nothing in this PR shows that. The row already says "probably", so this is acceptable. If you want it to be exact, write "not yet attributed (TF32 ruled out)", or name the CUDA-side conversion if you know where it is.
Nits
- The
Backend()summary still says "Create it inside aKnobScope, which the knobs are read through". Clearer: "…so the backend reads the scoped knobs at construction." - The PR body table has a "Reference features … HP" cell ("up to 4.1e-4, one block logged"), but the doc row doesn't give the HP feature number. That's fine, just slightly inconsistent.
Summary
All the code issues from the first review and the doc blocker from the second are fixed, and the TF32-off rerun gives a clear answer: TF32 is not what limits the UNet. Point 1 is a small robustness fix. Points 2 and 3 are wording. Nothing blocks the merge.
I didn't build or run the tests: this runner has no GPU and no weights.
What
The Paint 2.1 UNet reference lane (
HunyuanPaintUnet21ReferenceTests) can run on a GPU:HARTSY_PAINT21_BACKEND=cuda(orvulkan) picks the backend,HARTSY_PAINT21_HP=1turns on full-precision GEMMs,HARTSY_PAINT21_SETTINGSpoints at a knob file. On a GPU the stages only log their numbers (a stage past the CPU tolerance does not hide the later ones); on the CPU the assertions are unchanged. The test project references the Engine forBackendFactory.Result (RTX 3060, fixture
dump_paint21_unet.py, 6 views x 2 materials, 16 x 16 latents)HighPrecisionGemmAll correlations 0.999999 to 1.000000, inside the 5e-3 tolerance of the CPU lane. Exact attention changes nothing:
HighPrecisionGemmplusnumerics.sdpaNoTf32, and withnumerics.noTf32as well (flags loggedTf32Gemm=False Tf32Sdpa=False), give the same 7.8e-4 / 5.3e-4. So TF32 does not limit the UNet the way it limits Sonata, no exact-GEMM default is needed, and the remaining gap to the CPU (4.9e-4) is probably F16 weights and activations. Peak VRAM 5.6 to 6.1 GB. The lane sets and reloads the knob file (the first attempt set it after the file had loaded, which Claude's review caught) and restores it afterwards.Not verified
UniPC sampler, guidance, bake, the VAE on the GPU, a full run.