es-ude / es-ude/OnDeviceTraining

userApi: add ordered cleanup primitive for model + optimizer + auxiliary tensors

Open
#113 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
1
Forks
3
Avg merge
1d 1h
Merged PRs (30d)
8

Description

## Problem

Phase 2 LSan teardown idiom migrations (#109, #110, #111, #112) surfaced that the per-test cleanup orchestration is fragile. Each test must manually:

1. Know which destructors cascade to children (e.g., `freeOptimSgdM` does, `freeLinearLayer` does **not**) and which don't.
2. Free in a specific reverse-init order to avoid double-free or use-after-free.
3. Avoid re-freeing parameters that are already freed by `freeOptimSgdM`'s parameter-cascade.

A representative \"hard\" test (e.g. \`testMultiLayerTraining_MultipleSteps_GradsAccumulate\` in \`test/unit/userAPI/UnitTestMultiLayerTraining.c\` after #110) orchestrates ~10 frees across optimizer + 4 layers + 4 parameters + 2 tensors + 1 quantization, with subtle ownership rules between them. New contributors who get the order wrong silently introduce leaks (or worse, double-frees).

Two real production-code bugs were surfaced by this fragility during Phase 2:
- \`inference()\` / \`inferenceWithLoss()\` did not free the loop's final \`outputNext\` — fixed in #109.
- \`freeOptimSgdM\` / \`freeState\` freed children of arrays/structs but not the array/struct containers themselves — fixed in #110.

Both are the same structural pattern: \"destructor walks what it owns but forgets the wrapper.\"

## Proposed feature

A userApi-level cleanup primitive that owns the ordering, e.g. one of:

**Option A — Model-scope destructor.**
\`\`\`c
freeModel(layer_t **model, size_t numberOfLayers);
\`\`\`
Walks the model's layers in reverse, frees each layer's parameters (gating on whether they are also owned by an optimizer to avoid double-free), then frees each layer struct.

**Option B — Bound destructor on a builder/handle type.**
A new \`trainingContext_t\` (or similar) that aggregates model + optimizer + shared quantizations at construction time and exposes a single \`freeTrainingContext(ctx)\` that handles the full cleanup graph in one call.

**Option C — Uniform layer-level deep-free convention.**
Make every \`free*Layer\` cascade to its parameters by default. Optimizer construction registers a \"do not double-free\" flag on its parameters. Tests then don't have to reason about ownership at all.

Each option has different blast radius: A is additive, B is a new abstraction, C changes existing semantics across every layer free function.

## Out of scope for this issue

- The Phase 2 framework PRs (#109–#112) themselves; they ship the convention as-is.
- The remaining migration wave (12 test files) which will continue to use the explicit per-step orchestration.
- A handle-table StorageApi redesign (\`project_storage_api_handle_vision\`); that's a separate, longer-term initiative.

## Acceptance criteria (when implemented)

- Tests using the new primitive can replace their ~10-line cleanup block with 1–2 calls.
- LSan + valgrind continue to report zero residual.
- The cleanup-cascade table in \`docs/CONVENTIONS.md\` (added in #112) shrinks correspondingly.

Related: #109, #110, #111, #112, #82.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.