Skip to content

feat: chart generation throughput#696

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

feat: chart generation throughput#696
eric-tramel wants to merge 27 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://7f35b409.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 fill/disappear ANSI progress bars with a bounded throughput panel backed by asciichartpy, gated by a new RunConfig.display_tui field (default True). The progress_bar field becomes a deprecated property shim with deprecation warnings. The async scheduler gains run-ID correlation, a _cancel_requested Event for graceful Ctrl+C handling, and proper outer-finally cleanup that always unsubscribes token/admission event callbacks.

  • New progress stack: ProgressTracker (per-column), AsyncProgressReporter (reporter + event subscriptions), and TerminalThroughputPanel (ANSI chart + column/model tables) form a three-layer system; the panel uses a 0.75s redraw throttle and wraps root-logger handlers so log messages print above the live panel.
  • Config migration: RunConfig.progress_bar is removed as a pydantic field and re-exposed as a Python property that delegates to display_tui; the translate_deprecated_fields model validator, property getter/setter, and model_copy override all emit DeprecationWarning and are backed by new tests.
  • Scheduler hardening: AsyncTaskScheduler.request_cancel() sets a threading Event that bridged sync work checks via is_run_cancellation_requested(); _await_async_scheduler_result catches KeyboardInterrupt, signals cancellation, and waits for the scheduler to drain before re-raising.

Confidence Score: 5/5

Safe to merge; the new throughput panel, config migration, and scheduler hardening are all well-structured with thorough tests.

The architectural change is sound: the three-layer progress stack has clean ownership and correct lock ordering. The deprecated progress_bar shim handles all four consumer surfaces. The scheduler cancellation signaling and proper outer-finally cleanup for the reporter are correct. No functional regressions were found in the changed paths.

No files require special attention, though reviewers may want to spot-check the async-vs-sync panel label consistency in reporter.py and the ANSI truncation edge case in throughput_panel.py noted in the inline comments.

Important Files Changed

