Skip to content

test(3d): Paint 2.1 UNet reference lane on CUDA, with the GPU numbers - #788

Merged
kalebbroo merged 3 commits into
mainfrom
feat/paint21-gpu-lane
Nov 12, 2026
Merged

kalebbroo merged 3 commits into
mainfrom
feat/paint21-gpu-lane

Conversation

@kalebbroo

@kalebbroo kalebbroo commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

What

The Paint 2.1 UNet reference lane (HunyuanPaintUnet21ReferenceTests) can run on a GPU: HARTSY_PAINT21_BACKEND=cuda (or vulkan) picks the backend, HARTSY_PAINT21_HP=1 turns on full-precision GEMMs, HARTSY_PAINT21_SETTINGS points 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 for BackendFactory.

Result (RTX 3060, fixture dump_paint21_unet.py, 6 views x 2 materials, 16 x 16 latents)

Stage CPU (doc) CUDA, default TF32 CUDA, HighPrecisionGemm
UNet full branch, relRMS 4.9e-4 1.4e-3 7.8e-4
UNet unconditional, relRMS 3.2e-4 9.6e-4 5.3e-4
DINO projection, relRMS 3e-7 2.1e-4 3.5e-7
Reference features, relRMS 1e-6 to 4e-4 up to 1.7e-3 up to 4.1e-4 (one block logged)

All correlations 0.999999 to 1.000000, inside the 5e-3 tolerance of the CPU lane. Exact attention changes nothing: HighPrecisionGemm plus numerics.sdpaNoTf32, and with numerics.noTf32 as well (flags logged Tf32Gemm=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.

…BACKEND, HARTSY_PAINT21_HP); GPU numbers in the parity doc
@kalebbroo

Copy link
Copy Markdown
Contributor Author

@claude review PR

@claude

claude Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Claude finished @kalebbroo's task in 56s —— View job


PR review: Paint 2.1 UNet reference lane on CUDA

  • Read the diff
  • Compare against KnobFile loading and the CUDA backend construction
  • Post review

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. HARTSY_PAINT21_SETTINGS probably never took effect (affects the doc claim)

HunyuanPaintUnet21ReferenceTests.cs:42-43 sets KnobFile.ExplicitPath inside Backend(). In Unet_MatchesUpstream, Backend() runs at line 83. By then the test has already loaded the pickle, built two HunyuanPaintUnets and allocated tensors. In an xUnit process, other tests may also have run first.

KnobFile loads once, on the first knob read anywhere (KnobStore → KnobFile.EnsureLoaded(), KnobFile.cs:55-84). Once _loaded is true, a later ExplicitPath is ignored. The setter's own doc comment says so (KnobFile.cs:33: "Must be set before the first knob is read or set"). CudaBackend reads NoTf32 and SdpaNoTf32 at construction (CudaBackend.cs:1368,1374). If the file had loaded, profile: reference would have turned off both the TF32 GEMMs and TF32 SDPA, and the backend banner would have shown Tf32Gemm=False Tf32Sdpa=False.

So the observation in the PR body and in PARITY_VERIFICATION.md:267 ("A settings file … did not change the lane's flags or numbers … attention stayed TF32") most likely comes from the test harness, not from the engine. As a result, the configuration that fixed Sonata (HighPrecisionGemm + numerics.sdpaNoTf32) has not been measured for the UNet. Remaining 7.8e-4 vs 4.9e-4 may be TF32 attention rather than "probably F16 activations".

Suggested fix: reload after setting the path, then rerun the HP + sdpaNoTf32 configuration and update the row.

if (Environment.GetEnvironmentVariable("HARTSY_PAINT21_SETTINGS") is { Length: > 0 } settings)
{
    KnobFile.ExplicitPath = settings;
    KnobFile.Reload(); // the file may already have loaded on an earlier knob read
}

Until that rerun, the doc sentence should say the settings file was not applied, not that it had no effect.

