feat: experimental step plasticity + GPU step options - #45
Conversation
CUDA.jl 6 removed available_memory; EnsembleBrain VRAM report now uses free_memory. Compat CUDA = "6".
Add plasticity modes (:readout_only, :none, :recurrent_stdp), sync/record_history/use_device_noise, lazy STDP edge lists, ensemble kwargs forwarding, and GPU TDD coverage.
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR introduces experimental plasticity features and performance optimizations for the GPU step functions. The implementation is comprehensive with excellent test coverage (71/71 tests passing).
Key Changes
- ✅ Experimental pair STDP on sparse recurrent weights (
:recurrent_stdpmode) - ✅ Lazy edge list materialization (saves ~340 MB/lobe)
- ✅ Work buffer reuse to eliminate per-tick allocations
- ✅ Optional kwargs:
plasticity,sync,record_history,use_device_noise - ✅ CUDA.jl 6.x compatibility (
free_memoryAPI) - ✅ Comprehensive GPU test coverage for all plasticity modes
Review Findings
1 issue requiring attention:
- Critical: Missing bounds validation in CSC edge list parsing could cause crashes with malformed sparse matrices
Recommendation
Address the bounds checking issue before merge. The experimental features are well-designed and properly gated behind explicit kwargs with safe defaults.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Up to standards ✅🟢 Issues
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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: 0adcd17972
ℹ️ 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".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded experimental plasticity and execution controls to sparse and ensemble stepping. Added recurrent pair-STDP, reusable GPU buffers, CUDA.jl 6 compatibility, exception filtering, documentation, repository metadata updates, and GPU test coverage. ChangesExperimental stepping API
Repository support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ensemble_step!
participant SparseBrain
participant CUDA
Caller->>ensemble_step!: pass plasticity and execution controls
ensemble_step!->>SparseBrain: step each lobe with sync=false
SparseBrain->>ensemble_step!: return each lobe readout
ensemble_step!->>CUDA: synchronize once after aggregation when sync=true
ensemble_step!->>Caller: return aggregated readout
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Comment |
coderabbitai
Bot
added
the
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sparse_brain.jl (1)
637-662: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale docstring for
EnsembleBrainstep!wrapper.The docstring signature at Lines 637-638 and its body only describe
inhibition,reflex_eta, andreflex_signal. The actual function signature (Lines 643-651) also accepts and forwardsplasticity,recurrent_eta,sync,record_history, anduse_device_noisetoensemble_step!. The docstring forensemble_step!itself (Lines 571-579) does describe these forwarded kwargs, but this wrapper's docstring was not updated to match.📝 Proposed docstring fix
""" - step!(eb::EnsembleBrain, u; inhibition=0.0, reflex_eta=ETA, reflex_signal=0.0) + step!(eb::EnsembleBrain, u; inhibition=0.0, reflex_eta=ETA, reflex_signal=0.0, plasticity=:readout_only, ...) Forwards to [`ensemble_step!`](`@ref`). Keyword `reflex_signal` (default `0`) controls fast-lobe reflex gating. +Also forwards `plasticity`, `recurrent_eta`, `sync`, `record_history`, and `use_device_noise`. """As per coding guidelines, "Keep docstrings aligned with the actual function signatures."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sparse_brain.jl` around lines 637 - 662, Update the `step!(eb::EnsembleBrain, u::CuVector{Float32})` docstring to document all supported keyword arguments, including `plasticity`, `recurrent_eta`, `sync`, `record_history`, and `use_device_noise`, while retaining the existing descriptions for the reflex-related options.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sparse_brain.jl`:
- Around line 291-298: Validate the plasticity argument at the start of
_step_impl! against the supported symbols (:readout_only, :recurrent_stdp, and
:none), throwing an argument error for any other value before executing update
logic. Keep ensemble_step!, _ensemble_step_impl!, and the EnsembleBrain step!
wrapper forwarding the validated option without duplicating validation.
- Around line 317-321: Update the noise-generation branch in step! to preserve a
portable host-generated fallback: do not unconditionally use Random.randn! when
use_device_noise is enabled. Keep cpu_randn_cu (or the existing host
randn/copyto! path) as the default, and only use device RNG when a compatibility
check confirms it is available on the supported Julia/CUDA stack.
In `@test/runtests.jl`:
- Around line 235-248: Replace the tautological W_out assertion in the “GPU:
plasticity=:readout_only can update W_out” test with a direct check that
Array(brain.W_out) differs from W0. Keep the existing tick_count and output
finiteness assertions unchanged.
---
Outside diff comments:
In `@src/sparse_brain.jl`:
- Around line 637-662: Update the `step!(eb::EnsembleBrain,
u::CuVector{Float32})` docstring to document all supported keyword arguments,
including `plasticity`, `recurrent_eta`, `sync`, `record_history`, and
`use_device_noise`, while retaining the existing descriptions for the
reflex-related options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f86ab190-3aa5-4246-957f-bc4622f736d8
📒 Files selected for processing (6)
.gitignoreAGENTS.mdProject.tomlREADME.mdsrc/sparse_brain.jltest/runtests.jl
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
- Validate plasticity modes; reject typos with ArgumentError - CSC edge-list bounds checks - Skip host spike sum when sync=false; ensemble diags after loop - Device noise falls back to host on failure - Strong W_out Hebbian assertion; README kwargs table clarity
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review Follow-up push
Local GPU suite: 72/72 on Julia 1.12 + CUDA.jl 6.2.1. Note: |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d68752fe2
ℹ️ 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".
Do not report ArgumentError/DimensionMismatch from step! validation to Sentry (avoids noise from unit tests and caller misuse).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a75df8ab3
ℹ️ 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".
Document project-id check so LiquidCortex events do not misroute to the rust Sentry project.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/sparse_brain.jl (3)
76-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKernel/function parameter
nnzshadows the importedSparseArrays.nnz.
_pair_stdp_kernel!'snnz::Int32parameter (Line 78) and the localnnz = Int32(brain.nnz)in_apply_pair_stdp!(Line 284) both shadowSparseArrays.nnz, which is imported viausing SparseArrays(Line 19). This is valid Julia and does not break other call sites, but it makes the code harder to read sincennznormally refers to a function that reports nonzero counts on a sparse matrix. Rename the local/parameter (for exampleedge_count) to avoid the shadowing.Also applies to: 282-293
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sparse_brain.jl` around lines 76 - 90, Rename the nnz parameter in _pair_stdp_kernel! and the local nnz variable in _apply_pair_stdp! to edge_count (or another clear non-shadowing name), updating all references and kernel arguments consistently while preserving the existing edge-count behavior.
328-338: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDefault noise path still allocates and transfers a host array every tick.
When
use_device_noise=false(the default),randn(Float32, N)allocates a new hostVector{Float32}of 65,536 elements andcopyto!transfers it over PCIe on every call to_step_impl!. This runs once per lobe per tick, four times perensemble_step!call. The GPU-side buffer reuse (brain.noise) avoids a device allocation, but the host allocation and transfer remain a per-tick cost on the default, "portable" code path.Consider preallocating a persistent host buffer once (per lobe) and refilling it in place with
randn!(host_noise_buf)beforecopyto!, to eliminate the repeated host allocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sparse_brain.jl` around lines 328 - 338, The default noise path allocates a new host random vector on every _step_impl! call. Add a persistent per-lobe host noise buffer during brain initialization, refill it in place with randn! in the use_device_noise=false branch, and copy it into brain.noise; preserve the existing device RNG path and fallback behavior.
121-122: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
trace_preandtrace_postare always numerically identical.Both traces use the same decay constant
TAU_TRACEand the same inputbrain.S:brain.trace_pre .= brain.trace_pre .* (1.0f0 - DT / TAU_TRACE) .+ brain.S brain.trace_post .= brain.trace_post .* (1.0f0 - DT / TAU_TRACE) .+ brain.SBoth arrays start at zero and receive the identical recurrence every tick, so
trace_preequalstrace_postat all times. The pair-STDP kernel still produces correct results because it indexes each array by role (trace_pre[pre],trace_post[post]), but keeping two identical arrays wastes GPU memory and doubles the per-tick elementwise cost with no functional benefit. Merge them into a singletrace::CuVector{Float32}field, or givetrace_posta distinct time constant if asymmetric causal/anti-causal windows are the intended design.♻️ Proposed fix (single shared trace)
- trace_pre::CuVector{Float32} # Pre-synaptic trace - trace_post::CuVector{Float32} # Post-synaptic trace + trace::CuVector{Float32} # Shared spike-eligibility trace (used for both pre/post roles)- brain.trace_pre .= brain.trace_pre .* (1.0f0 - DT / TAU_TRACE) .+ brain.S - brain.trace_post .= brain.trace_post .* (1.0f0 - DT / TAU_TRACE) .+ brain.S + brain.trace .= brain.trace .* (1.0f0 - DT / TAU_TRACE) .+ brain.S(Update
_apply_pair_stdp!/_pair_stdp_kernel!call sites to passbrain.tracefor bothtrace_preandtrace_postarguments, or drop the duplicate kernel argument entirely.)Also applies to: 371-372
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sparse_brain.jl` around lines 121 - 122, Replace the duplicate trace_pre and trace_post fields with a single trace::CuVector{Float32} in the brain state, and update the trace update logic to compute it only once per tick. Adjust _apply_pair_stdp! and _pair_stdp_kernel! call sites to use brain.trace for both pre- and post-synaptic trace lookups, or remove the redundant kernel argument while preserving role-based indexing.test/runtests.jl (4)
165-165: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRun GPU cleanup in
finallyblocks.If a constructor or CUDA operation raises before these lines,
reclaim_gpu!()is skipped. The largeSparseBrainandEnsembleBrainallocations can then cause later GPU testsets to fail with out-of-memory errors.Initialize each holder to
nothing, wrap the test body intry, and reclaim infinally.Proposed pattern
- brain = SparseBrain(...) - ... - brain = nothing; reclaim_gpu!() + brain = nothing + try + brain = SparseBrain(...) + ... + finally + brain = nothing + reclaim_gpu!() + endThe file comment identifies these cases as large GPU allocations.
Also applies to: 175-175, 184-184, 193-193, 202-202, 211-212, 220-220, 233-233, 247-247, 257-257, 267-267, 278-278, 295-295, 307-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtests.jl` at line 165, Update each affected GPU test in test/runtests.jl to initialize its brain holder to nothing, wrap construction and test operations in try blocks, and move reclaim_gpu!() into finally blocks so cleanup runs even when setup or CUDA operations throw. Apply this consistently to the tests using SparseBrain and EnsembleBrain allocations, preserving their existing assertions and test behavior.
236-246: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that
:readout_onlykeeps recurrentWfrozen.This test captures only
W_outand output. A regression that mutatesbrain.W.nzValunder:readout_onlywould pass.Snapshot
Array(brain.W.nzVal)before the loop and assert equality after the loop.Proposed fix
W0 = copy(Array(brain.W_out)) + W_recurrent0 = copy(Array(brain.W.nzVal)) ... `@test` !all(Array(brain.W_out) .== W0) + `@test` Array(brain.W.nzVal) == W_recurrent0The PR contract states that
:readout_onlypreserves recurrentW.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtests.jl` around lines 236 - 246, Extend the GPU testset for plasticity=:readout_only by snapshotting Array(brain.W.nzVal) before the update loop and asserting it remains equal afterward. Keep the existing W_out-change, tick-count, and finite-output assertions unchanged.
226-230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompare the complete
W_outstate.Line 226 stores only the norm. A different matrix can have the same norm, so Line 230 can pass while
plasticity=:nonestill mutates readout weights.Copy
Array(brain.W_out)before the loop and compare the full matrix afterward.Proposed fix
- w0 = norm(Array(brain.W_out)) + w0 = copy(Array(brain.W_out)) ... - `@test` norm(Array(brain.W_out)) ≈ w0 atol=1e-5 + `@test` Array(brain.W_out) == w0The
:nonetest should prove that the complete readout matrix remains unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtests.jl` around lines 226 - 230, Update the plasticity=:none test around brain.W_out to copy the complete Array(brain.W_out) before the stepping loop, then compare the final matrix directly against that snapshot instead of comparing norms. Preserve the existing loop and tolerance while ensuring the full readout state is verified unchanged.
298-306: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the ensemble forwarding check observable.
The current assertions do not prove that
plasticity=:nonereaches each lobe. Ifensemble_step!drops the keyword, tick counts and finite output can still pass.Add an invalid-mode assertion or compare each lobe's
W_outbefore and after the:nonecall.Minimal verification
u = CUDA.zeros(Float32, 8) + `@test_throws` ArgumentError ensemble_step!(eb, u; plasticity=:typo) ensemble_step!(eb, u; plasticity=:none, inhibition=0.1f0)The PR objective requires
ensemble_step!to forward plasticity keywords.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtests.jl` around lines 298 - 306, Strengthen the GPU testset around ensemble_step! so it directly verifies plasticity=:none reaches every lobe. Capture each lobe’s W_out before the call and assert the weights remain unchanged afterward, or add an invalid-mode assertion that would fail if the keyword were dropped; retain the existing tick-count and output checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/LiquidCortex.jl`:
- Around line 69-77: Update _should_capture_runtime_exception and its callers so
ArgumentError and DimensionMismatch are suppressed only when explicitly marked
as expected validation failures, not based on exception type alone. Ensure the
step! catch path preserves Sentry capture for unexpected internal shape or
invariant errors, and add coverage for both marked validation errors and
unmarked exceptions.
In `@src/sparse_brain.jl`:
- Around line 328-334: Update the device-noise fallback around Random.randn! to
avoid a bare catch: catch only the expected RNG failure type, or log the caught
exception once before falling back to host noise. Preserve the existing copyto!
fallback for supported device RNG failures while allowing unrelated exceptions
such as interrupts or out-of-memory errors to remain visible.
In `@test/runtests.jl`:
- Line 232: Move or add the invalid plasticity-mode validation test outside the
GPU-only branch so CPU-only CI exercises it unconditionally. Update the tests
around step! to validate that plasticity=:typo raises ArgumentError without
requiring CUDA; retain the existing GPU integration coverage, and if step!
itself requires CUDA, test the underlying plasticity validator separately.
---
Outside diff comments:
In `@src/sparse_brain.jl`:
- Around line 76-90: Rename the nnz parameter in _pair_stdp_kernel! and the
local nnz variable in _apply_pair_stdp! to edge_count (or another clear
non-shadowing name), updating all references and kernel arguments consistently
while preserving the existing edge-count behavior.
- Around line 328-338: The default noise path allocates a new host random vector
on every _step_impl! call. Add a persistent per-lobe host noise buffer during
brain initialization, refill it in place with randn! in the
use_device_noise=false branch, and copy it into brain.noise; preserve the
existing device RNG path and fallback behavior.
- Around line 121-122: Replace the duplicate trace_pre and trace_post fields
with a single trace::CuVector{Float32} in the brain state, and update the trace
update logic to compute it only once per tick. Adjust _apply_pair_stdp! and
_pair_stdp_kernel! call sites to use brain.trace for both pre- and post-synaptic
trace lookups, or remove the redundant kernel argument while preserving
role-based indexing.
In `@test/runtests.jl`:
- Line 165: Update each affected GPU test in test/runtests.jl to initialize its
brain holder to nothing, wrap construction and test operations in try blocks,
and move reclaim_gpu!() into finally blocks so cleanup runs even when setup or
CUDA operations throw. Apply this consistently to the tests using SparseBrain
and EnsembleBrain allocations, preserving their existing assertions and test
behavior.
- Around line 236-246: Extend the GPU testset for plasticity=:readout_only by
snapshotting Array(brain.W.nzVal) before the update loop and asserting it
remains equal afterward. Keep the existing W_out-change, tick-count, and
finite-output assertions unchanged.
- Around line 226-230: Update the plasticity=:none test around brain.W_out to
copy the complete Array(brain.W_out) before the stepping loop, then compare the
final matrix directly against that snapshot instead of comparing norms. Preserve
the existing loop and tolerance while ensuring the full readout state is
verified unchanged.
- Around line 298-306: Strengthen the GPU testset around ensemble_step! so it
directly verifies plasticity=:none reaches every lobe. Capture each lobe’s W_out
before the call and assert the weights remain unchanged afterward, or add an
invalid-mode assertion that would fail if the keyword were dropped; retain the
existing tick-count and output checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0add7d3-a760-4fec-ac2f-41539d1e747e
📒 Files selected for processing (4)
README.mdsrc/LiquidCortex.jlsrc/sparse_brain.jltest/runtests.jl
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AGENTS.md (1)
28-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the unqualified “latest” claim.
Project.tomldocuments compatibility with CUDA.jl 6.x, not release recency. UseCUDA.jl \6.x`` or name the exact tested version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` at line 28, Update the CUDA.jl compatibility entry in AGENTS.md to remove the unqualified “latest” claim, using “CUDA.jl 6.x” or the exact tested version while preserving the Julia 1.12 verification note.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@AGENTS.md`:
- Line 28: Update the CUDA.jl compatibility entry in AGENTS.md to remove the
unqualified “latest” claim, using “CUDA.jl 6.x” or the exact tested version
while preserving the Julia 1.12 verification note.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5979e159-7260-4ae9-afe4-bf938b81aec0
📒 Files selected for processing (2)
.env.exampleAGENTS.md
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sparse_brain.jl`:
- Around line 393-404: The docstrings for the step! function use `...` notation
instead of explicitly listing supported keyword arguments and their defaults,
making it difficult for users to discover available options. In
src/sparse_brain.jl lines 393-404 (anchor site), replace the `...` in the step!
docstring signature with explicit keyword documentation for inhibition,
reflex_eta, plasticity, recurrent_eta, sync, record_history, and
use_device_noise, each with their default values clearly stated. In
src/sparse_brain.jl lines 599-606 (sibling site), do the same for the step!
function there, listing inhibition, reflex_eta, reflex_signal, plasticity,
recurrent_eta, sync, record_history, and use_device_noise with their defaults.
Ensure both docstrings match the actual function signatures they document.
- Around line 304-314: Validate recurrent_eta for finiteness in the user-facing
function before the STDP update or any kernel launch, rejecting NaN and infinite
values with the established argument-validation error pattern. Anchor the change
near the existing plasticity and input checks, before recurrent_eta is converted
to Float32; leave valid finite values and other plasticity paths unchanged.
In `@test/runtests.jl`:
- Around line 226-230: In test/runtests.jl at lines 226-230 (anchor), replace
the L2 norm assertion with a full snapshot of Array(brain.W_out) before the
stepping loop, then assert exact equality (using ==) of that snapshot with
Array(brain.W_out) after the loop to verify weight preservation. In
test/runtests.jl at lines 298-307 (sibling), snapshot each lobe's W_out before
the multi-step loop with activity-producing (non-zero) input, then assert exact
equality of each lobe's W_out snapshot with its value after stepping to confirm
plasticity=:none prevents any weight changes across all lobes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 952328c9-02ee-4897-9bfd-b6c0aa98829d
📒 Files selected for processing (8)
.env.example.gitignoreAGENTS.mdProject.tomlREADME.mdsrc/LiquidCortex.jlsrc/sparse_brain.jltest/runtests.jl
- STDP kernel skips clamp/write when dw==0; eta=0 early-outs - LiquidCortexValidationError for API misuse; keep internal ArgumentError in Sentry - Tag OutOfGPUMemoryError / CuError as gpu_failure for alerts - CSC colPtr start-at-1 + monotonic checks; rethrow InterruptException from noise - Validate finite recurrent_eta; prewarm STDP edges before ensemble loop - Document reflex gating as readout-only; full kwargs in step docstrings - CPU plasticity validation tests; exact W_out freeze for :none - CI and Codecov matrices: Julia 1.12 only
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ca03f0225
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/LiquidCortex.jl (1)
98-105: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftBound Sentry capture tasks.
When Sentry’s
Channel{TaskPayload}(100)is full,push!blocks.timedwaitstops waiting but does not cancelt. Repeated exceptions can therefore leave unbounded blocked tasks that retain exception backtraces.Use a bounded worker or cap in-flight captures and drop reports when saturated. Add a stress test with a full queue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/LiquidCortex.jl` around lines 98 - 105, Bound the Sentry capture work around Sentry.capture_exception so a full Channel{TaskPayload}(100) cannot leave blocked tasks after timedwait returns. Add an in-flight limit or bounded worker that drops reports when saturated, and ensure timed-out capture tasks are cancelled or otherwise released; add a stress test exercising a full queue and verifying capture tasks remain bounded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 26: Align Julia version declarations across
.github/workflows/ci.yml:26-26 and .github/workflows/codecov.yml:25-25 with
Project.toml compatibility: if Julia 1.10 and 1.11 remain supported, add both
versions to each workflow matrix; otherwise narrow Project.toml compatibility to
Julia 1.12.
In `@src/LiquidCortex.jl`:
- Around line 81-88: Update _tag_gpu_failure! so GPU failure metadata is
attached only to the specific captured Sentry event rather than through global
Sentry.set_tag state; pass or apply event-local tags at capture time and ensure
later or queued events cannot inherit or overwrite error_class. Add a regression
test covering multiple events and verifying tags remain isolated.
In `@src/sparse_brain.jl`:
- Around line 312-325: In src/sparse_brain.jl lines 312-325, extract plasticity
and recurrent_eta checks into a GPU-independent
_validate_plasticity_kwargs!(plasticity::Symbol, recurrent_eta::Real) helper,
then call it from _validate_step_kwargs! after the input-length check; in
test/runtests.jl lines 76-95, replace the fabricated exception test with direct
calls to this helper covering an invalid plasticity mode and :recurrent_stdp
with NaN32.
- Around line 602-609: Update the prewarm condition in the recurrent STDP block
around `_ensure_edge_indices!` to require `recurrent_eta` to be finite as well
as nonzero. Keep prewarming enabled only for valid, nonzero recurrent rates, so
NaN or Inf reaches the existing validation path without materializing edge
indices.
In `@test/runtests.jl`:
- Around line 76-95: Replace the direct LiquidCortexValidationError throw in the
“CPU: plasticity mode validation” testset with a call to the u-independent
helper introduced from _validate_step_kwargs!, such as
_validate_plasticity_kwargs!, passing an invalid mode and recurrent_eta value.
Keep the `@test_throws` assertion focused on the real validator so CPU tests
exercise rejection of invalid plasticity configuration without requiring GPU
execution.
---
Outside diff comments:
In `@src/LiquidCortex.jl`:
- Around line 98-105: Bound the Sentry capture work around
Sentry.capture_exception so a full Channel{TaskPayload}(100) cannot leave
blocked tasks after timedwait returns. Add an in-flight limit or bounded worker
that drops reports when saturated, and ensure timed-out capture tasks are
cancelled or otherwise released; add a stress test exercising a full queue and
verifying capture tasks remain bounded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b58e95d-35e1-406b-8cad-d51329c67aaa
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/codecov.ymlAGENTS.mdREADME.mdsrc/LiquidCortex.jlsrc/sparse_brain.jltest/runtests.jl
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Add gpu-ci.yml targeting labels self-hosted/Linux/X64/gpu with a repo-wide concurrency group so only one suite uses the RTX host at a time. CPU smoke remains on ubuntu-latest.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Verify CUDA.jl only after instantiate with --project=.; host preflight is julia + nvidia-smi only.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b9df1c211
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/gpu-ci.yml:
- Around line 38-39: Update the Checkout step using actions/checkout to set
persist-credentials to false, ensuring the GITHUB_TOKEN is not retained in the
local git configuration; leave the pinned action reference unchanged.
- Around line 41-49: Update the “Verify host toolchain” step in the GPU workflow
to assert that the installed Julia version is exactly 1.12, rather than only
printing it. Make the check fail clearly when the host version differs, while
preserving the existing Julia availability, version output, and GPU validation.
- Around line 5-13: Update the workflow trigger configuration under on to remove
pull_request for the GPU job, unless the job is moved to an isolated ephemeral
runner with no secrets; preserve push and workflow_dispatch triggers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 681404e4-9a3c-4ff3-a1c4-6a5b9039510b
📒 Files selected for processing (2)
.github/workflows/gpu-ci.ymlAGENTS.md
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
- Harden ensemble :none test with reclaim_gpu_hard! and fewer steps - GPU workflow: skip fork PRs, per-ref concurrency, persist-credentials false, assert host Julia 1.12 - Validate ensemble kwargs before STDP edge prewarm - CPU-safe plasticity validator; dual edge-buffer readiness check - Re-scope Sentry gpu_failure tags every capture; rethrow CUDA OOM/CuError from device-noise path
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Soft CUDA.reclaim left ~2GB pool growth per EnsembleBrain on 16GB, so the late :none ensemble hit 99% VRAM and failed on cuBLAS. Always device_reset! between ensembles; fold :none freeze into the existing ensemble inhibition test; serialize Sentry tag+capture.
There was a problem hiding this comment.
No application code in the PR — skipped Code Health checks.
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42d2d4d55b
ℹ️ 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".
| catch | ||
| end |
There was a problem hiding this comment.
When CUDA.device_reset!() is unavailable or fails on the documented self-hosted GPU runner, this empty handler silently returns after only the soft reclaim, even though the helper states that a full reset is required on 16 GB cards. Subsequent ensemble tests can then reproduce the VRAM OOM this helper was added to prevent while hiding the actual reset failure; let the exception fail the test or explicitly verify that enough memory was reclaimed before continuing.
AGENTS.md reference: AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/runtests.jl">
<violation number="1" location="test/runtests.jl:20">
P2: The bare `catch` around `CUDA.device_reset!()` silently swallows reset failures. Since the comment above this helper states a full device reset is required between EnsembleBrain cases on 16GB cards (soft reclaim alone leaves ~2GB reserved per ensemble), a failed reset here will go unnoticed and subsequent GPU ensemble tests can hit the VRAM OOM this helper was added to prevent. Consider logging the caught error or verifying free memory afterward instead of silently continuing.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| # Soft reclaim leaves the memory pool reserved (~2GB leak per ensemble in CI). | ||
| function reclaim_gpu_hard!() | ||
| reclaim_gpu!() | ||
| try |
There was a problem hiding this comment.
P2: The bare catch around CUDA.device_reset!() silently swallows reset failures. Since the comment above this helper states a full device reset is required between EnsembleBrain cases on 16GB cards (soft reclaim alone leaves ~2GB reserved per ensemble), a failed reset here will go unnoticed and subsequent GPU ensemble tests can hit the VRAM OOM this helper was added to prevent. Consider logging the caught error or verifying free memory afterward instead of silently continuing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/runtests.jl, line 20:
<comment>The bare `catch` around `CUDA.device_reset!()` silently swallows reset failures. Since the comment above this helper states a full device reset is required between EnsembleBrain cases on 16GB cards (soft reclaim alone leaves ~2GB reserved per ensemble), a failed reset here will go unnoticed and subsequent GPU ensemble tests can hit the VRAM OOM this helper was added to prevent. Consider logging the caught error or verifying free memory afterward instead of silently continuing.</comment>
<file context>
@@ -13,16 +13,13 @@ function reclaim_gpu!()
- catch
- end
- reclaim_gpu!()
+ try
+ CUDA.device_reset!()
+ catch
</file context>
Local GPU CI assessment — how good is the GPU code?Author: Grok Build / Grok 4.5 (high) (xAI) Assessment of self-hosted GPU CI on this PR (head at time of writing: Setup
Latest green run numbers
Recent run history (honest)
CI is green now, but it was flaky under VRAM pressure until the suite was hardened. That is a test/hosting issue as much as a “bad kernel” issue. What the GPU suite actually coversNot a microbenchmark. It is a correctness + integration suite on real 65k-neuron lobes:
So CI answers: “Does the GPU path run and obey the contracts we wrote?” How good is the GPU code? (engineering judgment)Strengths (genuinely good)
Weaknesses / risk (not “bad,” but real)
Bottom line scorecard
In one sentence: Useful follow-ups if we want a sharper bar: a short steps/s benchmark job, and a multi-ensemble stress test that fails if free VRAM after teardown is too low. Posted by Grok Build using Grok 4.5 (high). |
Local GPU test run (RTX 5080)Author: Grok Build · Grok 4.5 (high) (xAI) Full package test log from local/self-hosted GPU verification. Result
Notable log linesInterpretation
Posted by Grok Build · Grok 4.5 (high). |
1 participant
Summary
step!/ensemble_step!kwargs:plasticity,recurrent_eta,sync,record_history,use_device_noise:readout_onlypreserves frozen recurrentW+ Hebbian readout:recurrent_stdp(pair STDP every tick on sparse edges; lazy edge lists; CUDA.jl only)sync=truefree_memorycompat for Ensemble VRAM report)Tracking
Test plan
julia --version→ 1.12.x;Pkg.test()→ 71/71 on RTX 5080 (Blackwell).explore-notes/, or third-party STDP deps in PROut of scope
Summary by cubic
Adds experimental plasticity to
step!/ensemble_step!with GPU controls for sync, history, and device noise, plus pair STDP on sparse edges. Hardens validation, Sentry reporting, tests, and CI (Julia 1.12-only, self-hosted GPU) for reliability; aligns with Linear epic LIM-898.Bug Fixes
LiquidCortexValidationErrorfor API misuse; ensemble validatesplasticity/recurrent_etabefore STDP prewarm via a CPU-safe validator.LiquidCortexValidationError; tag GPU failures and serialize tag+capture per event; device RNG path rethrowsCUDA.OutOfGPUMemoryError/CUDA.CuErrorand falls back to host only on non-GPU RNG failures.sync=true. GPU tests hard-reset (CUDA.device_reset!) between ensembles to stop VRAM pool growth and fold the:nonefreeze into the ensemble inhibition test.Dependencies
CUDA.jl6; useCUDA.free_memoryfor VRAM reporting.ci.ymlandcodecov.yml; self-hostedgpu-ci.ymlwith per-ref concurrency, fork PR isolation, runner Julia 1.12 assertion, and project-envCUDA.jlverification afterPkg.instantiate.Written for commit 42d2d4d. Summary will update on new commits.