Filename Overview
packages/data-designer-engine/src/data_designer/engine/progress/terminal/throughput_panel.py New 885-line ANSI panel: chart rendering, column/model tables, feedback marker overlay, redraw throttling, handler wrapping, TTY fallback. Well-structured with lock discipline; redraw is correctly throttled at 0.75s via _redraw_if_due.
packages/data-designer-engine/src/data_designer/engine/progress/reporter.py New AsyncProgressReporter: owns per-column ProgressTracker instances, drives panel updates at DEFAULT_TTY_REPORT_INTERVAL (0.75s), subscribes to token/admission events, and filters by run_id. close() is idempotent. Subscriptions are correctly set up in log_start (single-threaded) before workers spawn.
packages/data-designer-engine/src/data_designer/engine/dataset_builders/async_scheduler.py Swapped StickyProgressBar for TerminalThroughputPanel, added _cancel_requested Event + request_cancel(), added CancelledError → request_cancel() handling in dispatch loop, and added outer finally to always close the reporter. Context vars for column and cancel event set/reset correctly around task execution.
packages/data-designer-config/src/data_designer/config/run_config.py display_tui replaces progress_bar as the canonical field (default True). Deprecated shim via model_validator, property getter/setter, and model_copy override; all emit DeprecationWarning. setdefault ensures display_tui wins when both are present.
packages/data-designer-engine/src/data_designer/engine/dataset_builders/dataset_builder.py Replaced StickyProgressBar with TerminalThroughputPanel, added _await_async_scheduler_result helper for KeyboardInterrupt → cancel propagation, updated run_config.display_tui reads.
packages/data-designer-engine/src/data_designer/engine/progress/tracker.py Moved from utils/ to progress/ package. Minor additions: panel integration (add_bar, update) and quiet mode. Thread-safe via Lock with correct double-acquisition pattern for log-interval checks.
packages/data-designer-engine/src/data_designer/engine/models/usage_events.py New module-level pub/sub for TokenUsageEvent, with Lock-protected callback dict. Pattern mirrors the RequestAdmissionEvent subscriber in observability.py.
packages/data-designer-engine/src/data_designer/engine/models/facade.py _record_usage now allows output-only token events (input_tokens can be None), emits TokenUsageEvent after recording stats, and resolves column from context var or correlation.
packages/data-designer-engine/src/data_designer/engine/column_generators/generators/custom.py _AsyncBridgedModelFacade now checks is_run_cancellation_requested() before submitting work and propagates column/cancel context vars into the async coroutine; handles CancelledError and interpreter-shutdown RuntimeError on scheduling.
packages/data-designer/src/data_designer/cli/commands/create.py Adds --tui/--no-tui flag that overrides display_tui for a single run by calling model_copy on the active run config before generation starts.
packages/data-designer-engine/tests/engine/progress/terminal/test_throughput_panel.py 553-line test suite with FakeTTY fixture, covering add/update/remove_bar, chart/legend rendering, feedback markers, model table, log interleaving, reporter integration, run-id filtering, and TTY fallback.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI (create.py)
    participant GC as GenerationController
    participant DB as DatasetBuilder
    participant AS as AsyncTaskScheduler
    participant Panel as TerminalThroughputPanel
    participant Reporter as AsyncProgressReporter
    participant Worker as Task Workers
    participant Facade as ModelFacade

    CLI->>GC: "create(..., tui=True)"
    GC->>GC: "run_config.model_copy(update={display_tui: tui})"
    GC->>DB: create(builder, num_records)
    DB->>AS: "AsyncTaskScheduler(display_tui=True)"
    AS->>Panel: TerminalThroughputPanel()
    AS->>Reporter: "AsyncProgressReporter(trackers, progress_bar=panel)"

    DB->>AS: run() via thread future
    AS->>Panel: __enter__() hide cursor, wrap log handlers
    AS->>Reporter: log_start() _ensure_event_subscriptions()
    Reporter-->>usage_events: subscribe_token_usage()
    Reporter-->>observability: subscribe_request_admission_events()

    loop dispatch loop
        AS->>Worker: spawn task sets column/cancel context vars
        Worker->>Facade: agenerate()
        Facade-->>usage_events: emit_token_usage_event(TokenUsageEvent)
        usage_events-->>Reporter: _record_token_usage()
        Reporter->>Panel: record_model_usage()
        Worker->>Reporter: record_success(column)
        Reporter->>Reporter: _maybe_report() throttled 0.75s
        Reporter->>Panel: update_many()
        Panel->>Panel: _redraw_if_due() _format_panel() + asciichartpy.plot()
    end

    AS->>Reporter: log_final() close() unsubscribe
    AS->>Panel: __exit__() show cursor, unwrap log handlers
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/progress/reporter.py:51-54
The `add_bar` calls in `__init__` use the raw column key as both the key and the label, while `ProgressTracker` (in the sync fan-out path) registers bars with the richer `label` string (e.g. `"column 'foo'"`). In the async path the panel will show `"foo"` instead of `"column 'foo'"`. Using `tracker.label` here aligns the async panel display with the sync path and with the log output emitted by `log_start`.

```suggestion
        if self._bar is not None:
            for col, tracker in trackers.items():
                self._bar.add_bar(col, tracker.label, tracker.total_records)
            self._ensure_event_subscriptions()
```

### Issue 2 of 2
packages/data-designer-engine/src/data_designer/engine/progress/terminal/throughput_panel.py:70-74
When `_fit_ansi` truncates a string it strips all ANSI codes and slices the plain text, which correctly avoids leaving open escape sequences. However it also drops the trailing `_RESET` that callers such as `_format_progress_bar` always append. If a colored progress bar fills exactly `inner_width` visible characters and gets truncated, the open color code bleeds into the next rendered line. Adding an explicit reset at the end of the truncation path closes this edge case.

```suggestion
def _fit_ansi(text: str, width: int) -> str:
    visible = _visible_len(text)
    if visible > width:
        return _ANSI_RE.sub("", text)[:width] + _RESET
    return text + (" " * (width - visible))
```

