Skip to content

feat: chart generation throughput#696

Open
eric-tramel wants to merge 20 commits into
mainfrom
codex/progress-throughput-panel
Open

feat: chart generation throughput#696
eric-tramel wants to merge 20 commits into
mainfrom
codex/progress-throughput-panel

Conversation

@eric-tramel
Copy link
Copy Markdown
Contributor

@eric-tramel eric-tramel commented May 21, 2026

Summary

This PR replaces the old fill/disappear terminal progress bars with a bounded ANSI/asciichart throughput panel that is enabled by default through the canonical RunConfig.display_tui=True setting. RunConfig.progress_bar remains supported as a deprecated compatibility shim with a warning, and the CLI exposes data-designer create --tui / --no-tui as a per-run override.

Data Designer throughput panel

Usage

Default CLI behavior

The TUI is enabled by default for create runs when the process is attached to a terminal. In non-TTY environments, such as redirected output or many notebook contexts, the renderer falls back instead of trying to draw the live panel.

data-designer create path/to/config.py \
  --num-records 1000 \
  --dataset-name throughput-demo

Force the TUI for one CLI run

Use --tui when the saved or programmatic run config disables the panel but this terminal run should show it.

data-designer create path/to/config.py \
  --num-records 1000 \
  --dataset-name throughput-demo \
  --tui

Disable the TUI for one CLI run

Use --no-tui for logs-only runs, CI jobs, captured terminal output, or any case where the live panel would be noisy.

data-designer create path/to/config.py \
  --num-records 1000 \
  --dataset-name throughput-demo \
  --no-tui

Configure from Python

display_tui is the canonical run config option. It defaults to True; set it explicitly when creating datasets from a Python program.

from data_designer.config import RunConfig
from data_designer.interface import DataDesigner

# builder = ...  # Build or load a DataDesignerConfigBuilder.
data_designer = DataDesigner()
data_designer.set_run_config(RunConfig(display_tui=True))

results = data_designer.create(
    builder,
    num_records=1000,
    dataset_name="throughput-demo",
)

Disable from Python

from data_designer.config import RunConfig
from data_designer.interface import DataDesigner

# builder = ...
data_designer = DataDesigner()
data_designer.set_run_config(RunConfig(display_tui=False))

results = data_designer.create(
    builder,
    num_records=1000,
    dataset_name="logs-only-demo",
)

Deprecated compatibility shim

Existing callers that still set progress_bar get the same behavior through display_tui, plus a DeprecationWarning.

from data_designer.config import RunConfig

run_config = RunConfig(progress_bar=False)  # Deprecated: use display_tui=False.

Changes

Added

  • New engine progress package:
    • data_designer.engine.progress.tracker for ProgressTracker
    • data_designer.engine.progress.reporter for AsyncProgressReporter
    • data_designer.engine.progress.terminal.throughput_panel for TerminalThroughputPanel
  • RunConfig.display_tui, defaulting to True, as the canonical run config switch for the terminal throughput panel.
  • A deprecated RunConfig.progress_bar compatibility shim that maps to display_tui and emits DeprecationWarning on constructor use, property reads, and property writes.
  • asciichartpy as an engine dependency for terminal chart rendering.
  • Model usage events and request-admission feedback events that let the live panel render model-level rpm, in tok/s, out tok/s, and red feedback markers.
  • data-designer create --tui / --no-tui to force the create command's TUI setting for a single CLI run.

Changed

  • Engine generation paths now read run_config.display_tui; production code no longer reads run_config.progress_bar.
  • The terminal progress display now renders a chart with one records/sec curve per generation column, plus native aligned tables for column progress and model traffic.
  • The chart uses throttled redraws, wider sampling windows, smoothed/fitted series, bounded rate history, stable chart height, and colored per-column progress bars.
  • The column table reports only column-level now rec/s, avg rec/s, and completion progress; token rates are shown in the model-alias table.
  • AsyncTaskScheduler accepts display_tui for async panel setup and passes its run id into AsyncProgressReporter, so global token/admission events from other runs do not pollute the active panel.
  • Interrupt handling and async bridge cleanup avoid scheduling new async work during cancellation/interpreter shutdown.
  • Tests now mirror the new package structure under packages/data-designer-engine/tests/engine/progress/.

Removed

  • The old progress implementation locations under data_designer.engine.dataset_builders.utils.
  • Example/demo scripts and the screenshot asset from the final PR diff; local runnable demos live under .scratch/ only.

