Repository navigation
Release CPU vector resources and make close safe - #31
m2kulkarni wants to merge 2 commits into
Conversation
m2kulkarni
left a comment
There was a problem hiding this comment.
Independent review by Codex session review_26, recorded by coordinator root. The reviewer fetched published commit 4660900 in a separate worktree and confirmed its two files exactly match the independently reviewed and tested content. No blocking findings.
Reviewer reproduced GCC native and legacy builds; full simulator suite 256 passed/46 existing skips; trainer regressions 12 passed/9 subtests; lifecycle suite 5 passed/7 subtests; actual CPU smoke (336 steps, finite losses, eight updated tensors, exact saved-weight predictions). Constructor cleanup and repeated training with double close passed. Linux x86_64, Python 3.12.14, Torch 2.14.1+cpu.
No sanitizer, GPU, long-run leak or throughput checks. Counters cover binding-owned dictionaries and vectors only. This COMMENT records another agent's review, not independent GitHub approval or permission to merge.
m2kulkarni
left a comment
There was a problem hiding this comment.
Final-head independent review by Codex review_26, recorded by coordinator root. Reviewer independently fetched fac4677 and found no blocking findings. Source and tests match the previously reviewed/tested content exactly. The only new change is the three-line lifecycle pytest step in the existing CPU training job, after dependencies/native builds and before smoke. No permissions, triggers or existing checks changed. Previous independently reproduced test results apply to unchanged code; final-head CI remains pending. COMMENT only; no merge approval.
Closes #26. Depends on #29.
Author: Codex root session puffer26-20261022. Independent reviewer: Codex review_26, separate worktree. Review found no blocking issues and confirmed exact published commit fac4677. The coordinator recorded the attributed COMMENT review; this is not independent GitHub approval.
Closing a CPU vector twice or using its native methods/pointer properties after close crashed Python. The binding also leaked temporary configuration dictionaries and did not close abandoned vectors. Scoped ownership now frees dictionary storage (never borrowed Python keys), closes vectors on destruction, makes close idempotent, and rejects post-close operations with RuntimeError before pointer access. A partially configured wrapper owns its vector during constructor exceptions. Reset/step retain the GIL so another Python thread cannot close the vector mid-call; throughput impact is unmeasured.
The existing CPU training CI job now runs the lifecycle suite; no permissions, triggers or existing checks were changed.
Private resource counters check binding-owned dictionary/vector balance, not all engine allocations. Regressions cover constructor exceptions, explicit and implicit cleanup, all post-close methods/pointers, and two actual CPU training runs with trainer close invoked twice each.
Validation on Linux x86_64, Python 3.12.14, Torch 2.14.1+cpu, GCC:
Setup failures: default legacy build could not find clang; explicitly using GCC succeeded without changing source flags. Initial trainer tests lacked rich/rich_argparse; installing dependencies resolved this and all 12 tests passed on rerun. CLI push lacked credentials; the connected GitHub API published the identical file tree. Official gh-stack successfully adopted this branch locally with #29's branch as its base. Remote stack registration remains pending CLI authentication; the connected API created this draft PR against the correct parent.
Keep draft: exact-commit independent review is recorded. Automatic Linux CI passed both jobs at this exact commit: https://github.com/m2kulkarni/PufferWelfare/actions/runs/39837526914, including the new lifecycle suite. Keep draft under the parent branch's current review instructions; no merge approval is implied. No sanitizer, GPU, throughput or long-run leak checks were run. Layout validation (#27), engine allocation failure (#28), importance weights (#24), installation (#30), and known game/action/map defects remain outside this change. This is not a finished migration or learned baseline. No merge without explicit human approval.