Fix this →

2. On a GPU the test cannot fail

With OnGpu, every numeric check is skipped. Only Assert.Equal(fixtureBlocks, compared) and the shape checks remain, so a CUDA run with completely wrong output still passes Unet_MatchesUpstream. Logging every stage is the right call. Consider also collecting the per-stage results and asserting a loose GPU bound at the end (for example corr > 0.999, relRMS < 1e-2). The run would then still log everything but fail on a real regression. Your measured numbers clear that bound easily.

3. The settings path stays set after the test

KnobFile.ExplicitPath is process-global and is never restored. Any later test in the same xUnit run that reads knobs will use the Paint settings file (after #1, also its values via Reload). This only matters when the env var is set, but restoring the old path and calling Reload() in a finally/Dispose would keep the lane self-contained.

4. Minor

  • OnGpu (line 38) compares name != "cpu" case-sensitively. HARTSY_PAINT21_BACKEND=CPU would silently skip the assertions if BackendFactory accepts that spelling. Use !string.Equals(name, "cpu", StringComparison.OrdinalIgnoreCase).
  • Backend() reads HARTSY_PAINT21_BACKEND again instead of sharing one helper with OnGpu. A small static string BackendName would remove the duplication.
  • Using order: using HartsyInference.Engine; sits between two HartsyInference.Core.* usings.
  • VoxelIndicesAndVae_MatchUpstream now also follows the env var. That's fine, and the doc correctly lists the GPU VAE as not compared. A GPU run of that test also skips its VAE assertion, so the same point as Performance grind #2 applies.
  • The test project now references HartsyInference.Engine only for BackendFactory. That's acceptable for an integration lane; just noting the new dependency.

Summary

The code change is sound. The main issue is that the "settings file had no effect / attention stayed TF32" finding is very likely caused by setting ExplicitPath after the knob file had already loaded. Fix #1 and rerun with reference/sdpaNoTf32 before the doc row presents that as a result. #2 is a suggestion. The rest are nits.

I didn't build or run the tests here: there's no GPU or weights on this runner.

…estores it, asserts a loose bound at the end, compares the backend name case-insensitively
@kalebbroo

Copy link
Copy Markdown
Contributor Author

@claude review PR

@claude

claude Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Claude finished @kalebbroo's task in 57s —— View job


PR review: Paint 2.1 UNet reference lane on CUDA (re-review after 9c180d2)

  • Read the diff, including the new commit 9c180d2
  • Check the earlier review points against the code (KnobFile, KnobStore, BackendFactory)
  • Post review

9c180d2 fixes the code issues from the first review. The doc row and the PR body still describe the old behaviour, so they are now wrong.

Earlier points: status

# 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 the HighPrecisionGemm run 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_SETTINGS pointing at a reference profile file, check that the backend banner shows Tf32Gemm=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.

Fix this →

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> on Backend() says "Create it inside a KnobScope, which the knobs are read through". "Create it inside a KnobScope so 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.

@kalebbroo

Copy link
Copy Markdown
Contributor Author

@claude review PR

@claude

claude Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Claude finished @kalebbroo's task in 41s —— View job


PR review: Paint 2.1 UNet reference lane on CUDA (third pass, after 766f83e)

  • Read the diff, including the new commit 766f83e
  • Check the open points from the second review
  • Post review

766f83e resolves the blocker from the last review. You reran with the knob file applied (the banner shows Tf32Gemm=False Tf32Sdpa=False). The doc row now reports that measurement, and the incorrect "settings file had no effect / attention stayed TF32" sentence is gone. The result is also useful: unlike Sonata, the UNet stays at 7.8e-4 / 5.3e-4 with exact attention. This is ready to merge. What's left is optional.

Status of earlier points

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.

Fix this →

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 a KnobScope, 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.

@kalebbroo
kalebbroo merged commit fe6be4a into main Nov 12, 2026
2 checks passed
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.

1 participant