Fix offloading_decision_summary.py to read all split segments#69
Open
ff225 wants to merge 1 commit into
Open
Conversation
The summary script used iter_offloading_decisions(), which only reads a single file. Once offloading_decisions.csv splitting is enabled the base file is never written, so pointing the script at it silently produced an empty summary. Switch to iter_run_offloading_decisions(), which discovers and reads every .segNNNN_* segment for the run. Addresses one item of #52 (the offline-reader verification task); the other analysis entry points already glob all segments.
4 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.
Summary
Addresses part of #52 (offline-reader verification task, first bullet, scoped to
offloading_decision_summary.py).scripts/analysis/offloading_decision_summary.pyread the input path withiter_offloading_decisions(), which only reads a single file. Onceoffloading_decisions.csvsegment splitting is enabled (evaluation.split, seedocs/OFFLOADING_DECISION_EVENTS.md), the base file is never written — only.segNNNN_*segments are — so running the script against the base path silently produced an empty summary instead of an error or a real result.Fix: use
iter_run_offloading_decisions()(already exported byserver.telemetry.offloading_decisions, and already used elsewhere, e.g. in the offloading-decision-events tests), which discovers and reads every segment for the run viarun_segment_paths. For unsplit/legacy single-file runs, behavior is unchanged —run_segment_pathsjust returns the one file.I checked the other locations #52 calls out:
scripts/analysis/telemetry_notebook_utils.py::discover_csv_filesalready globsoffloading_decisions*.csv, so it already reads all segments — no change needed.src/server/evaluations/edit_evaluations.ipynbreads a single hardcoded./evaluations.csvin one ad-hoc cell; it isn't part of the segmented offloading-decisions/inference-cycles output pipeline, so it's out of scope for this fix.The issue's second bullet (evaluating a dedicated per-output
output.splitconfig) is a design question, not a bug fix, and is left for separate discussion — out of scope here to keep this change small and reviewable.Test plan
test_offloading_decision_summary_reads_all_split_segmentsintests/unit/test_offloading_decision_summary.py, writing 3 rows withmax_rows=1(forcing 3 segments, no base file) and asserting the summary covers all of them in order.uv run pytest tests/unit -q— 148 passed, 1 skippedtests/unit, integration subset, mqtt, offloading_algo) — 204 passed, 1 skipped, no regressionsdocs/OFFLOADING_DECISION_EVENTS.mdto reflect the new behavior (previously told users to manually point at a segment).