feat(generation): Track generation tokens and refactor with ParsedRecord - #392
Conversation
There was a problem hiding this comment.
Overall looks good!
Love that you have a test plan! I think some end-to-end testing should be added to the testing plan, like can we see what the changes look like in
- the log (you already have that)
- wandb
- the results summary json
- whatever else is changed
in the three modes
- non-group by
- group by
- time series?
kendrickb-nvidia
left a comment
There was a problem hiding this comment.
A few small comments, and then majority are around ExtractionResult and these parallel lists of valid, invalid, errors, valid tokens, invalid tokens that we're passing all over. I think the code would be simpler if we switch to lists of ParsedRecord where the ParsedRecord instance contains an is_valid, error messages, # tokens, etc. Instead of the ParsedResponse and ExtractionResult with several parallel lists. But that would also be a bigger refactor (and may be more complicated than I think).
Curious on other thoughts on the patterns @binaryaaron @mckornfield . @seayang-nv let's chat more tomorrow.
There was a problem hiding this comment.
Overall, looks good.
main issues
- Training path - should we do this? can we add it?
src/nemo_safe_synthesizer/training/huggingface_backend.py:507:create_processor(...)is called withouttokenizer=, soInferenceEvalCallbackcan't collect token metrics.self.tokenizeris in scope.src/nemo_safe_synthesizer/training/callbacks.py:156:batch.process(idx, text)doesn't passcompletion_tokens=, so this path always records 0 tokens and thetotal_completion_tokens > 0gates downstream drop the metrics.
Follow-ups
-
Batch-tokenize: one
tokenizer(list_of_strings, add_special_tokens=False, return_length=True)call per extraction instead of N·K individualtokenizer.encode(str)calls. Gate on measured cost — instrumenttokenization_overhead_sec / generation_time_secin production first. <1% → defer, >5% → prioritize. An offset-mapping approach was rejected; it depends ontokenize(decode(ids)) == idsholding for the vLLM detokenizer. -
Parallel-list bookkeeping: 5 aligned lists across 5 files, in-place surgery in
_reject_group_records, and thezip(... or [0] * len(...))workaround in_apply_data_actions_fn. Two refactors collapse it: aValidationErrorNamedTuple for the(message, validator)pair, andlist[ClassifiedRecord]with aRecordStatusenum so text, count, error, and status live on the same object. The_reject_group_recordstext/count mismatch flagged inline becomes unrepresentable under the second. -
Aggregation:
TokenStatswith__add__collapses the 10 sum-properties acrossBatchandGenerationBatchesinto one aggregable dataclass.
|
Consolidating with @kendrickb-nvidia's review so @seayang-nv doesn't have to triangulate. we roughly agree on the Direct overlap across reviews
Points kendrick raised that i did not and that I endorse
Stale commentsThe 5 inline comments on
Overall -
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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>
146d1ca to
832f666
Compare
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>
There was a problem hiding this comment.
Thanks for taking the ParsedRecord refactor all the way through, Sean. The new metrics surfaced through the summary / W&B / batch progress will be really helpful for tuning runs.
Ran an astnav sweep and left four suggestions inline — all optional for this PR, none of them change behavior. happy to file follow-up issues for any we don't land here.
One pre-existing observation (not inline because it's outside the diff): over in make_nss_results at src/nemo_safe_synthesizer/results.py:156, make_nss_summary(...) gets called before the None / empty-DataFrame guards on lines 157-160 run, so a None input would AttributeError inside make_nss_summary before the ValueError fires. One-line reorder to move the two guards above the call. Totally fine to skip or handle separately since this PR didn't touch it.
<!-- SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> <!-- SPDX-License-Identifier: Apache-2.0 --> <!-- Thank you for contributing to Safe Synthesizer! --> # 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 <!-- These checks should be completed before a PR is reviewed, --> <!-- but you can submit a draft early to indicate that the issue is being worked on. --> Ensure that the following pass: - [x] `make format && make check` or via prek validation. - [x] `make test` passes locally - [x] `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 <!-- These checks need to be completed before a PR is merged, --> <!-- but as PRs often change significantly during review, --> <!-- it's OK for them to be incomplete when review is first requested. --> - [x] New or updated tests for any fix or new behavior - [x] Updated documentation for new features and behaviors, including docstrings for API docs. ## Other Notes <!-- Please add the issue number that should be closed when this PR is merged. --> - Closes #426 --------- Signed-off-by: Sean Yang <seayang@nvidia.com> Signed-off-by: seayang <seayang@nvidia.com> Signed-off-by: seayang-nv <seayang@nvidia.com> Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Summary
Tracks token usage throughout the generation pipeline so operators can see how efficiently the LLM's output budget is being spent -- how many tokens land in valid records vs. invalid or non-record overhead.
ParsedRecord(text, parsed dict, error, token count on one object) and reshapesParsedResponseto holdlist[ParsedRecord], replacing the prior parallel lists (valid_records/invalid_records/errors/*_token_counts) that previously threaded through extraction, validation, group-level rejection, batch aggregation, and data-action post-processing. ← newParsedRecordthrough validation, group-level rejection, and time-series cascade invalidation.GenerateJobResults, and the finalSafeSynthesizerSummary/ W&B: completion token totals, valid/invalid/non-record breakdowns, tokens-per-second throughput (with and without startup), valid-tokens-per-second, and tokenization overhead.InferenceEvalCallback) now passes the tokenizer through and forwards per-row completion-token counts so the same metrics apply during training-time evaluation._sec,_seconds) are displayed as plain floats instead of percentages.Example summary for
patient_events.csvPre-Review Checklist
Ensure that the following pass:
make format && make checkor via prek validation.make testpasses locallymake test-e2epasses locallymake test-ci-containerpasses locally (recommended)/syncon this PR to trigger a run (auto-triggers on ready-for-review)Pre-Merge Checklist
Other Notes