Give training its own GPU device - #10
Conversation
Adapt upstream PR ArthurBrussee#526 to the local native-MSL, appearance, LOD, metrics, iOS, and dataset paths. Keep validation readbacks switchable and update Burn/CubeCL/Cubek to the PR's pinned revisions.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c13634540e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| connect_device(burn.clone()); | ||
| // A JS host can call `init()` and `initExisting()`, or a dev-mode double | ||
| // mount can re-run setup. Whoever gets there first wins. | ||
| let _ = DEVICE.set(burn.clone().into()); |
There was a problem hiding this comment.
Await registration of the host-provided device
tokio::sync::OnceCell::set is asynchronous, so assigning the returned future to _ drops it without ever storing burn. Consequently every initExisting(...) call appears successful, but the first later device() call opens the internal default device instead; training buffers then belong to that device and cannot be bound by the host WebGPU device as the brush-js API promises. Make this initialization path await set (or otherwise synchronously register the value) before returning success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a false positive for the dependency version pinned by this PR. Cargo.lock resolves Tokio 1.53.1, where tokio::sync::OnceCell::set is synchronous: pub fn set(&self, value: T) -> Result<(), SetError>. Its implementation calls set_value before returning, so the host-provided device is stored before initExisting returns; no future is created or dropped here. The ignored Result only covers an earlier or in-flight initializer winning, consistent with the documented requirement that initExisting be called before anything touches the device. No code change is needed.
Adapt upstream 65f19e0 across fork-only appearance, native-MSL, loss, and benchmark paths. Keep burn-cubecl direct only in brush-cube.
Reject conflicting host-device initialization while preserving the Rust API. Avoid redundant viewer uploads, keep validation active in tests, adopt direct Burn CubeCL ownership, and add browser interoperability coverage.
Summary
65f19e0ada6221b6848bd9cda09b0a8ba83cf250.CubeRuntimeand update the pinned Burn, CubeCL, and Cubek revisions.burn-cubeclsurface inbrush-cube.Fork-specific adaptations
brush-cube.initExistingcalls idempotent.Upstream source: ArthurBrussee#526
Upstream head reviewed:
65f19e0ada6221b6848bd9cda09b0a8ba83cf250Validation
cargo fmt --all -- --checkcargo check --workspace --all-targets --all-featurescargo clippy --workspace --all-targets --all-features -- -D warningscargo check --locked --workspace --no-default-features --all-targetswasm32-unknown-unknownThe ignored tests are the documented large host-Metal appearance dispatch regression (~1.5 GiB peak) and native-Metal launch-geometry soak. The local cargo-deny binary is unavailable; the upstream head cargo-deny check is green.