Attention Areas

Reviewers: Please pay special attention to the following:

  • throughput_panel.py - terminal rendering, chart/table layout, redraw throttling, feedback marker projection, and TTY fallback behavior.
  • reporter.py - progress aggregation, token/admission event subscriptions, cleanup, and run-id filtering.
  • async_scheduler.py - async progress setup, run correlation, cancellation behavior, and display_tui plumbing.
  • create.py - CLI --tui / --no-tui override surface.
  • run_config.py - canonical display_tui field plus deprecated progress_bar shim behavior.
  • pyproject.toml - adds the asciichartpy runtime dependency.

Validation

Focused config, engine progress/scheduler, CLI, and interface tests were run after merging the latest origin/main; the branch is pushed and up to date.


Description updated with AI

Replace sticky progress bars with a bounded ANSI/asciichart throughput panel that plots records per second per generation column. Default progress_bar to enabled and add a local demo config plus screenshot for PR review.

Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
@eric-tramel eric-tramel marked this pull request as ready for review May 21, 2026 01:57
@eric-tramel eric-tramel requested a review from a team as a code owner May 21, 2026 01:57
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 21, 2026

MkDocs preview: https://bdf16b4e.dd-docs-preview.pages.dev

Fern preview: https://nvidia-preview-pr-696.docs.buildwithfern.com/nemo/datadesigner

Fern previews include the docs-website version archive with PR changes synced into latest. Notebook tutorials are rendered without execution outputs in previews.

@github-actions
Copy link
Copy Markdown
Contributor

Code Review: PR #696 — feat: chart generation throughput

Summary

Replaces the existing sticky fill-bar progress display with a bounded ANSI throughput chart panel rendered via asciichartpy. Key changes:

  • New _BarState.record_update records per-column records/sec samples (gated by a 0.25s minimum interval) and stores them in a rates list.
  • StickyProgressBar._redraw now emits a fixed-height panel (border + header + chart + legend + border) instead of one line per column.
  • update_many accepts both 3-tuple (legacy (completed, success, failed)) and 4-tuple (+ skipped) values; async_progress_reporter and progress_tracker now feed skipped through.
  • log_final / __exit__ no longer remove the bars — the panel stays visible at completion.
  • RunConfig.progress_bar defaults flip from FalseTrue (with non-TTY fallback to log lines).
  • Adds asciichartpy>=1.5.25,<2 runtime dep, an examples/progress_panel_demo.py, and a screenshot asset.
  • Tests are rewritten around the new panel; three former tests (test_drawn_lines_tracks_add_and_remove, test_wrapping_counts_physical_lines, test_wrapping_stable_across_updates) are removed.

Findings

Correctness / Behavior

  • Default flip is a user-facing behavior change. Setting progress_bar=True by default means any TTY caller (CLI, notebook with TTY-like stderr, integrations) now gets a 16-line ANSI panel they didn't ask for. The non-TTY fallback covers CI/pipes, but interactive users running long jobs in terminals that emulate TTY may be surprised. Worth confirming this aligns with broader UX intent and is called out in release notes.
  • Unbounded growth of _BarState.rates. record_update appends one float per sample at a 0.25s minimum cadence with no cap. A long generation (e.g. 1 hour) accumulates ~14,400 samples per column. Display is bounded via _compress_series, but the source list is not — for many columns × long runs this drifts toward MBs of floats. Consider capping (e.g. ring buffer of N samples), since the chart already downsamples for display.
  • average_rate ignores now. _BarState.average_rate(self, now) accepts now but uses self.start_time to compute elapsed, then returns self.completed / elapsed. The now argument is unused in the math but the method takes it; readers will assume it's instantaneous-vs-historical. Either drop the parameter or use it. Minor.
  • progress_tracker.log_final early-returns when bar is active. New code:
    if self._bar is not None and self._bar.is_active:
        self._bar.update(self._bar_key, completed=..., ...)
        return
    if self.completed > 0:
        self._log_progress_unlocked()
    This means in a TTY run, the final consolidated log line (_log_progress_unlocked) no longer fires — the panel update replaces it. That seems intentional ("keep the final chart visible"), but it is a quiet observability regression for anyone who scraped these final log lines. Worth a sentence in the PR body.
  • async_progress_reporter.log_final calls _update_bar. It used to remove all bars via remove_bar(col). Now it simply triggers a final draw. That works, but the panel stays printed in stderr after the run; subsequent stderr output appears below it (no clear). On exit (__exit__) the previous _clear_bars() call has been removed and replaced with self._drawn_lines = 0. This is intentional per the PR description, but it leaves the cursor hide/show pair the only state that gets unwound — verify on a real terminal that the cursor is restored cleanly even if an exception aborts the panel.
  • update_many mixed-tuple polymorphism. The runtime check update[3] if len(update) > 3 else bar.skipped accepts both shapes for the same dict. All in-tree callers were updated to 4-tuples, so the 3-tuple compat path appears to have no users. If it's defensive for external code, keep it; otherwise this is dead complexity worth removing.
  • Panel sizing edge case at very narrow widths. panel_width = max(4, terminal_size.columns - 1). With columns=8, inner_width=5 and max_points = max(2, 5-12) = 2. asciichartpy.plot is then called on 2 samples with a 6-char numeric format prefix; the rendered chart may not fit the inner width, and _fit_ansi will strip ANSI and truncate, losing colors. Not a crash, just visual degradation. Existing test only covers columns=36. Consider an is_active = False short-circuit when the terminal is too narrow to draw a useful chart (e.g. < 30 cols).