Reviews (9): Last reviewed commit: "Merge branch 'main' into codex/progress-..." | 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
@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.

…hput-panel

# Conflicts:
#	packages/data-designer-engine/src/data_designer/engine/dataset_builders/async_scheduler.py
#	packages/data-designer-engine/src/data_designer/engine/models/facade.py
#	packages/data-designer-engine/tests/engine/dataset_builders/test_async_builder_integration.py
@nabinchha
Copy link
Copy Markdown
Contributor

Nice work on this one, @eric-tramel - the new progress package is thoughtfully separated and the compatibility path is in good shape.

Summary

This PR replaces the old sticky progress bars with a terminal throughput panel, wires model/request-admission events into the live display, makes RunConfig.display_tui the canonical switch, and adds CLI --tui/--no-tui overrides. The implementation matches the PR description overall, but I found a couple of user-facing polish issues that are worth tightening before merge.

Findings

Warnings - Worth addressing

packages/data-designer-engine/src/data_designer/engine/progress/terminal/throughput_panel.py:595 — Legend rows can exceed the terminal height

  • What: _format_panel() clamps panel_height to the terminal size, but _format_legend_lines() still appends every column row and every model row. With display_tui=True by default, a dataset with many generation columns or model aliases can draw a panel much taller than the viewport.
  • Why: Once _drawn_lines grows past the terminal height, every redraw emits a large stack of cursor-up/clear sequences. That can scroll the terminal, erase useful logs, and make the default progress UI noisy for wide configs.
  • Suggestion: Treat minimum_legend_capacity as an actual max budget too: render the first N column/model rows that fit, then add a muted overflow row like ... 12 more column(s), 3 more model(s). A focused test with a short terminal and many columns/models would lock in the bounded behavior.

fern/versions/latest/pages/devnotes/posts/async-all-the-way-down.mdx:182 — Public docs still describe the removed sticky bars

  • What: The page updates the RunConfig(display_tui=True) snippet, but the surrounding prose and sample output still describe "sticky ANSI bars" that update on every task completion. The mirrored docs/devnotes/posts/async-all-the-way-down.md:183 has the same stale text.
  • Why: This PR removes that implementation and replaces it with the throughput chart/table panel, plus throttled redraws and model traffic rows. Readers following the updated snippet will see something materially different from the docs.
  • Suggestion: Update the prose and sample block in both the Fern page and the docs mirror to describe the throughput panel, redraw throttling, chart/table layout, and model traffic display.

Suggestions - Take it or leave it

uv.lock:285 — Orphan astroid lock entry

  • What: The lockfile adds astroid==3.3.11, but no changed pyproject.toml or lockfile dependency references astroid.
  • Why: It looks like merge or resolver churn unrelated to asciichartpy; leaving orphan lock entries makes future dependency diffs harder to read.
  • Suggestion: Regenerate the lockfile from the updated dependency set, or otherwise drop the unused astroid entry if it is not actually required.

What Looks Good

  • The deprecated RunConfig.progress_bar shim is well covered: constructor, property access, setter, and model_copy(update=...) all preserve compatibility while steering callers toward display_tui.
  • The progress event plumbing is nicely scoped. AsyncProgressReporter.close() unsubscribes from global callbacks, and the run-id filter avoids cross-run token/admission events leaking into the active panel.
  • The focused panel tests cover the important terminal mechanics: non-TTY fallback, redraw throttling, log interleaving, sanitization, rate-history bounds, feedback markers, and run-id filtering.

Verdict

Needs changes - please bound the panel height for wide configs and refresh the stale progress-bar docs before merge. The lockfile cleanup is optional but would make the dependency diff cleaner.


This review was generated by an AI assistant.

Copy link
Copy Markdown
Contributor

@johnnygreco johnnygreco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really cool @eric-tramel!

