Skip to content

fix: post token tracking nits (follow up on #392) - #429

Merged
seayang-nv merged 11 commits into
mainfrom
seayang/426-post-token-tracking-nits
Apr 28, 2026
Merged

fix: post token tracking nits (follow up on #392)#429
seayang-nv merged 11 commits into
mainfrom
seayang/426-post-token-tracking-nits

Conversation

@seayang-nv

@seayang-nv seayang-nv commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses the four nit suggestions @binaryaaron left on #392 after the main token-tracking refactor was merged. No behavior change — all four are cleanups / readability improvements around code introduced in that PR.
Closes #426.

Changes

1. results.py — collapse repetitive token-field wiring

Replaced the 13-line block of num_valid_records = None, if isinstance(results, GenerateJobResults): num_valid_records = results.num_valid_records, ..., SafeSynthesizerSummary(num_valid_records=num_valid_records, ...) with:

  • _GENERATE_RESULT_FIELDS tuple — names that pass through verbatim from GenerateJobResults to SafeSynthesizerSummary.
  • _REPORT_SCORE_FIELDS dict — {summary_field: report_score_name} for report-derived scores.
    make_nss_summary now does a single getattr loop for the gen fields, computes valid_record_token_fraction inline, projects report scores (or None) via a dict comprehension, and **-unpacks both into one SafeSynthesizerSummary(...) call. ~45 lines gone.

2. batch.py — single-pass token aggregation

Added a private _record_token_totals() -> (valid, invalid) that walks self._responses exactly once. The three token properties all share it:

  • total_valid_record_tokens / total_invalid_record_tokens destructure one side.
  • total_non_record_tokens gets both and reuses them for the clamped-to-zero warning context (so the extra={} payload no longer re-triggers the scan).

3. processors.py — extract _valid_parsed helper

The comprehension [r.parsed for r in group_records if r.is_valid and r.parsed is not None] appeared twice inside GroupedDataProcessor._process_text_generation (non-unique-groupby check and order_by check). Hoisted into a module-level helper with a docstring explaining the is not None narrows the type for static analysis (valid records always carry parsed).

4. record_utils.py — reframe ParsedResponse docstring

Dropped the "backward compatibility with callers that want the old parallel-list shape" framing. The doc now describes valid_records / invalid_records / errors as convenience views that project records into the shapes expected by downstream aggregation code (parsed dicts / original text / (msg, validator) tuples).

Pre-Review Checklist

Ensure that the following pass:

  • make format && make check or via prek validation.
  • make test passes locally
  • make test-e2e passes locally
  • make test-ci-container passes locally (recommended)
  • GPU CI status check passes -- comment /sync on this PR to trigger a run (auto-triggers on ready-for-review)

Pre-Merge Checklist

  • New or updated tests for any fix or new behavior
  • Updated documentation for new features and behaviors, including docstrings for API docs.

Other Notes

seayang-nv and others added 10 commits April 17, 2026 14:02
Signed-off-by: Sean Yang <seayang@nvidia.com>
Signed-off-by: Sean Yang <seayang@nvidia.com>
Signed-off-by: Sean Yang <seayang@nvidia.com>
Signed-off-by: seayang <seayang@nvidia.com>
Signed-off-by: Sean Yang <seayang@nvidia.com>
Signed-off-by: Sean Yang <seayang@nvidia.com>
Signed-off-by: Sean Yang <seayang@nvidia.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Signed-off-by: seayang-nv <seayang@nvidia.com>
Signed-off-by: Sean Yang <seayang@nvidia.com>
Signed-off-by: Sean Yang <seayang@nvidia.com>
Signed-off-by: Sean Yang <seayang@nvidia.com>

# Conflicts:
#	src/nemo_safe_synthesizer/data_processing/record_utils.py
#	src/nemo_safe_synthesizer/generation/batch.py
#	src/nemo_safe_synthesizer/generation/processors.py
#	src/nemo_safe_synthesizer/results.py
@seayang-nv seayang-nv changed the title Seayang/426 post token tracking nits fix: post token tracking nits (follow up on #392) Apr 22, 2026
@seayang-nv
seayang-nv marked this pull request as ready for review April 22, 2026 17:53
@seayang-nv
seayang-nv requested a review from a team as a code owner April 22, 2026 17:53
@codecov

codecov Bot commented Apr 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@binaryaaron binaryaaron added the feature New feature or request label Apr 23, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Follow-up cleanup PR that refactors recently introduced token-tracking and parsed-record codepaths for readability and minor efficiency improvements, without changing intended behavior (per PR description and review of the diffs).

Changes:

  • Refactors make_nss_summary to populate SafeSynthesizerSummary via field lists/dicts instead of repetitive local wiring.
  • Avoids redundant scanning of batch responses when computing non-record token totals and warning context.
  • Extracts a shared helper for “valid parsed dicts” in grouped processing and updates ParsedResponse docstring to reflect current usage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/nemo_safe_synthesizer/results.py Collapses repetitive summary-field forwarding into _GENERATE_RESULT_FIELDS + _REPORT_SCORE_FIELDS driven dict construction.
src/nemo_safe_synthesizer/generation/batch.py Introduces _record_token_totals() to reuse a single scan of responses for token aggregations (notably for total_non_record_tokens).
src/nemo_safe_synthesizer/generation/processors.py Extracts _valid_parsed() helper to remove duplicated comprehensions in grouped record validation logic.
src/nemo_safe_synthesizer/data_processing/record_utils.py Updates ParsedResponse docstring to describe valid_records/invalid_records/errors as primary convenience views.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/nemo_safe_synthesizer/results.py
@seayang-nv
seayang-nv merged commit 11580d4 into main Apr 28, 2026
14 checks passed
@seayang-nv
seayang-nv deleted the seayang/426-post-token-tracking-nits branch April 28, 2026 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nits: readability / perf / doc cleanups after generation-token tracking (PR #392)

4 participants