Design / Conventions

  • asciichartpy is a tiny (~8KB sdist) pure-Python module; module-level import is fine and doesn't violate the lazy-import guideline. Good.
  • The new module-level constants (_RESET, _BORDER, _TITLE, _MUTED, _FAILED, _OK) are inline ANSI strings. They read fine here, but future style changes will need to find each constant; consider a small _ANSI namedtuple or dataclass if more colors are added.
  • _format_chart_lines's max_points = max(2, inner_width - 12) uses 12 for the y-axis label width. The chart format string is "{:6.1f} " (7 chars) — the relationship between 12 and the format width is implicit. Pull both into named constants (_Y_AXIS_RESERVED = 12, _RATE_FORMAT = "{:6.1f} ") so they don't drift.
  • ProgressUpdate = tuple[int, int, int] | tuple[int, int, int, int] is exported from the module but only used internally; consider keeping this private (_ProgressUpdate) since it's not part of any public API.
  • _visible_len regex was widened from \033\[[0-9;]*m\033\[[0-9;?]*[a-zA-Z] to match private-mode/CSI sequences. That's correct, but the new pattern would also match \033[?25l cursor-show/hide sequences if they ever appeared inside a measured string. Currently they don't (cursor sequences are written separately via _write), so safe; just worth noting in case future code passes a full draw buffer through _visible_len.

Tests

  • New tests cover the 16-line panel height, panel-width bound at narrow terminals, skipped/failed propagation, and update_many semantics. Reasonable coverage of the rewritten surface.
  • Lost coverage:
    • test_wrapping_counts_physical_lines and test_wrapping_stable_across_updates — these guarded against off-by-one regressions in _drawn_lines when individual lines wrap to multiple physical lines. The new code computes self._drawn_lines = len(lines) ignoring physical wrap. If any panel line ever exceeds terminal.columns, redraw will leak (clear too few lines). _panel_line does call _fit_ansi(text, inner_width) to pad/truncate, which should keep the line at exactly inner_width + 2 visible chars — so wrap is unlikely in practice, but a test asserting "no rendered line exceeds panel_width" would lock that invariant.
    • test_drawn_lines_tracks_add_and_removeremove_bar is still on the public API but now untested in the bar-lifecycle sense. If callers still invoke it (the PR removed the only in-tree caller in async_progress_reporter), the behavior of removing a bar mid-run is no longer exercised.
  • Test fragility: the new tests hard-code drawn_lines == 16 and count(CURSOR_UP_CLEAR) == 16, which depend on panel_height = min(_DEFAULT_PANEL_HEIGHT, max(_MIN_PANEL_HEIGHT, terminal.lines - 1)). If a CI runner reports terminal.lines < 17, the test will silently shrink the panel and fail. Consider patching shutil.get_terminal_size to a fixed value in these tests for determinism (the narrow-terminal test already does this).
  • The examples/progress_panel_demo.py is unimported and untested; that's appropriate for a demo, but please confirm the example loaders are excluded from test discovery (they are under examples/, so should be).

Performance

  • _redraw calls asciichartpy.plot on every update/update_many. With the async reporter calling _update_bar periodically (driven by progress_interval, default 5s), this is fine. But update() is also called per-record in some code paths; the rate-sampling guard caps the data appended to rates, but the chart redraw still happens on each call. Worth confirming there's no high-frequency call site that now hits asciichartpy on every record.

Security

  • No security-relevant changes. New dependency asciichartpy is pinned to >=1.5.25,<2 and pulled from PyPI; package is small and pure-Python. Lockfile is updated. No user-controlled data flows into ANSI escape construction beyond column labels (bar.label); a malicious column name containing escape codes would be rendered through _fit_ansi which preserves codes, so a column named "\033[2J\033[H" could clear the screen mid-render. Low-impact (column names are user-authored), but a future hardening could strip control bytes from labels in add_bar.

Verdict

Approve with minor changes. The rewrite is well-scoped, the rendering math is reasonable, and tests cover the new surface adequately. Before merge I'd ask for:

  1. Cap _BarState.rates length (ring buffer or trim) to bound memory on long runs.
  2. Resolve whether the 3-tuple branch in update_many has any external callers; if not, drop it.
  3. Note the progress_bar=True default change in release notes / changelog so users with existing TTY automation aren't surprised.
  4. Either use or drop the now parameter in _BarState.average_rate.
  5. Pin shutil.get_terminal_size in the new height-stability tests to avoid CI flakiness on small-LINES runners.

The remaining points (constant naming, label-escape hardening, lost wrap-test coverage) are nice-to-haves rather than blockers.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 21, 2026

Greptile Summary

This PR replaces the old sticky ANSI progress bars with a bounded throughput panel (TerminalThroughputPanel) that renders a live ASCII chart of records/sec per column, plus model-level token rates. It introduces RunConfig.display_tui (defaulting to True) as the canonical switch, deprecates RunConfig.progress_bar with a shim, and adds --tui / --no-tui CLI overrides for the create command.

  • A new engine.progress package adds ProgressTracker, AsyncProgressReporter, and TerminalThroughputPanel; the old dataset_builders/utils/{sticky_progress_bar,async_progress_reporter,progress_tracker}.py are removed.
  • Global pub/sub buses (subscribe_token_usage, subscribe_request_admission_events) feed token and rate-limit events into the panel; run_id correlation filtering prevents cross-run event pollution.
  • AsyncTaskScheduler wires in display_tui, a per-run cancellation Event, and run_id context, while _await_async_scheduler_result adds KeyboardInterrupt → graceful cancel bridging.

Confidence Score: 5/5

Safe to merge. The async engine path is well-wired: single panel, throttled redraws, run_id filtering for cross-run event isolation, and proper cleanup in finally blocks.

The core async path — panel creation, event subscription, redraw throttling, run_id correlation, and subscriber cleanup — is correctly implemented. The two observations are limited to the sync engine path (multiple non-clearing panels per run, empty model table), which is a lower-traffic code path and does not affect correctness of generation output.

dataset_builder.py sync-path panel usage (_fan_out_with_threads / _fan_out_with_async) — one panel is created per column batch and none are cleared on exit, which accumulates panels in multi-column sync runs.

Important Files Changed

Filename Overview
packages/data-designer-engine/src/data_designer/engine/progress/terminal/throughput_panel.py New 873-line TUI panel: ANSI chart via asciichartpy, per-column progress bars, model-alias table, feedback marker overlay, throttled redraws (0.75s min interval), handler wrapping for log interleaving, and TTY fallback. Logic is well-structured; exit intentionally leaves the drawn panel visible.
packages/data-designer-engine/src/data_designer/engine/progress/reporter.py AsyncProgressReporter correctly throttles bar redraws (0.75s), filters events by run_id, and closes subscriptions in a finally block. No issues found.
packages/data-designer-engine/src/data_designer/engine/dataset_builders/async_scheduler.py Single-panel async progress wiring, run_id correlation, and reporter.close() in finally are all correct. Adds KeyboardInterrupt to request_cancel bridging and current_generation_column/cancel_event context propagation.
packages/data-designer-config/src/data_designer/config/run_config.py Adds display_tui (default True), progress_bar deprecated shim with DeprecationWarning on read/write/constructor use. Model validator handles backward compatibility via setdefault.
packages/data-designer-engine/src/data_designer/engine/dataset_builders/dataset_builder.py Switches from StickyProgressBar to TerminalThroughputPanel and adds KeyboardInterrupt-aware future awaiting. Sync path creates one panel per column batch, leaving multiple panels on screen for multi-column runs; model-alias table will also be empty on the sync path.
packages/data-designer-engine/src/data_designer/engine/models/usage_events.py New module-level pub/sub bus for token usage events; thread-safe with proper unsubscribe pattern, exception isolation per callback.
packages/data-designer-engine/src/data_designer/engine/observability.py Adds run_id to RuntimeCorrelation and a parallel pub/sub bus for request admission events. Same thread-safe pattern as usage_events.py.
packages/data-designer/src/data_designer/cli/commands/create.py Adds --tui/--no-tui optional flag (default None = defer to RunConfig). Clean integration.
packages/data-designer-engine/src/data_designer/engine/progress/tracker.py ProgressTracker refactored to new progress package; double-lock pattern in _record_completion is sound (interval advancement is atomic in first lock, preventing duplicate log emissions).

Sequence Diagram

sequenceDiagram
    participant CLI as CLI create
    participant GC as GenerationController
    participant DB as DatasetBuilder
    participant AS as AsyncTaskScheduler
    participant TTP as TerminalThroughputPanel
    participant APR as AsyncProgressReporter
    participant MF as ModelFacade
    participant UE as usage_events bus

    CLI->>GC: "run_create(tui=override)"
    GC->>DB: create(...)
    DB->>AS: "__init__(display_tui=True, run_id=...)"
    AS->>TTP: TerminalThroughputPanel()
    AS->>APR: AsyncProgressReporter(trackers, bar, run_id)
    APR->>UE: subscribe_token_usage(_record_token_usage)

    DB->>AS: run()
    AS->>TTP: __enter__() [activates, wraps log handlers]

    loop per async task
        AS->>MF: acompletion(...)
        MF->>UE: emit_token_usage_event(event, run_id)
        UE->>APR: _record_token_usage(event)
        APR->>TTP: record_model_usage(...)
        AS->>APR: record_success(column)
        APR->>APR: _maybe_report() [throttled 0.75s]
        APR->>TTP: update_many(...)
    end

    AS->>APR: log_final() then close() [unsubscribes]
    AS->>TTP: __exit__() [panel stays visible]
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
packages/data-designer-engine/src/data_designer/engine/dataset_builders/dataset_builder.py:1453-1458
**Sync-path creates a new panel per column, leaving all panels on screen**

Each call to `_fan_out_with_async` / `_fan_out_with_threads` constructs a fresh `TerminalThroughputPanel` and enters its context manager. Because `TerminalThroughputPanel.__exit__` intentionally does not clear its drawn lines (it leaves the final state as a persistent summary), every column batch in a sync-engine run adds ~22 lines of chart to the terminal before drawing the next one. A dataset with five LLM columns will leave five stacked panels on screen. The async engine avoids this because `AsyncTaskScheduler` creates a single panel for all columns. Consider clearing the panel in `__exit__` on the sync path, or constructing one shared panel above these call sites that is reused across columns.

### Issue 2 of 2
packages/data-designer-engine/src/data_designer/engine/dataset_builders/dataset_builder.py:1453-1460
**Model-alias table always empty in sync-engine runs**

`ModelFacade._track_usage` calls `emit_token_usage_event` on every completion, but that event bus is only subscribed by `AsyncProgressReporter.__init__`, which is never constructed in the sync path. `TerminalThroughputPanel` here receives record-count updates via `ProgressTracker.update`, but no subscriber calls `record_model_usage`, so the model-alias table in the panel will remain blank for all sync-engine generations. The async path renders the table correctly. This is worth a note in the PR for clarity if the empty model table is intentional for the sync path.

Reviews (3): Last reviewed commit: "test: make throughput redraw assertion l..." | Re-trigger Greptile

Throttle active TTY redraws, sample rates over larger windows, smooth and fit chart series, bound rate history, and harden panel tests.

Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
@eric-tramel eric-tramel marked this pull request as draft May 21, 2026 17:06
Render the progress legend as a stable table with live token-rate columns, attribute model usage to active generation columns across async bridge boundaries, and cancel the async scheduler cleanly on KeyboardInterrupt.

Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
Emit synthetic token usage in the credential-free progress panel demo so the live token-rate columns are visible, and accept output-only provider usage as a progress event.

Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
Replace the pipe-delimited progress legend with a native spaced layout, keep column alignment via computed widths, mute the header row, and refresh the PR screenshot.

Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
Show raw column names in the async progress panel legend because the table header already provides the column context, and refresh the PR screenshot.

Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
Label the progress legend's now and average record-rate columns as rec/s and refresh the screenshot.

Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
@eric-tramel eric-tramel self-assigned this May 22, 2026
…hput-panel

# Conflicts:
#	packages/data-designer-config/tests/config/test_run_config.py
#	uv.lock
@eric-tramel eric-tramel marked this pull request as ready for review May 22, 2026 02:00
@eric-tramel eric-tramel deployed to agentic-ci May 22, 2026 02:00 — with GitHub Actions Active
@github-actions
Copy link
Copy Markdown
Contributor

Code Review — PR #696: feat: chart generation throughput

Summary

This PR replaces the old StickyProgressBar ANSI progress bars with a new TerminalThroughputPanel that renders a bounded asciichartpy chart plus column / model usage tables. The setting is canonicalised as RunConfig.display_tui (default True); RunConfig.progress_bar becomes a deprecated shim. Adjacent infrastructure is added: a new data_designer.engine.progress package, global TokenUsageEvent + RequestAdmissionEvent pub/sub, run-id filtering in the reporter, a CLI --tui/--no-tui flag, and propagation of cancellation context into the sync→async bridge so KeyboardInterrupt actually unwinds in-flight worker threads.

Footprint: 32 files, +2188/-668. Engine, config, and CLI packages all touched. Layering direction (interface → engine → config) is preserved.

Findings

Behaviour / correctness

  • Default flip from FalseTrue is a user-visible behaviour change. Anyone running interactively will now see the TUI by default. The non-TTY fallback in TerminalThroughputPanel.__enter__ keeps CI/notebook output clean, but the change-of-default (progress_bar=Falsedisplay_tui=True) is effectively a behaviour bump for end users who previously relied on the periodic log lines in a TTY. Worth calling out in release notes; the PR description does mention it.
  • RunConfig.progress_bar property on a Pydantic v2 model is a sharp edge. progress_bar is a @property on RunConfig (not a model field). model_copy(update={"progress_bar": True}) will not round-trip through the model_validator(mode="before") because model_copy skips validators; it will set an attribute that the property tries to shadow. If users were calling run_config.model_copy(update={"progress_bar": ...}), that path is now silently broken. Constructor-style (RunConfig(progress_bar=...)) and direct attribute reads/writes are covered by tests. Consider either documenting the migration, adding a model_copy/model_validate test for the legacy key, or detecting the case explicitly. (run_config.py:230-247)
  • translate_deprecated_fields has an unreachable return normalized. The if "throttle" in normalized branch ends with return normalized and is followed by the same return normalized outside the if. Harmless, minor cleanup. (run_config.py:227-228)
  • AsyncProgressReporter._matches_run uses correlation: object. The runtime branches over None, dict, and dataclass-via-getattr. RequestAdmissionEvent.captured_correlation is a dataclass and TokenUsageEvent.correlation is RuntimeCorrelation | None, so the dict branch is defensive only. The looser typing is fine, but a RuntimeCorrelation | Mapping[str, Any] | None annotation would make intent clearer and match what's tested. (reporter.py:165)
  • Module-level pub/sub state is process-global. _callbacks in usage_events.py and _request_event_callbacks in observability.py are module-level dicts. The reporter properly unsubscribes in close() (called from both log_final and the scheduler finally), and the run-id filter prevents cross-run contamination, so this is OK. Two latent risks worth mentioning in a comment near each registry:
    • In test runs that crash before close(), callbacks accumulate across tests in the same process.
    • Subscriber callbacks are invoked synchronously on the model request hot path (facade.py: _track_usage). Slow/blocking subscribers will slow down generation. Today the only subscriber is the panel and its work is in-memory state, so it's fine — but the contract should be explicit.
  • TerminalThroughputPanel.record_feedback_signal projects the y-coord using latest_rate at event time. latest_rate is only refreshed every _RATE_SAMPLE_INTERVAL_SECONDS (2.0s). A feedback event between samples lands at a slightly stale y-position. Acceptable for a visual marker; not a correctness issue.
  • _overlay_feedback_markers x-positioning math. int(round(marker_elapsed / current_elapsed * (plot_column_count - 1))) with plot_column_count = max(1, point_count - 1). When plot_column_count == 1, this multiplies by 0, so all markers stack at column 0. Edge case for very-early feedback signals; visually fine.
  • Bridge cancellation propagation. agenerate_with_bridge_context correctly suppresses ValueError on context-var reset (cross-thread close) and surfaces concurrent.futures.CancelledError as asyncio.CancelledError. The is_run_cancellation_requested() short-circuit before run_coroutine_threadsafe is the right place to bail out. Test test_async_bridge_obeys_run_cancellation_before_scheduling covers it. (custom.py:108-141)

Style / conventions

  • asciichartpy is imported at module top of throughput_panel.py. AGENTS.md mandates lazy-loading "heavy third-party libraries" via data_designer.lazy_heavy_imports. asciichartpy is tiny (~7 KB wheel, pure Python), so this is unlikely to regress import time — but make perf-import CLEAN=1 should be run to confirm. More notably, from data_designer.engine.progress.terminal.throughput_panel import TerminalThroughputPanel is at the top of both async_scheduler.py and dataset_builder.py, so the panel module is now imported unconditionally even when display_tui=False. Cheap today, worth flagging if the panel grows new heavy deps later.
  • Deprecation surface is consistent with the existing throttle shim. Same module-level message constant, same model_validator(mode="before") pattern, same stacklevel=2. Good.
  • CLI --tui/--no-tuitui: bool | None = None correctly distinguishes "not set" from "explicit value", and the controller only mutates RunConfig when the override is present. (generation_controller.py:147-149)

Testing

Coverage is strong:

  • Deprecation warnings on constructor, property getter, property setter.
  • TUI override plumbing through CLI → controller → DataDesigner.
  • Bridge cancellation behaviour and column-context preservation.
  • Token usage event filtering by run_id.
  • Throughput panel rendering: header, column / model tables, narrow-terminal fallback, control-sequence sanitisation, redraw throttling, log interleaving, marker reprojection, rate-sample bounds.
  • Old sticky-bar tests are deleted; equivalent assertions exist in the new suite.

Gaps / nice-to-haves:

  • No test exercises request_cancel end-to-end through _execute_task_inner — the existing test mocks the future. A small unit covering "cancel flag flips and a bridge worker raises CancelledError" would close the loop, though it's hard to write deterministically.
  • RunConfig.model_copy(update={"progress_bar": ...}) is not covered. If you intend to support that, add a test; if not, document.
  • The reporter _emit path passes 0.0 to get_snapshot purely to read the completed counter (reporter.py:172). A short comment explaining why would prevent a future cleanup from "fixing" it to time.perf_counter() - self._start_time and changing semantics.

Performance

  • Per-request token-usage emit + admission emit add a lock acquire and a small tuple copy each. With 1 subscriber this is trivial; for high-RPS runs (10k+ req/s) it's still well under the noise floor.
  • Redraw is throttled to _MIN_REDRAW_INTERVAL_SECONDS = 0.75, and the reporter only forwards updates at DEFAULT_TTY_REPORT_INTERVAL = 0.75. ANSI clear-and-redraw happens at most ~1.3 Hz in steady state. Matches the existing sticky-bar behaviour.
  • The 22-line panel performs ~22 cursor-up/erase escape sequences per redraw. Fine for terminal emulators but could flicker over slow ssh; not new — same characteristic as the old sticky bar.

Security

Nothing new exposed. ANSI sanitisation in _sanitize_label strips control characters from incoming labels and model names before they reach the terminal. Good defensive posture given that model aliases and column names can come from user-supplied configs.

Verdict

Looks good to merge with minor follow-ups:

  1. (Recommended) Decide on RunConfig.model_copy(update={"progress_bar": ...}) semantics — either test+document or actively reject in the validator.
  2. (Optional) Drop the unreachable return normalized after the throttle branch in translate_deprecated_fields.
  3. (Optional) Tighten the type annotation on AsyncProgressReporter._matches_run.
  4. (Optional) Run make perf-import CLEAN=1 to confirm asciichartpy doesn't regress engine import time, since it is loaded eagerly through async_scheduler.py / dataset_builder.py.
  5. (Optional) Brief comment near the global pub/sub registries noting that subscribers must be cheap, non-blocking, and that close() is mandatory.

Architectural direction is sound: the new progress package sits cleanly under engine, the deprecation path mirrors the existing throttle shim, and the cancellation plumbing into the sync bridge is a real correctness improvement that goes beyond the visible UI change.

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