I just have some nits related to more explicit labeling and logging. Would be nice if we could get "Preparing samplers to generate 50 records across 5 columns" to print before we start logging above the TUI. And can we have more explicit logs and/or labeling to make super clear that we are showing you llm/model-generated columns in the TUI? Of course, it is obvious when you think about it, but I could see users being confused mid-generation to see progress bars for some but not all columns.

The other nit is with all the labels at the top of graph, which shows the y-axis label (rec/s) plus the total stats. How flexible is the layout? Can we have just records/s on the left and then justify the total stats on the right or something like this? As is, it took me a second to understand "now 1.4" in │Throughput rec/s | 351.6s | 150/150 | now 1.4 | 0 failed, for example.

Image

@johnnygreco
Copy link
Copy Markdown
Contributor

@mikeknep – heads up about this new tui

@nabinchha
Copy link
Copy Markdown
Contributor

Nice iteration on the follow-up commits. I re-reviewed latest head e2f03b70; the progress_bar compatibility path is now covered through model_copy, rate samples are bounded, and the stray astroid lockfile entry is gone.

Summary

This PR replaces the old sticky progress bars with the new TerminalThroughputPanel, makes display_tui the canonical RunConfig flag with --tui/--no-tui CLI overrides, and wires token/request-admission events into the live display. The architecture still looks clean, but two user-facing issues from the previous review remain in the latest head.

Findings

Warnings - Worth addressing

packages/data-designer-engine/src/data_designer/engine/progress/terminal/throughput_panel.py:493 — Dense panels still exceed the terminal height

  • What: _format_panel() clamps panel_height, then builds all column/model legend rows and only shrinks the chart down to _MIN_CHART_LINE_COUNT; once the legend itself is larger than the remaining viewport, the final panel grows past the terminal height. The new regression test locks in that behavior with assert len(panel_lines) > 22 for 8 columns plus 8 models.
  • Why: With display_tui=True by default, wide configs can still draw a default-on panel taller than the viewport. Redraws then emit cursor-up/clear sequences for more lines than fit on screen, which can scroll or erase nearby logs and make the TUI noisy in exactly the dense case the latest commit is meant to handle.
  • Suggestion: Treat the legend area as a hard budget after the chart reaches its minimum height. Render the rows that fit, then add a muted overflow summary such as ... 5 more column(s), 4 more model(s). The dense-table test should assert len(panel_lines) <= terminal.lines - 1 instead of > 22.

fern/versions/latest/pages/devnotes/posts/async-all-the-way-down.mdx:182 — Dev note still describes the removed sticky-bar UI

  • What: The snippet was updated to RunConfig(display_tui=True), but the surrounding prose and code block still describe “sticky ANSI bars” that redraw on every task completion and show the old per-column bar output. The mirrored docs/devnotes/posts/async-all-the-way-down.md:183 has the same stale section, and the later build-history sentence still says PR chore: async engine follow-up - rename, preview, lifecycle, progress #456 added sticky ANSI progress bars.
  • Why: Readers following the updated config will now see the throughput chart plus column/model tables, throttled redraws, and request/model feedback, not the old four-line progress bar example. This makes the public docs materially misleading.
  • Suggestion: Update the prose and sample output in the Fern page, and keep the docs mirror consistent if it is still maintained in this PR.

What Looks Good

  • The RunConfig.progress_bar deprecation shim now covers constructor input, property get/set, and model_copy(update={"progress_bar": ...}), with tests for precedence when display_tui is also supplied.
  • The TUI event subscriptions are run-id filtered and unsubscribed via AsyncProgressReporter.close(), including the scheduler finally path.
  • The focused panel suite now covers non-TTY fallback, cursor restoration, redraw throttling, model-usage rows, label sanitization, feedback markers, bounded rate history, and CLI/controller plumbing.

Validation

  • ruff check: passed
  • ruff format --check: passed
  • Focused pytest suite: 398 passed
  • fern check: not run locally because the fern CLI is not installed in this worktree environment.

Verdict

Needs changes - please bound dense TUI panels to the terminal height and refresh the stale progress UI docs before merge.


This review was generated by an AI assistant.

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.

3 participants