Extract per-device state out of RequestHandler (#11)#74
Merged
Conversation
The committed lock listed `google-cloud-firestore` in the `analysis` extra
without a version/source, while the package is forked across two versions by
platform marker. uv 0.11+ refuses to parse it:
Dependency `google-cloud-firestore` has missing `source` field
but has more than one matching package
Every uv command failed on this, so the environment could not be synced at all.
Regenerated with `uv lock` (224 packages resolved).
RequestHandler kept every piece of runtime state in class-level dicts mutated from both the ASGI thread and the background I/O thread, with no locking. Introduce DeviceStateManager, which owns the device profiles, the EMA tables, the variance detectors, the adaptive-risk state and the layer-sizes cache behind a single RLock. RequestHandler.device_profiles stays as a live view of the manager's mapping, since http_server and the dashboard read it directly. The variance detector is now scoped per device: it used to be a single class-level instance, so heterogeneous devices reporting the same layer indices polluted each other's variance statistics and skewed offloading decisions. Also drop a redundant _load_stats() call in the inference-cycle telemetry path that recomputed layer sizes already available in scope, behind a hasattr() guard that was always true. First step of #45; unblocked now that #9 has landed.
Two tangled problems in the same code path; the fixes share the same lines, so they land together. #72 — Edge per-layer times were smoothed twice. `track_inference_time` applied an EMA with a hard-coded alpha=0.2 to `self.inference_times` — which is the very dict `device_profiles[device_id]["edge_inference_times"]`, passed by reference — and the request handler then applied the configured EMA on top. Edge times were therefore smoothed twice while device times were smoothed once, so the two sides of the offloading cost comparison responded to change at different rates; and the `ema_alpha` setting added in #9 never reached the first application. `Edge.run_inference` already measures and returns the raw per-layer times, so the decorator's copy was redundant. It is now instrumentation only, and `DeviceStateManager.update_edge_times` is the single place that smooths, with the configured alpha. The timing there now uses perf_counter rather than time.time, since it became the authoritative measurement. #73 — `Edge.variance_detector` was a second, process-global detector fed from the same decorator. Nothing ever read it: no production code calls `should_retest_offloading()` on it, so it only burned time on the hot path and logged re-test warnings that triggered no re-test. Removed; the per-device detectors from #11 remain the only ones. Also drops the now-unwritten `inference_times` plumbing from ModelManager, `save_inference_times` (no callers), and a commented-out __init__ referencing attributes that no longer exist. `scripts/analysis/variance_analysis.py` moves to the per-device API.
The simulation CSV handle, its writer and its row counter were class-level attributes on RequestHandler, opened and closed from ASGI request threads while the background I/O thread wrote rows through them. Three races followed: - `close_simulation_csv` cleared `csv_file` while the I/O thread was inside `_write_csv_row`, so a queued row could reach a closed file and raise "I/O operation on closed file"; - `_write_csv_row` tested `csv_writer` and then used `csv_file`, with nothing keeping the two consistent across the gap; - `inference_counter += 1` ran on concurrent request threads, and `set_simulation_csv` reset it underneath them, so ids could collide. InferenceCycleRecorder owns all of it behind one lock: the check and the write happen together, a row that arrives after a close is dropped and reported rather than written, and row ids are handed out atomically. A row that fails to serialize is now logged and swallowed instead of propagating into the background I/O worker. The debug-JSON writing moves here too, since it shares the same background thread and per-server-start lifecycle. `RequestHandler.set_simulation_csv` / `close_simulation_csv` stay as delegating classmethods; `http_server` is unchanged. Second step of #45.
…t path (#45) Third and last step of #45. RequestHandler built the offloading algorithm, ran it, unpicked its candidate list and published the decision telemetry inline. OffloadingService now owns that, and returns an OffloadingDecision carrying everything telemetry needs to explain the choice: the layer, the reason, the candidates, the switch penalty and the estimated cost. Deriving the switch penalty and the cost estimate moves onto the decision object, where they belong — they are properties of the decision, not of the request. The handler is left orchestrating: it asks for a decision and reports it. Building the telemetry *events* stays in the handler on purpose: those need `message_data`, and moving them would couple the service to the transport message shape. The algorithms themselves are untouched: `src/server/offloading_algo/` has no changes. Only the caller moved. Adds tests for handle_device_inference_result, which #45 flagged as the most critical component with no dedicated tests: the EMA updates for device and edge layers, the variance recording, the edge-inference skip conditions, the offloading decision (including forced-local, and that the decision sees the freshly smoothed times), the background I/O scheduling for every output, and that the debug snapshot is immune to later mutation of the live tables. request_handler.py: 1158 -> 933 lines; coverage 48% -> 61%.
5 tasks
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.
Closes #11. First of three steps toward #45; unblocked now that #9 has landed.
Why
RequestHandlerkept every piece of runtime state in class-level dictionaries —device_profiles,variance_detector,offloading_states,_layer_sizes_cache— mutated from both the ASGI thread and the background I/O thread with no locking.What changed
New
src/server/communication/device_state.py.DeviceStateManagerowns the device profiles, the EMA tables, the variance detectors, the adaptive-risk state and the layer-sizes cache, all behind oneRLock.Per-device variance detectors. The detector used to be a single class-level instance, so heterogeneous devices reporting the same layer indices polluted each other's variance statistics and skewed offloading decisions. Each device now has its own.
Thread-safety. The debug-JSON snapshot is now taken under the lock before being handed to the background writer; it previously serialized a dict that the ASGI thread could be mutating.
Backwards compatibility.
RequestHandler.device_profilesremains a live view of the manager's mapping, sincehttp_server.py:233and the dashboard read it directly as a plain dict. No caller outsiderequest_handler.pychanges.Drive-by cleanup. The inference-cycle telemetry path recomputed
layer_sizesvia_load_stats()— data already in scope — behind ahasattr()guard that was always true.uv.lock
The first commit regenerates
uv.lock, which was unparsable by uv 0.11+ and blocked everyuvcommand, including CI's:The package is forked across two versions by platform marker; the
analysisextra listed it without a version/source to disambiguate. Regenerated withuv lock(224 packages resolved). Included here because nothing else could be verified without it.Testing
244 passed, 1 skipped (fast suite + HTTP end-to-end).
device_state.pyat 100% coverage; newtests/unit/test_device_state.pycovers the EMA maths, per-device isolation, snapshot copying, and an 8-thread concurrency test. ruff clean on all touched files.Notes for the reviewer
_load_statsis gone rather than kept as a deprecated shim: it is private and had no remaining callers.VarianceDetector). This PR preserves both behaviours exactly; they are fixed in the follow-up.