Skip to content

Fix/episode cache bytes metric wiring#258

Open
huynna12 wants to merge 2 commits into
FastCrest:mainfrom
huynna12:fix/episode-cache-bytes-metric-wiring
Open

Fix/episode cache bytes metric wiring#258
huynna12 wants to merge 2 commits into
FastCrest:mainfrom
huynna12:fix/episode-cache-bytes-metric-wiring

Conversation

@huynna12

@huynna12 huynna12 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Description

Closes the loop on #103. The requested tracking (EpisodeCache byte accounting + tether_episode_cache_bytes_total Prometheus gauge) was already implemented in #106. This PR fixes two correctness gaps left in that implementation:

  1. Metric never emits in production. The only real call site, Pi05DecomposedInference.__init__ with cache_level="episode" (src/tether/runtime/pi05_decomposed_server.py:378), constructed EpisodeCache without embodiment/model_id. Since EpisodeCache._emit_bytes_metric() no-ops unless both are set, the gauge silently never emitted once episode mode was enabled. Fixed by reusing cuda_graphs_embodiment/cuda_graphs_model_id, already threaded into the same constructor for CUDA graph metrics.

  2. insert() double-counts bytes on a same-key re-insert. Calling insert() twice for the same (episode_id, lang_hash) never backed out the old entry's bytes before adding the new ones - a permanent leak in _total_bytes (and therefore the gauge), a chance of evicting an unrelated LRU entry at capacity, and inflated stats.episode_count ("unique episodes seen"). Not reachable via today's only caller (which always checks lookup() first), but insert() is public API on the exact class [Metrics] Track Episode Cache Memory Usage #103 asked to be byte-accurate, so it's fixed directly rather than left latent.

Note: cache_level="episode" isn't wired to any tether serve CLI flag yet, so this doesn't make the feature live today, it makes the byte-tracking and metric correct for when it becomes reachable.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change
  • Documentation update
  • Infrastructure / CI update

How Has This Been Tested?

  • pytest tests/ passes locally

  • tether doctor sanity check

  • Other: manual end-to-end script against the real (non-mocked)
    classes, reproducible with:

    pytest tests/test_episode_cache_bytes.py \
           tests/test_episode_cache_production_wiring.py \
           tests/test_episode_cache_utils.py \
           tests/test_decomposed_server.py \
           tests/test_cuda_graphs_integration.py \
           tests/test_pi05_action_fast_path_integration.py -v
    # -> 51 passed, 1 skipped (CUDA-only test, expected without a GPU)

    Confirmed both new test groups fail against the pre-fix code before passing with it:
    assert None == 'franka' (gap 1), assert 1032 == 516 / episode_count 2-vs-1 / evictions 1-vs-0 (gap 2).

    Also drove the real Pi05DecomposedInference class (mocked ORT session only - no real model export available here) through cache_level="episode" and read the actual rendered Prometheus output:

    tether_episode_cache_bytes_total{embodiment="franka",model_id="pi05-real-check",policy_slot="prod"} 516.0
    
  • ruff check clean on all touched files.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes
  • I have kept the PR scoped to a single concern

huynna12 added 2 commits July 7, 2026 13:46
…es gauge

Pi05DecomposedInference is the only production call site that constructs
EpisodeCache (cache_level="episode"), but it never passed embodiment/model_id.
EpisodeCache._emit_bytes_metric() is a no-op unless both are set, so
tether_episode_cache_bytes_total (added in FastCrest#106, closes the tracking half
of FastCrest#103) never actually emitted once episode mode was enabled.

Reuse cuda_graphs_embodiment/cuda_graphs_model_id, already threaded into
this same constructor for CUDA graph metrics, as the label values.

Adds tests/test_episode_cache_production_wiring.py to cover the
construction path end-to-end (mocked ORT sessions), which previously had
no coverage.
insert() never checked whether (episode_id, lang_hash) already existed
in the cache. Re-inserting an existing key would:
  - leak _total_bytes (old entry's bytes never subtracted before the
    new entry's bytes were added), permanently inflating the
    tether_episode_cache_bytes_total gauge from FastCrest#106
  - potentially evict an unrelated LRU entry at capacity, since the
    eviction check runs off len(_cache), which doesn't grow on replace
  - inflate stats.episode_count, documented as "unique episodes seen"

Not reachable via the current production caller (predict_action_chunk
only calls insert() after a lookup() miss), but insert() is public API
on the class FastCrest#103 asked to make byte-accurate, so this closes that gap
directly rather than leaving it latent.

Adds three regression tests; confirmed each fails against the prior
implementation (assert 1032 == 516, episode_count 2 vs 1, evictions 1
vs 0) before passing with the fix.

Also drops two pre-existing unused imports (dataclasses.field,
tests/test_episode_cache_bytes.py's pytest) in the files touched here —
mechanical, zero behavior change.
@huynna12 huynna12 requested a review from rylinjames as a code owner July 7, 2026 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant