Repository navigation
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 781f7019b6
ℹ️ 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".
| if cu_device.is_event_tracking() { | ||
| unsafe { cu_device.disable_event_tracking() }; |
There was a problem hiding this comment.
When the supplied device shares its context with another CUDA stream, this permanently disables cudarc's cross-stream synchronization for every subsequent operation on that context. Graph::new is a safe public API and cannot enforce the comment's single-stream assumption; work submitted on another stream can therefore race with graph inputs or outputs, even though replay synchronizes only after launching. Use an isolated context/stream strategy or otherwise preserve synchronization rather than globally disabling tracking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3635186, refined in 38e9d25.
The pause is now an RAII guard scoped to the capture and restored on drop, including the
error paths out of from_code/end_capture, so the caller's context is left exactly as it
was found. cudarc exposes CudaContext::enable_event_tracking, which makes the restore
possible (candle-core only re-exports disable_event_tracking).
On the "isolated context/stream" suggestion: an isolated stream would not help, since the
flag lives on the CudaContext — is_managing_stream_synchronization() is
is_in_multi_stream_mode() && is_event_tracking(), both context-wide. An isolated context
would mean the graph could not touch the caller's tensors at all, since candle tensors are
bound to their device's context. So pausing and restoring is the option actually available
here.
Validated on a self-hosted CUDA runner:
https://github.com/astorise/candle_graphs/actions/runs/32158143087
| DType::I64 => memcpy_dtod::<i64>(tgt, src, &stream)?, | ||
| DType::U32 => memcpy_dtod::<u32>(tgt, src, &stream)?, | ||
| DType::U8 => memcpy_dtod::<u8>(tgt, src, &stream)?, | ||
| dtype => anyhow::bail!("copy_inplace: unsupported dtype {dtype:?}"), |
There was a problem hiding this comment.
With Candle 0.11, DType::I16 and DType::I32 are valid CUDA tensor types, but this wildcard rejects both. Consequently, the public copy_inplace function—and any derived or map-based GraphInput using either dtype—fails every replay before the graph launches, despite the replay API documenting only matching, contiguous inputs. Add typed copy arms for these new integer variants or reject them when constructing the graph and document the restriction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3635186. copy_inplace now has DType::I16 and DType::I32 arms.
The same gap existed one level down and is fixed too: update_kv.cu has always shipped
copy2d_i16/copy2d_i32, but extension.rs's dispatch never wired them, so
slice_set_fingerprinted would have failed on those dtypes as well. It was unreachable
under candle 0.8, whose DType had neither variant, and became reachable in 0.11.
The remaining wildcard now covers only the fp8 types and errors explicitly rather than
silently leaving the input un-copied.
| - run: apt update && apt install curl build-essential libssl-dev pkg-config -y | ||
| - uses: dtolnay/rust-toolchain@29eef336d9b2848a0b548edc03f92a220660cdb8 # stable | ||
| - run: cargo test -p candle_graph | ||
| - run: cargo run --example decode_replay |
There was a problem hiding this comment.
Any manual dispatch reaches a guaranteed Cargo error here: local cargo run --help defines --example <NAME> as selecting the named example target, while workspace metadata lists only attention, cache, matmuls, model, and simple_matmul; a repo-wide search also finds no decode_replay target or source. Point this step at an existing verification example or add the intended example so the temporary GPU workflow can complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3635186 by deleting the file — it should never have been in this PR.
It was temporary GPU-verification scaffolding for a different branch that leaked in from my
fork's default branch, which this branch was cut from. The diff against upstream master now
contains no .github/ changes at all.
| match (&src.slice, &dst.slice) { | ||
| (S::U8(s), S::U8(d)) => { | ||
| builder.arg(s); | ||
| builder.arg(d); |
There was a problem hiding this comment.
When slice_set_fingerprinted is used outside a graph with multiple CUDA streams, every dtype arm passes d as an immutable &CudaSlice, so cudarc's new argument tracking records the destination as another read rather than as the write performed by this kernel. A later read of the cache on another stream therefore need not wait for this mutation and can observe stale or partially written data; obtain mutable destination access and push it as a mutable/write kernel argument.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged, and not changed here — this one is not reachable from an external crate with
candle-core 0.11's public API.
Pushing the destination as a write needs &mut CudaSlice, which needs &mut CudaStorage.
candle gets one internally: Tensor::slice_set does
src.storage().copy2d(&mut self.storage_mut(), ...)
but Tensor::storage_mut() is pub(crate) (candle-core/src/tensor.rs:2736), so a downstream
crate cannot obtain mutable storage access at all. That is the blocker, not a design choice on
my side.
Worth adding that even with that &mut, candle's own copy2d then routes the destination
through slice_ptr -> device_ptr() — the immutable path — so the write is not registered as
a write there either (candle-core/src/cuda_backend/mod.rs, fn copy2d and fn slice_ptr).
This crate therefore matches upstream candle's current behaviour rather than diverging from it.
Fixing it properly would need candle to expose mutable storage access, or this crate to stop
mutating through a shared reference (which is why copy_inplace is unsafe). Happy to take
it on separately if you want it pursued — it is orthogonal to the dependency update.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36351860be
ℹ️ 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".
| let restore = ctx.is_event_tracking(); | ||
| if restore { | ||
| // SAFETY: the caller takes over cross-stream synchronization. Everything this | ||
| // crate does is confined to one stream -- capture, replay and `copy_inplace` all | ||
| // use the device's own stream, and `replay` synchronizes the device afterwards -- | ||
| // so there is no second stream to race against for the duration of the pause. | ||
| unsafe { ctx.disable_event_tracking() }; |
There was a problem hiding this comment.
When two Graph::new calls overlap on streams sharing this CudaContext, the first guard disables tracking, the second observes it already disabled and therefore does not plan a restore, and the first can re-enable tracking while the second is still capturing. Subsequent kernel arguments in that capture may then emit cudarc event waits and fail capture, while unrelated work on the context can also run without its normal cross-stream dependencies during the pause. The new RAII restoration fixes the previously permanent state change but does not make the context-wide toggle exclusive; serialize capture for the context or use an isolated context/stream mechanism.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 38e9d25. Good catch — the read-then-write against the live flag was a real race.
Pauses are now reference counted per context, behind a Mutex, so the check and the flag
write happen as one step: the first pause disables tracking and records the original value,
the last one restores it. Overlapping captures therefore keep tracking off for the whole
overlap instead of one restoring it under another. The map is keyed by Arc::as_ptr on the
CudaContext so a pause on one device's context does not hold another's paused. A poisoned
lock is recovered from rather than propagated, since the counter is still consistent.
To be straight about the evidence: CI here is single-threaded, so the run below shows no
regression rather than exercising the race. The concurrency argument is by construction, not
by test.
Validated on a self-hosted CUDA runner:
https://github.com/astorise/candle_graphs/actions/runs/32163575151
Moves candle-core/candle-nn from 0.8.0 to 0.11.0, which brings cudarc
0.12.1 -> 0.19.9. cache.rs and extension.rs are ported rather than
deleted, and Graph::drop keeps launching a never-replayed graph once
before destroying it.
graph.rs / node.rs: the raw driver calls move from
`driver::sys::lib().cuXxx(...)` to the `sys::cuXxx(...)` free functions,
and the graph lifecycle goes through the `result::stream::{begin,end}_capture`
and `result::graph::{instantiate,launch,destroy}` wrappers. Streams become
`Arc<CudaStream>`.
extension.rs: cudarc replaced `LaunchAsync::launch(cfg, params)` with the
`CudaFunc::builder()` + `PushKernelArg::arg()` builder, which also removes
the manual device-pointer extraction. `get_or_load_func` no longer takes
PTX source -- `kernels::Module` is a closed type whose index only covers
candle-core's own kernel families -- so custom kernels go through
`get_or_load_custom_func(fn_name, module_name, ptx)`.
Event tracking: cudarc >= 0.19 attaches CUDA events to every CudaSlice and
emits cuStreamWaitEvent/cuEventRecord around each launch while
`is_managing_stream_synchronization()` holds. `Device::new_cuda_with_stream`
goes through `CudaContext::new_stream()`, which is what puts a context into
multi-stream mode, so every launch inside a capture waited on an event
recorded outside the graph being captured and capture failed with
CUDA_ERROR_INVALID_VALUE. Capture now runs under an RAII pause that restores
the context's previous setting on drop, including the error paths. The pause
is reference counted per context so overlapping captures cannot restore
tracking under one another, and so a pause on one device does not hold
another's.
dtypes: DType is #[non_exhaustive] and gained I16/I32/F8E4M3. copy_inplace
handles I16/I32 and errors explicitly on the rest instead of silently
skipping the copy; extension.rs's copy2d dispatch gains the matching arms
(update_kv.cu already shipped copy2d_i16/copy2d_i32, but they were
unreachable under 0.8, whose DType had neither).
Also fixes src_o/dst_o, which were passed as usize where the kernel
declares uint32_t.
Verified on CUDA hardware: cargo test -p candle_graph and
cargo run --example cache both pass.
38e9d25 to
ee8fe19
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee8fe1943a
ℹ️ 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".
| // crate does is confined to one stream -- capture, replay and `copy_inplace` | ||
| // all use the device's own stream, and `replay` synchronizes the device | ||
| // afterwards -- so there is no second stream of ours to race against. | ||
| unsafe { ctx.disable_event_tracking() }; |
There was a problem hiding this comment.
When an input tensor was last written on another CudaStream sharing this context, disabling event tracking here prevents cudarc from inserting the producer-event wait when from_code captures its use; the earlier device.synchronize() drains only the graph device's stream, so the first graph launch can read unfinished input. Fresh evidence after the prior fix is that the final reference-counted implementation protects only the pause depth and still performs no producer synchronization. Synchronize the relevant streams/events before this toggle, or enforce that captured tensors belong exclusively to the graph stream.
Useful? React with 👍 / 👎.
Updates
candle-core/candle-nnfrom 0.8.0 to 0.11.0 (cudarc 0.12.1 -> 0.19.9).Unlike #1,
cache.rsandextension.rsare ported rather than deleted, andGraph::dropkeeps its existing behaviour of launching a never-replayed graph once before destroying it.
Changes
graph.rs/node.rs— raw driver calls move fromdriver::sys::lib().cuXxx(...)to thesys::cuXxx(...)free functions, and the graph lifecycle now goes through theresult::stream::{begin_capture,end_capture}/result::graph::{instantiate,launch,destroy}wrappers. Streams become
Arc<CudaStream>.extension.rs— cudarc droppedLaunchAsync::launch(cfg, params)in favour of theCudaFunc::builder()+PushKernelArg::arg()builder, which also removes the need to extractraw device pointers by hand. Also fixes
src_o/dst_o, which were passed asusizewhile thekernel declares
uint32_t.get_or_load_func->get_or_load_custom_func—kernels::Moduleis now a closed type(private fields, no public constructor) whose index only covers candle-core's own 11 built-in
kernel families.
get_or_load_custom_func(fn_name, module_name, ptx)is the intended entrypoint for external kernels such as
update_kv.ptx.DType— now#[non_exhaustive]and extended withI16/I32/F8E4M3;copy_inplacegains a wildcard arm that errors explicitly instead of mishandling an unsupported dtype.
CudaSliceand emitscuStreamWaitEvent/cuEventRecordaround each launch whileis_managing_stream_synchronization()holds.Device::new_cuda_with_streamgoes throughCudaContext::new_stream(), which is precisely what puts a context into multi-stream mode, soevery launch inside a capture was waiting on an event recorded outside the graph being
captured (
CUDA_ERROR_INVALID_VALUE).Graph::newnow callsCudaDevice::disable_event_tracking(), which candle-core exposes for this case. Its safetycontract holds here: capture, replay and
copy_inplaceall usecu_device.cuda_stream(), andreplaysynchronizes the device after each launch.Testing
Validated on a self-hosted CUDA runner:
cargo test -p candle_graphandcargo run --example cacheboth pass.