fix: green-light CI — Dense fit-flag tracer, buffer-donation test pollution, L1 loss contract; Codecov token#855
Merged
Conversation
…donation pollution, L1 loss; Codecov token
Source fix
----------
brainpy/dnn/linear.py: ``Dense.update`` did
``if share.load('fit', False) and self.online_fit_by is not None:``. Inside a
grad-/jit-traced fit step (BPFF/BPTT) the ``fit`` share value is a JAX tracer,
so converting it to a Python bool raised ``TracerBoolConversionError`` and broke
the canonical Dense/RNNCell back-prop training example. Reordered to consult the
static ``*_fit_by`` configuration first; the ``and`` then short-circuits before
the tracer is forced when online/offline fitting is not configured.
This also removes a cross-test pollution: when the fit raised mid-trace it left
a stale traced ``fit`` in the global ``share`` store, so the next test running
``Dense.update`` (e.g. ``LoopOverTime`` over a ``Dense``) also raised.
Test isolation
--------------
brainpy/running/jax_multiprocessing_test.py: ``test_vectorize_map_partial_chunk_clear_buffer``
ran ``jax_vectorize_map(..., clear_buffer=True)``, which invokes the process-global
``bm.clear_buffer_memory()`` and deletes EVERY live device buffer, poisoning later
test modules ("deleted/donated buffer" errors). Patched the wipe to a no-op for the
duration of the call (same guard already used in boost_misc_test and
train_analysis_glue_fixes_test) so the code path stays covered without nuking the
shared session.
Test contract updates
---------------------
brainpy/train/back_propagation_test.py: rewrote the pinned-defect test into a
regression test that asserts ``Dense`` trains under BPFF (finite losses, weight
moves) and that a subsequent plain forward pass does not raise (pollution guard).
brainpy/losses/comparison_coverage_test.py: ``l1_loss`` delegates to
``braintools.metric.l1_loss`` (>=0.3.0, the required/CI dependency), which reduces
each sample to its mean absolute error then applies the batch reduction. Updated
the L1 expectations (none -> [1.5, 3.5], sum -> 5.0, mean -> 2.5) and comments to
the 0.3.0 contract; the previous numbers pinned stale braintools 0.1.10 behaviour.
CI
--
.github/workflows/CI.yml: Codecov upload now passes
``token: ${{ secrets.CODECOV_TOKEN }}`` and ``slug: brainpy/BrainPy``.
Reviewer's GuideMakes Dense.update safe under JAX tracing, fixes a test that globally clears JAX buffers, updates L1 loss tests to match the current braintools contract, strengthens the Dense/BPFF regression test, and wires Codecov token/slug into CI so the full pytest brainpy/ suite passes under braintools>=0.3.0. Sequence diagram for Dense.update fit-flag handling under JAX tracingsequenceDiagram
actor User
participant BPFF as BPFF.fit
participant Share as share
participant Dense as Dense.update
User->>BPFF: fit(...)
BPFF->>Share: Share.save fit = tracer
rect rgb(245,245,245)
note over BPFF,Dense: Pre-fix behaviour
BPFF->>Dense: update(x)
Dense->>Share: load fit, False
Share-->>Dense: tracer
Dense->>Dense: [bool(tracer)]
Dense-->>BPFF: TracerBoolConversionError
end
rect rgb(235,255,235)
note over BPFF,Dense: Post-fix behaviour
BPFF->>Dense: update(x)
alt [online_fit_by is not None]
Dense->>Share: load fit, False
Share-->>Dense: tracer
Dense->>Dense: [record fit data]
else [online_fit_by is None]
Dense-->>BPFF: return res (no tracer bool)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
test_vectorize_map_partial_chunk_clear_buffer, consider using pytest'smonkeypatchfixture (or a context manager helper reused where you already guardclear_buffer_memory) instead of manually reassigningbm.clear_buffer_memoryto reduce the risk of leaks or interference if the test is ever parallelized or extended. - The new regression test
test_dense_layer_fit_flag_under_grad_tracemixes several assertions (loss finiteness, weight movement, pollution guard) in one test; you might consider splitting the pollution guard into a separate test to isolate failure modes and make future regressions easier to localize.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_vectorize_map_partial_chunk_clear_buffer`, consider using pytest's `monkeypatch` fixture (or a context manager helper reused where you already guard `clear_buffer_memory`) instead of manually reassigning `bm.clear_buffer_memory` to reduce the risk of leaks or interference if the test is ever parallelized or extended.
- The new regression test `test_dense_layer_fit_flag_under_grad_trace` mixes several assertions (loss finiteness, weight movement, pollution guard) in one test; you might consider splitting the pollution guard into a separate test to isolate failure modes and make future regressions easier to localize.
## Individual Comments
### Comment 1
<location path=".github/workflows/CI.yml" line_range="57-61" />
<code_context>
run: |
pytest --cov=brainpy --cov-report=xml brainpy/
- - name: Upload coverage to Codecov
+ - name: Upload coverage reports to Codecov
uses: codecov/codecov-action@v5
with:
+ token: ${{ secrets.CODECOV_TOKEN }}
+ slug: brainpy/BrainPy
files: ./coverage.xml
fail_ci_if_error: false
</code_context>
<issue_to_address>
**🚨 question (security):** Explicit Codecov token usage in CI might be unnecessary for a public repo and increases secret surface area.
Using `token: ${{ secrets.CODECOV_TOKEN }}` adds an extra secret to manage and expose to this workflow. For public repos, recent `codecov/codecov-action` versions typically work with just the GitHub-provided token, which simplifies configuration and slightly reduces risk.
If this repo is public and not using private uploads or advanced Codecov features, consider verifying whether the explicit `token` and `slug` are required and removing them if they’re not.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+57
to
+61
| - name: Upload coverage reports to Codecov | ||
| uses: codecov/codecov-action@v5 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| slug: brainpy/BrainPy |
There was a problem hiding this comment.
🚨 question (security): Explicit Codecov token usage in CI might be unnecessary for a public repo and increases secret surface area.
Using token: ${{ secrets.CODECOV_TOKEN }} adds an extra secret to manage and expose to this workflow. For public repos, recent codecov/codecov-action versions typically work with just the GitHub-provided token, which simplifies configuration and slightly reduces risk.
If this repo is public and not using private uploads or advanced Codecov features, consider verifying whether the explicit token and slug are required and removing them if they’re not.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the full
pytest brainpy/suite (the CI command on Linux/macOS/Windows,Py 3.13, against
braintools>=0.3.0) fully green: 4180 passed, 13 skipped, 0failed. Fixes one real source bug, one test-isolation defect, and two stale
test contracts; also wires the Codecov token/slug into CI.
Source fix —
brainpy/dnn/linear.pyDense.updatedidif share.load('fit', False) and self.online_fit_by is not None:.Inside a grad-/jit-traced fit step (
BPFF/BPTT) thefitshare value is a JAXtracer, so
bool(tracer)raisedTracerBoolConversionError, breaking thecanonical Dense/RNNCell back-prop training example. Reordered to check the static
*_fit_byconfig first so theandshort-circuits before the tracer is forcedwhen online/offline fitting is not configured. This also eliminates a cross-test
pollution: a fit that raised mid-trace left a stale traced
fitin the globalsharestore, breaking the next test that ranDense.update.Test isolation —
brainpy/running/jax_multiprocessing_test.pytest_vectorize_map_partial_chunk_clear_buffercalledjax_vectorize_map(..., clear_buffer=True), which triggers the process-globalbm.clear_buffer_memory()— deleting every live device buffer and poisoninglater test modules with "deleted/donated buffer" errors. Patched the wipe to a
no-op for the call (the same guard already used in
boost_misc_testandtrain_analysis_glue_fixes_test), keeping the code path covered without nukingthe shared session.
Test contracts
brainpy/train/back_propagation_test.py— rewrote the pinned-defect testinto a regression test: asserts
Densetrains under BPFF (finite losses,weight moves) and a subsequent plain forward pass does not raise (pollution
guard).
brainpy/losses/comparison_coverage_test.py—l1_lossdelegates tobraintools.metric.l1_loss(>=0.3.0, the required/CI dependency), which reduceseach sample to its mean absolute error then applies the batch reduction.
Updated L1 expectations (
none -> [1.5, 3.5],sum -> 5.0,mean -> 2.5) andcomments to the 0.3.0 contract; the prior numbers pinned stale 0.1.10 behaviour.
CI —
.github/workflows/CI.ymlCodecov upload now passes
token: ${{ secrets.CODECOV_TOKEN }}andslug: brainpy/BrainPy(keptfiles: ./coverage.xml,fail_ci_if_error: false).Test plan
pytest brainpy/againstbraintools 0.3.0: 4180 passed, 13 skipped, 0 failed.running/+ the two former victims pass (polluter neutralised);losses/85 passed;dnn/+back_propagation_test.py+transform_test.pypass.Summary by Sourcery
Fix Dense layer fit-flag handling to work under gradient/JIT tracing, align L1 loss tests with the current braintools contract, prevent JAX multiprocessing tests from clearing global buffers, and wire Codecov token/slug into CI coverage uploads.
Bug Fixes:
Enhancements:
CI:
Tests: