feat: Use Partial Prefix to Prompt Time Series Generation - #708
feat: Use Partial Prefix to Prompt Time Series Generation#708seayang-nv wants to merge 6 commits into
Conversation
github-actions
Bot
added
area:data-processing
area:generation
area:training
area:tests
area:docs
labels
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTime-series preprocessing now orders identity columns first and rejects invalid identity-only inputs. Metadata stores typed group registries and source-column order. Prompt construction uses partial records and rolling history. Generation uses per-group state, context clamping, and retries. ChangesData contracts, metadata, and validation
Training-compatible prompt construction
Generation runtime
Documentation alignment
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to This change improves time-series generation but currently exposes raw group identifiers in warning logs and understates the need to retrain older artifacts, which can create privacy concerns and failed generation attempts for users. These bounded issues should be addressed before merge. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR replaces training-record prefills with typed group registries and training-compatible partial-record prompts, while preserving exact accepted records as rolling history.
Confidence Score: 4/5The PR is not yet safe to merge because one near-limit rolling prompt can still starve shorter groups or abort their shared generation run. Current code computes one completion limit from the longest active prompt and applies it to every group, leaving the previously reported heterogeneous-prompt generation failure outstanding. Files Needing Attention: src/nemo_safe_synthesizer/generation/timeseries_backend.py Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Signed-off-by: seayang <seayang@nvidia.com>
Signed-off-by: seayang <seayang@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bc9d6f79-fd72-4ed0-8247-b029cc6dbfe3
📒 Files selected for processing (16)
docs/developer-guide/example-generation.mddocs/tutorials/time-series-financial-transactions.ipynbdocs/user-guide/configuration.mdsrc/nemo_safe_synthesizer/TIMESERIES_README.mdsrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/preflight/checks/dataframe.pysrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pytests/data_processing/test_assembler.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_backend.pytests/generation/test_timeseries_prompting.pytests/preflight/test_preflight.pytests/smoke/test_nss_timeseries_gpu.pytests/training/test_timeseries_preprocessing.py
💤 Files with no reviewable changes (1)
- docs/user-guide/configuration.md
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.14)
- GitHub Check: Smoke Tests
- GitHub Check: End-user Wheel Install
- GitHub Check: conventional-commit / semantic-pull-request
- GitHub Check: Analyze (Python)
- GitHub Check: Greptile Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (19)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pysrc/nemo_safe_synthesizer/preflight/checks/dataframe.pydocs/developer-guide/example-generation.mdsrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/TIMESERIES_README.mdtests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounit
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pytests/preflight/test_preflight.pytests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Useuvfor Python dependency management and execution; never usepipor rawpython. Target Python 3.11–3.14 and use modern syntax such asX | Y,list[str], andSelf.
Run project tooling throughmisetasks or wrapper scripts intools/; do not invokeruffortydirectly. Useuv runfor Python execution.
Put durable implementation guidance in public function and class docstrings; put local invariants in source comments.
Use Python 3.11–3.14-compatible modern type syntax, includingX | Y,list[str], andSelf.
**/*.py: Use American English spelling in Python code, comments, and documentation; usefrom __future__ import annotationsin every module.
UseBaseSettingsfor environment/CLI settings; preferAliasChoicesfor fields accepting both Python and environment-variable names.
Pydantic model fields must includeField(description=...); prefer assignment-styleField()and useAnnotatedonly for additional metadata or constraints.
Use@dataclass(frozen=True)for immutable value objects and validators, andfield(default_factory=...)for mutable defaults; never use mutable default values directly.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal-only constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Do not useprint()for operational library output; use the approved logger,click.echo()for CLI output, orsys.stdout.write()for raw tool output.
Use loggerextra={}for metrics, counts, durations, and other data intended for machine querying; use f-strings for human-readable context.
Raise known errors through the Safe Synthesizer custom hierarchy, using dual inheritance where callers should also catch a built-in exception.
UseX | Y, built-in generic types,Selffor fluent returns, collection ABCs for arguments,Protocolfor structural boundaries, and avoidAny.
Pre...
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pysrc/nemo_safe_synthesizer/preflight/checks/dataframe.pysrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pytests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
**/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitpytest marker instead of the deprecatedunit_testmarker; async tests do not need@pytest.mark.asynciobecauseasyncio_mode = auto.
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pytests/preflight/test_preflight.pytests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: ReadAGENTS.local.mdif it exists and give its instructions top priority.
When a task matches a repository-specific skill, read the corresponding skill under.agents/skills/instead of duplicating its workflow instructions.
Do not commit unless the user explicitly asks for a commit or PR work.
When committing, require DCO sign-off and GPG signing usinggit commit --signoff --gpg-sign(or-s -S); never manually addSigned-off-byor use--no-gpg-sign.
Use feature branches based onmain; branch names commonly include an issue-number prefix such as<author>/123-short-name.
For recurring testing, building, syncing, bootstrapping, worktree, and GitHub workflows, use the matching skill under.agents/skills/.
For a full GPU/development environment, useuv sync --frozen --extra cu129 --extra engine --group dev; bareuv sync --frozenis incomplete and can causety, import-check, and GPU-test failures.End files with a newline, contain no trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pysrc/nemo_safe_synthesizer/preflight/checks/dataframe.pydocs/developer-guide/example-generation.mdsrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/TIMESERIES_README.mddocs/tutorials/time-series-financial-transactions.ipynbtests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pysrc/nemo_safe_synthesizer/preflight/checks/dataframe.pydocs/developer-guide/example-generation.mdsrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/TIMESERIES_README.mddocs/tutorials/time-series-financial-transactions.ipynbtests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*.{md,py,sh,toml,yml,yaml,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Follow the detailed language and file-format conventions defined in
STYLE_GUIDE.md.
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pysrc/nemo_safe_synthesizer/preflight/checks/dataframe.pydocs/developer-guide/example-generation.mdsrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/TIMESERIES_README.mdtests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
tests/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
tests/**/*.py: Use absolute imports in tests.
Name test filestest_*.py, classesTest*, and functionstest_<module>_<expected_behavior>; prefix fixtures withfixture_and give each fixture a one-line purpose docstring.
Use function-scoped fixtures by default, bareassert,pytest.raises(match=...),pytest.approx(), andpytest.mark.parametrizefor input combinations.
Usetmp_pathfor file operations, mock only external boundaries, avoid shared mutable state and order dependencies, and mark CUDA-dependent tests appropriately.All existing tests must pass before submitting a pull request; new features must include tests and bug fixes must include regression tests.
tests/**/*.py: Every test must have exactly one category marker:unit,smoke, ore2e; use modifier markers such asslowandrequires_gpuonly in addition to a category marker.
Usepytest.importorskipfor optional dependencies that require specific extras, such assentence_transformersandvllm.
Usefixture_mock_processororfixture_mock_processor_without_valid_recordsforParsedResponsetest data, withvalid_records,invalid_records,errors, and integerprompt_numberfields.
Use the sharedload_test_dataset(filename)andload_test_dataframe(filename)helpers for loading test datasets where applicable.
Tests should mirror the source structure, such astests/training/,tests/generation/, and corresponding source modules.
print()is permitted in tests for debug output; Ruff ruleT201is suppressed for thetests/directory.
Do not import helpers directly from another file undertests/; for shared methods, use a relative import fromconftest.py(pytest fixtures are available automatically).
When adding a vLLM GPU smoke-test file, addpytest.mark.vllm, create a dedicatedtest:smoke:gpu:*mise task for per-file process isolation, and include that task intest:smoke:gpu.
GPU tests must use therequires_gpumarker; vLLM tests additionally r...
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pytests/preflight/test_preflight.pytests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.py
⚙️ CodeRabbit configuration file
Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pytests/preflight/test_preflight.pytests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include the required SPDX copyright and Apache-2.0 license headers, using comment syntax appropriate to the file format.
Include SPDX copyright headers in all source files, except files explicitly listed in
.copyrightignore.
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pysrc/nemo_safe_synthesizer/preflight/checks/dataframe.pydocs/developer-guide/example-generation.mdsrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/TIMESERIES_README.mdtests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{py,pyi}: Keep shared Python package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
Use the repository's pinned Ruff tasks for Python formatting, import sorting, and linting rather than invoking unpinned tools directly.
Run the repository's pinnedtytype checker and maintain type-correct Python code.
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pysrc/nemo_safe_synthesizer/preflight/checks/dataframe.pysrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pytests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*.{py,sh,yaml,yml,toml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use
misetasks with the repository's pinned tool versions for formatting, checking, and testing before submitting changes.
Files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pysrc/nemo_safe_synthesizer/preflight/checks/dataframe.pydocs/developer-guide/example-generation.mdsrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pytests/preflight/test_preflight.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/TIMESERIES_README.mdtests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
tests/smoke/**/*.py
📄 CodeRabbit inference engine (tests/TESTING.md)
Mark smoke tests with the
smokepytest marker.
Files:
tests/smoke/test_nss_timeseries_gpu.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
UseTYPE_CHECKINGguards for heavy imports such aspandas,torch, andtransformers; use relative imports insrc/.
Do not add__init__.pyto test directories; every directory undersrc/containing Python files must have one.
Comments should explain why rather than narrate what; do not add redundant docstrings, defensiveexcept Exceptionaround trusted internal calls, unjustified type ignores, or casts/Anyto hide type errors.
Usepathlib.Pathinstead ofos.path, and use explicitif/raisevalidation instead ofassertin library code.
Files:
src/nemo_safe_synthesizer/preflight/checks/dataframe.pysrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
⚙️ CodeRabbit configuration file
Review library code against STYLE_GUIDE.md. Focus on behavior, API contracts, error handling, resource cleanup, typing, logging, and user-facing failures. Public APIs and nontrivial functions need Google-style docstrings.
Files:
src/nemo_safe_synthesizer/preflight/checks/dataframe.pysrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*.{md,markdown}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown}: Bold is acceptable only in markdown tables where it's the conventional way to mark header-like cells in the body
Use##headers to segment markdown sections instead of bold text
Use--(em-dash) instead of-(hyphen) for asides in markdown
Files:
docs/developer-guide/example-generation.mdsrc/nemo_safe_synthesizer/TIMESERIES_README.md
docs/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/writing-docs.mdc)
docs/**/*.md: Use MkDocs Material admonition syntax (!!! note, !!! warning, ??? tip) for highlighting important information and collapsible sections in documentation
Use MkDocs Material tabs syntax (=== "Label") to present alternative views or language-specific examples in documentation
Use code block syntax with title and highlight line parameters (title="filename", hl_lines="2 3") for code examples in documentation
Use Mermaid diagram syntax (```mermaid flowchart, etc.) for visualizations in documentationClassify documentation using Diataxis and use MkDocs Material syntax for admonitions, tabs, titled code blocks, and highlights.
docs/**/*.md: Place documentation pages under the appropriate Diataxis directory (getting-started,user-guide,architecture,reference, ordev-notes) and add new pages to thenavsection ofmkdocs.yml.
Write Google-style docstrings insrc/nemo_safe_synthesizer/for API reference content; generated reference pages must not be edited manually.
Files:
docs/developer-guide/example-generation.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Do not use decorative bold text in body content; use single backticks for inline code and
--for asides. In Python docstrings, use double backticks and MkDocs autorefs rather than Sphinx roles.
Files:
docs/developer-guide/example-generation.mdsrc/nemo_safe_synthesizer/TIMESERIES_README.md
docs/**
⚙️ CodeRabbit configuration file
Review documentation as MkDocs Material content. Check Diataxis fit, accurate commands, internal links, code fences, and markdown style from STYLE_GUIDE.md.
Files:
docs/developer-guide/example-generation.mddocs/tutorials/time-series-financial-transactions.ipynb
src/nemo_safe_synthesizer/training/**/*.py
⚙️ CodeRabbit configuration file
Review training changes for dataset preprocessing, model path handling, artifact writes, LoRA/DP behavior, GPU memory usage, reproducibility, and cleanup on failure.
Files:
src/nemo_safe_synthesizer/training/timeseries_preprocessing.py
src/nemo_safe_synthesizer/data_processing/**/*.py
⚙️ CodeRabbit configuration file
Review for data-contract regressions. Check input/training/test/synthetic naming, group boundaries, token-budget math, record ordering, schema and column validation, nullable dtypes, and deterministic behavior.
Files:
src/nemo_safe_synthesizer/data_processing/timeseries_validation.py
src/nemo_safe_synthesizer/generation/**/*.py
⚙️ CodeRabbit configuration file
Review generation changes for retry loops, stopping conditions, invalid record handling, regex/structured output contracts, backend teardown, memory cleanup, and vLLM assumptions.
Files:
src/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
🧠 Learnings (6)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pytests/preflight/test_preflight.pytests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.py
📚 Learning: 2026-07-27T22:07:22.590Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 673
File: tests/pii_replacer/test_edit.py:327-327
Timestamp: 2026-07-27T22:07:22.590Z
Learning: When tests read structured logging context from Python `logging.LogRecord` instances, don’t access `record.ctx` directly (it isn’t declared on `LogRecord` and will break static typing). Instead, use `getattr(record, "ctx", default)` (or an appropriate fallback) to safely handle cases where `ctx` may or may not be attached. This applies even if Ruff rule `B009` isn’t enabled in the repo.
Applied to files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pytests/preflight/test_preflight.pytests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.py
📚 Learning: 2026-07-29T17:12:32.642Z
Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 654
File: tests/config/test_parameters.py:116-118
Timestamp: 2026-07-29T17:12:32.642Z
Learning: In Pydantic validation tests (e.g., models configured with `from_attributes`), when asserting failures from `model_validate(...)`, assert the structured error details (such as `ValidationError.errors()[0]["type"]`, e.g. `"model_attributes_type"`) rather than relying on the human-readable error message text. This keeps tests stable even if wording changes, while still verifying the correct validation rule is triggered.
Applied to files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pytests/preflight/test_preflight.pytests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.py
📚 Learning: 2026-08-05T19:07:15.856Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 679
File: tests/conftest.py:89-96
Timestamp: 2026-08-05T19:07:15.856Z
Learning: In the Safe Synthesizer test suite, treat CPU and CUDA installation profiles as the supported pytest collection profiles because they install PyTorch. A bare installation without PyTorch is an incomplete, unsupported profile. GPU-marked test modules may import PyTorch before tests/conftest.py::pytest_collection_modifyitems executes, so do not require that collection hook to prevent such imports.
Applied to files:
tests/smoke/test_nss_timeseries_gpu.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_prompting.pytests/data_processing/test_assembler.pytests/preflight/test_preflight.pytests/training/test_timeseries_preprocessing.pytests/generation/test_timeseries_backend.py
📚 Learning: 2026-06-04T16:14:09.868Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 526
File: tests/generation/test_vllm_backend.py:399-509
Timestamp: 2026-06-04T16:14:09.868Z
Learning: In this repo, only apply `pytest.mark.vllm` to smoke tests under `tests/smoke/` that actually run real vLLM GPU generation and therefore require per-file process isolation (e.g., `test-smoke-gpu-*` Makefile targets). Do not apply `pytest.mark.vllm` to unit-style tests under `tests/generation/` that merely import `vllm_backend` but never instantiate a real vLLM engine and never call `.generate()` (GPU not required). Note that `tests/conftest.py` auto-marks these as `unit` via `pytest_collection_modifyitems`, and `vllm` is not among the auto-mark categories—so if a test in `tests/generation/` has `vllm`, it should be treated as a review issue unless it meets the real GPU generation criteria above.
Applied to files:
tests/smoke/test_nss_timeseries_gpu.pytests/generation/test_timeseries_prompting.pytests/generation/test_timeseries_backend.py
📚 Learning: 2026-08-06T15:42:45.969Z
Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 703
File: src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py:356-362
Timestamp: 2026-08-06T15:42:45.969Z
Learning: In NeMo Safe Synthesizer Python code, do not flag diagnostics solely because they report raw DataFrame column names. This is an established convention used in preflight warnings and validation errors. Flag such reporting only when an explicit repository-wide logging or privacy policy requires redaction, or when the diagnostic includes cell values, records, or other sensitive data.
Applied to files:
src/nemo_safe_synthesizer/preflight/checks/dataframe.pysrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
🪛 markdownlint-cli2 (0.23.2)
src/nemo_safe_synthesizer/TIMESERIES_README.md
[warning] 266-266: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.16.1)
tests/generation/test_timeseries_prompting.py
[error] 116-116: Possible hardcoded password assigned to argument: "bos_token"
(S106)
[error] 118-118: Possible hardcoded password assigned to argument: "eos_token"
(S106)
[error] 157-157: Possible hardcoded password assigned to argument: "bos_token"
(S106)
[error] 159-159: Possible hardcoded password assigned to argument: "eos_token"
(S106)
docs/tutorials/time-series-financial-transactions.ipynb
[warning] 146-146: Found useless expression. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (7)
src/nemo_safe_synthesizer/data_processing/timeseries_validation.py (1)
474-480: LGTM!src/nemo_safe_synthesizer/preflight/checks/dataframe.py (1)
196-196: LGTM!src/nemo_safe_synthesizer/training/timeseries_preprocessing.py (1)
17-41: LGTM!Also applies to: 61-61, 111-115
tests/data_processing/test_timeseries_validation.py (1)
131-151: LGTM!tests/preflight/test_preflight.py (1)
853-865: LGTM!tests/training/test_timeseries_preprocessing.py (1)
37-37: LGTM!Also applies to: 55-64, 78-78
tests/data_processing/test_assembler.py (1)
812-815: LGTM!
1100a3b to
417ea1c
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: seayang <seayang@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 62147a43-6cd1-4a7c-a060-94a902519e29
📒 Files selected for processing (3)
src/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.pytests/generation/test_timeseries_prompting.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/nemo_safe_synthesizer/data_processing/timeseries_validation.py
- src/nemo_safe_synthesizer/generation/timeseries_backend.py
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: Unit Tests (3.14)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: End-user Wheel Install
- GitHub Check: Smoke Tests
- GitHub Check: Greptile Review
- GitHub Check: Typecheck
- GitHub Check: Analyze (Python)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
tests/generation/test_timeseries_prompting.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounit
Files:
tests/generation/test_timeseries_prompting.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Useuvfor Python dependency management and execution; never usepipor rawpython. Target Python 3.11–3.14 and use modern syntax such asX | Y,list[str], andSelf.
Run project tooling throughmisetasks or wrapper scripts intools/; do not invokeruffortydirectly. Useuv runfor Python execution.
Put durable implementation guidance in public function and class docstrings; put local invariants in source comments.
Use Python 3.11–3.14-compatible modern type syntax, includingX | Y,list[str], andSelf.
**/*.py: Use American English spelling in Python code, comments, and documentation; usefrom __future__ import annotationsin every module.
UseBaseSettingsfor environment/CLI settings; preferAliasChoicesfor fields accepting both Python and environment-variable names.
Pydantic model fields must includeField(description=...); prefer assignment-styleField()and useAnnotatedonly for additional metadata or constraints.
Use@dataclass(frozen=True)for immutable value objects and validators, andfield(default_factory=...)for mutable defaults; never use mutable default values directly.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal-only constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Do not useprint()for operational library output; use the approved logger,click.echo()for CLI output, orsys.stdout.write()for raw tool output.
Use loggerextra={}for metrics, counts, durations, and other data intended for machine querying; use f-strings for human-readable context.
Raise known errors through the Safe Synthesizer custom hierarchy, using dual inheritance where callers should also catch a built-in exception.
UseX | Y, built-in generic types,Selffor fluent returns, collection ABCs for arguments,Protocolfor structural boundaries, and avoidAny.
Pre...
Files:
tests/generation/test_timeseries_prompting.py
**/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitpytest marker instead of the deprecatedunit_testmarker; async tests do not need@pytest.mark.asynciobecauseasyncio_mode = auto.
Files:
tests/generation/test_timeseries_prompting.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: ReadAGENTS.local.mdif it exists and give its instructions top priority.
When a task matches a repository-specific skill, read the corresponding skill under.agents/skills/instead of duplicating its workflow instructions.
Do not commit unless the user explicitly asks for a commit or PR work.
When committing, require DCO sign-off and GPG signing usinggit commit --signoff --gpg-sign(or-s -S); never manually addSigned-off-byor use--no-gpg-sign.
Use feature branches based onmain; branch names commonly include an issue-number prefix such as<author>/123-short-name.
For recurring testing, building, syncing, bootstrapping, worktree, and GitHub workflows, use the matching skill under.agents/skills/.
For a full GPU/development environment, useuv sync --frozen --extra cu129 --extra engine --group dev; bareuv sync --frozenis incomplete and can causety, import-check, and GPU-test failures.End files with a newline, contain no trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
Files:
tests/generation/test_timeseries_prompting.py
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
tests/generation/test_timeseries_prompting.py
**/*.{md,py,sh,toml,yml,yaml,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Follow the detailed language and file-format conventions defined in
STYLE_GUIDE.md.
Files:
tests/generation/test_timeseries_prompting.py
tests/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
tests/**/*.py: Use absolute imports in tests.
Name test filestest_*.py, classesTest*, and functionstest_<module>_<expected_behavior>; prefix fixtures withfixture_and give each fixture a one-line purpose docstring.
Use function-scoped fixtures by default, bareassert,pytest.raises(match=...),pytest.approx(), andpytest.mark.parametrizefor input combinations.
Usetmp_pathfor file operations, mock only external boundaries, avoid shared mutable state and order dependencies, and mark CUDA-dependent tests appropriately.All existing tests must pass before submitting a pull request; new features must include tests and bug fixes must include regression tests.
tests/**/*.py: Every test must have exactly one category marker:unit,smoke, ore2e; useslowandrequires_gpuas modifiers where applicable.
Usepytest.importorskipfor optional dependencies that require specific extras, such assentence_transformersandvllm.
Use the sharedfixture_mock_processororfixture_mock_processor_without_valid_recordsfixtures for processor mocks, withParsedResponsefieldsvalid_records,invalid_records,errors, andprompt_number.
Useload_test_dataset(filename)andload_test_dataframe(filename)for loading shared test datasets.
Keep tokenizers function-scoped unless there is a specific reason to change scope;fixture_session_cache_diris session-scoped.
Tests should mirror the source structure, such astests/training/,tests/generation/, and related module directories.
print()is permitted in tests because Ruff ruleT201is suppressed for thetests/directory.
Do not import directly from another ordinary file undertests/; share methods through relative imports fromconftest.pywhen needed.
When debugging NSS logs with pytest, use-sor--capture=no; use-n0for visible output when xdist is involved.
Files:
tests/generation/test_timeseries_prompting.py
⚙️ CodeRabbit configuration file
Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.
Files:
tests/generation/test_timeseries_prompting.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include the required SPDX copyright and Apache-2.0 license headers, using comment syntax appropriate to the file format.
Include SPDX copyright headers in all source files, except files explicitly listed in
.copyrightignore.
Files:
tests/generation/test_timeseries_prompting.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{py,pyi}: Keep shared Python package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
Use the repository's pinned Ruff tasks for Python formatting, import sorting, and linting rather than invoking unpinned tools directly.
Run the repository's pinnedtytype checker and maintain type-correct Python code.
Files:
tests/generation/test_timeseries_prompting.py
**/*.{py,sh,yaml,yml,toml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use
misetasks with the repository's pinned tool versions for formatting, checking, and testing before submitting changes.
Files:
tests/generation/test_timeseries_prompting.py
🧠 Learnings (5)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/generation/test_timeseries_prompting.py
📚 Learning: 2026-07-27T22:07:22.590Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 673
File: tests/pii_replacer/test_edit.py:327-327
Timestamp: 2026-07-27T22:07:22.590Z
Learning: When tests read structured logging context from Python `logging.LogRecord` instances, don’t access `record.ctx` directly (it isn’t declared on `LogRecord` and will break static typing). Instead, use `getattr(record, "ctx", default)` (or an appropriate fallback) to safely handle cases where `ctx` may or may not be attached. This applies even if Ruff rule `B009` isn’t enabled in the repo.
Applied to files:
tests/generation/test_timeseries_prompting.py
📚 Learning: 2026-07-29T17:12:32.642Z
Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 654
File: tests/config/test_parameters.py:116-118
Timestamp: 2026-07-29T17:12:32.642Z
Learning: In Pydantic validation tests (e.g., models configured with `from_attributes`), when asserting failures from `model_validate(...)`, assert the structured error details (such as `ValidationError.errors()[0]["type"]`, e.g. `"model_attributes_type"`) rather than relying on the human-readable error message text. This keeps tests stable even if wording changes, while still verifying the correct validation rule is triggered.
Applied to files:
tests/generation/test_timeseries_prompting.py
📚 Learning: 2026-08-05T19:07:15.856Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 679
File: tests/conftest.py:89-96
Timestamp: 2026-08-05T19:07:15.856Z
Learning: In the Safe Synthesizer test suite, treat CPU and CUDA installation profiles as the supported pytest collection profiles because they install PyTorch. A bare installation without PyTorch is an incomplete, unsupported profile. GPU-marked test modules may import PyTorch before tests/conftest.py::pytest_collection_modifyitems executes, so do not require that collection hook to prevent such imports.
Applied to files:
tests/generation/test_timeseries_prompting.py
📚 Learning: 2026-06-04T16:14:09.868Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 526
File: tests/generation/test_vllm_backend.py:399-509
Timestamp: 2026-06-04T16:14:09.868Z
Learning: In this repo, only apply `pytest.mark.vllm` to smoke tests under `tests/smoke/` that actually run real vLLM GPU generation and therefore require per-file process isolation (e.g., `test-smoke-gpu-*` Makefile targets). Do not apply `pytest.mark.vllm` to unit-style tests under `tests/generation/` that merely import `vllm_backend` but never instantiate a real vLLM engine and never call `.generate()` (GPU not required). Note that `tests/conftest.py` auto-marks these as `unit` via `pytest_collection_modifyitems`, and `vllm` is not among the auto-mark categories—so if a test in `tests/generation/` has `vllm`, it should be treated as a review issue unless it meets the real GPU generation criteria above.
Applied to files:
tests/generation/test_timeseries_prompting.py
🪛 Ruff (0.16.1)
tests/generation/test_timeseries_prompting.py
[error] 118-118: Possible hardcoded password assigned to argument: "bos_token"
(S106)
[error] 120-120: Possible hardcoded password assigned to argument: "eos_token"
(S106)
[error] 159-159: Possible hardcoded password assigned to argument: "bos_token"
(S106)
[error] 161-161: Possible hardcoded password assigned to argument: "eos_token"
(S106)
🔇 Additional comments (8)
tests/generation/test_timeseries_prompting.py (8)
1-18: LGTM!
20-38: LGTM!
41-58: LGTM!
61-74: LGTM!
76-89: LGTM!
91-99: LGTM!
100-141: LGTM!
145-165: LGTM!
Signed-off-by: seayang <seayang@nvidia.com>
Signed-off-by: seayang <seayang@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nemo_safe_synthesizer/generation/timeseries_backend.py (1)
923-923: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a bounded termination rule for interval-less time series. Preflight permits
timestamp_interval_seconds=Nonewhen every group has one record, so_process_group_resultskips chronological validation. Valid records below_stop_timestamp_valuethen reset the retry counter and keepactive_statesnon-empty indefinitely. Reject this configuration or add a maximum-attempt bound, and remove the obsolete global stop conditions from the class docstring.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f2230332-a204-455a-9bef-0f13ae7dd18b
📒 Files selected for processing (1)
src/nemo_safe_synthesizer/generation/timeseries_backend.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Greptile Review
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Smoke Tests
- GitHub Check: End-user Wheel Install
- GitHub Check: Unit Tests (3.14)
- GitHub Check: Typecheck
- GitHub Check: Analyze (Python)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Useuvfor Python dependency management and execution; never usepipor rawpython. Target Python 3.11–3.14 and use modern syntax such asX | Y,list[str], andSelf.
Run project tooling throughmisetasks or wrapper scripts intools/; do not invokeruffortydirectly. Useuv runfor Python execution.
Put durable implementation guidance in public function and class docstrings; put local invariants in source comments.
Use Python 3.11–3.14-compatible modern type syntax, includingX | Y,list[str], andSelf.
**/*.py: Use American English spelling in Python code, comments, and documentation; usefrom __future__ import annotationsin every module.
UseBaseSettingsfor environment/CLI settings; preferAliasChoicesfor fields accepting both Python and environment-variable names.
Pydantic model fields must includeField(description=...); prefer assignment-styleField()and useAnnotatedonly for additional metadata or constraints.
Use@dataclass(frozen=True)for immutable value objects and validators, andfield(default_factory=...)for mutable defaults; never use mutable default values directly.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal-only constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Do not useprint()for operational library output; use the approved logger,click.echo()for CLI output, orsys.stdout.write()for raw tool output.
Use loggerextra={}for metrics, counts, durations, and other data intended for machine querying; use f-strings for human-readable context.
Raise known errors through the Safe Synthesizer custom hierarchy, using dual inheritance where callers should also catch a built-in exception.
UseX | Y, built-in generic types,Selffor fluent returns, collection ABCs for arguments,Protocolfor structural boundaries, and avoidAny.
Pre...
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: ReadAGENTS.local.mdif it exists and give its instructions top priority.
When a task matches a repository-specific skill, read the corresponding skill under.agents/skills/instead of duplicating its workflow instructions.
Do not commit unless the user explicitly asks for a commit or PR work.
When committing, require DCO sign-off and GPG signing usinggit commit --signoff --gpg-sign(or-s -S); never manually addSigned-off-byor use--no-gpg-sign.
Use feature branches based onmain; branch names commonly include an issue-number prefix such as<author>/123-short-name.
For recurring testing, building, syncing, bootstrapping, worktree, and GitHub workflows, use the matching skill under.agents/skills/.
For a full GPU/development environment, useuv sync --frozen --extra cu129 --extra engine --group dev; bareuv sync --frozenis incomplete and can causety, import-check, and GPU-test failures.End files with a newline, contain no trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*.{md,py,sh,toml,yml,yaml,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Follow the detailed language and file-format conventions defined in
STYLE_GUIDE.md.
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
UseTYPE_CHECKINGguards for heavy imports such aspandas,torch, andtransformers; use relative imports insrc/.
Do not add__init__.pyto test directories; every directory undersrc/containing Python files must have one.
Comments should explain why rather than narrate what; do not add redundant docstrings, defensiveexcept Exceptionaround trusted internal calls, unjustified type ignores, or casts/Anyto hide type errors.
Usepathlib.Pathinstead ofos.path, and use explicitif/raisevalidation instead ofassertin library code.
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
⚙️ CodeRabbit configuration file
Review library code against STYLE_GUIDE.md. Focus on behavior, API contracts, error handling, resource cleanup, typing, logging, and user-facing failures. Public APIs and nontrivial functions need Google-style docstrings.
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include the required SPDX copyright and Apache-2.0 license headers, using comment syntax appropriate to the file format.
Include SPDX copyright headers in all source files, except files explicitly listed in
.copyrightignore.
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{py,pyi}: Keep shared Python package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
Use the repository's pinned Ruff tasks for Python formatting, import sorting, and linting rather than invoking unpinned tools directly.
Run the repository's pinnedtytype checker and maintain type-correct Python code.
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*.{py,sh,yaml,yml,toml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use
misetasks with the repository's pinned tool versions for formatting, checking, and testing before submitting changes.
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
src/nemo_safe_synthesizer/generation/**/*.py
⚙️ CodeRabbit configuration file
Review generation changes for retry loops, stopping conditions, invalid record handling, regex/structured output contracts, backend teardown, memory cleanup, and vLLM assumptions.
Files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
🧠 Learnings (1)
📚 Learning: 2026-08-06T15:42:45.969Z
Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 703
File: src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py:356-362
Timestamp: 2026-08-06T15:42:45.969Z
Learning: In NeMo Safe Synthesizer Python code, do not flag diagnostics solely because they report raw DataFrame column names. This is an established convention used in preflight warnings and validation errors. Flag such reporting only when an explicit repository-wide logging or privacy policy requires redaction, or when the diagnostic includes cell values, records, or other sensitive data.
Applied to files:
src/nemo_safe_synthesizer/generation/timeseries_backend.py
🔇 Additional comments (2)
src/nemo_safe_synthesizer/generation/timeseries_backend.py (2)
18-18: LGTM!Also applies to: 30-37, 75-75, 95-97, 146-146, 159-159, 210-210, 223-227, 237-248, 257-300, 392-398, 442-444, 453-455, 560-563, 572-577, 617-657, 835-853, 871-872, 890-900
1007-1007: 🩺 Stability & AvailabilityNo constructor error occurs.
GenerationBatches.__init__defaults both parameters toNone. The reduced call is valid and still stops on zero-valid batches viaSTOP_NO_RECORDS.> Likely an incorrect or invalid review comment.
| @@ -262,9 +262,9 @@ these conditions are met: | |||
| ### Initial prefill | |||
|
|
|||
| For each group, the first 3 training records are stored as `initial_prefill` -- | |||
There was a problem hiding this comment.
question: Do we still need to store the first 3 training records? Would 1 be sufficient now?
Actually, this is getting confusing now. initial_prefill as a dictionary with 3 records may be right, but GroupState.initial_prefill is the prefix only version with partial of the 1st record.
Overall, naming of these different things is getting a bit hard to follow. I thought I had the prefill = whole records, prefix = initial partial record making sense to me, but then that's not always the case as GroupState.current_prefill may contain either depending on where during generation we are.
There was a problem hiding this comment.
Addressed. I removed the old training-record prefill mechanism and initial_prefill metadata entirely. The terminology is now:
- prefix: the incomplete initial JSON record
- history: accepted generated records used by later prompts
- RecordPromptState: the state containing the prefix/history and controlling the transition between them
The artifact now stores only typed group values, not copies of training records.
| exact accepted record text is appended to the group's rolling context. | ||
| 4. The three most recent accepted records become the prefill for the next | ||
| prompt. | ||
| 5. This repeats until the target time range is covered or retries are |
There was a problem hiding this comment.
nit: This is a bit hard to read and follow. Please make an editing pass.
Possible things to clarify:
- main loop of inference (using up to 3 most recently generated records) versus the first prompt, maybe explain the main loop and then have the initializaiton be a special case noted afterwards
- initial prompt from incomplete JSON record may generate 1 or more records, it doesn't just complete the fields of 1 record
- "constructed prefix" is confusing
There was a problem hiding this comment.
Addressed. The sequential-generation documentation now explains the normal sliding-history loop first, followed by the initial prefix iteration as a special case. It also clarifies that the prefix prompt may generate one or more records and removes the ambiguous “constructed prefix” wording.
| last_timestamp_seconds: int | None = None | ||
| """Timestamp (in seconds) of the most recently generated record, used for chronological validation.""" | ||
|
|
||
| processor_prefix: str = "" |
There was a problem hiding this comment.
question: This is a substring of initial_prefill, right? What's the benefit of storing this separately, versus calculating it from initial_prefill, or splitting on the combination of prompt and completion?
There was a problem hiding this comment.
Addressed by removing processor_prefix and the old initial_prefill state. RecordPromptState now owns the initial prefix directly and exposes it as the completion prefix only while generation is in prefix mode.
| to seed generation for each group. | ||
| _group_prefills (dict[str, str]): Saved mapping of group IDs to training | ||
| sample text. The keys define generation groups; values remain | ||
| available internally but are not inserted into prompts. |
There was a problem hiding this comment.
question: Do we use the values somewhere?
There was a problem hiding this comment.
They were not used. I wanted to keep them in case we could use them for a different feature etc. I figured it's rather easy to do it if we do need it, so I removed them. Metadata now stores timeseries_group_values, an ordered list of typed group identifiers used to initialize one generation stream per training group. No training-record text is retained for prompting.
| """Return the declared JSON types for a schema column. | ||
|
|
||
| Prefix seed values do not always retain their training-data types: group | ||
| IDs become strings when stored as dictionary keys, and configured |
There was a problem hiding this comment.
question: Why do we lose type information? Python dictionaries can have integer or float keys. This feels like patching over an upstream issue where we control all the upstream code, rather than fixing it at the source. Is it not feasible to get the properly runtime typed values passed to build_partial_record_prefix and avoid this machinery inspecting json schema objects?
Side note, #699 would help in giving us a nicely typed pydantic model or something to work with here instead of all the extra logic to pull info out of a json schema. But a fix for the future.
There was a problem hiding this comment.
Addressed at the source. The artifact now stores typed group values in timeseries_group_values, preserving strings, integers, floats, and booleans through metadata serialization. Schema inspection remains only where needed to serialize configured timestamp values consistently with the saved training schema and validate prefix field order.
| A view with group and timestamp columns first, followed by all remaining | ||
| columns in their original relative order. | ||
| """ | ||
| leading_columns = list(dict.fromkeys((group_by_column, timestamp_column))) |
There was a problem hiding this comment.
| leading_columns = list(dict.fromkeys((group_by_column, timestamp_column))) | |
| leading_columns = [group_by_column, timestamp_column] |
There was a problem hiding this comment.
? leading_columns is just a regular list[str], right, can create it directly instead of going through dict.fromkeys?
There was a problem hiding this comment.
Implemented as suggested: leading_columns = [group_by_column, timestamp_column].
| training_df, | ||
| validation.group_by_column, | ||
| validation.timestamp_column, | ||
| ) |
There was a problem hiding this comment.
question: This means the resulting synthetic data will have the column reordered too, right? Not a big deal, but we might consider putting the order back to match training data before returning to the user.
There was a problem hiding this comment.
Implemented. Training saves the original source-column order in timeseries_source_columns, and generation restores that order before returning the synthetic DataFrame while retaining any unexpected extra columns at the end.
| serialized = records_to_jsonl([ordered_values]).rstrip("\n") | ||
| if not serialized.endswith("}"): | ||
| raise GenerationError("Could not serialize the initial time-series partial record.") | ||
| return f'{serialized[:-1]},"' |
There was a problem hiding this comment.
comment: This all feels very brittle and potentially model specific as we're making assumptions about how the tokenizer works. Don't have any better ideas right now, but something to keep in mind, and maybe helped by some of the tokenization work @binaryaaron has been looking at.
There was a problem hiding this comment.
Agreed. The explicit token construction is still necessary because model families differ in whether their tokenizers inject the required BOS/EOS markers during inference. To reduce the risk, the boundary logic is now shared with training rather than duplicated, and regression tests verify that both prefix and history generation IDs match the corresponding training-example IDs.
| """Partial first-record prefix used to seed generation.""" | ||
|
|
||
| current_prefill: str | ||
| """Current prefill string, updated as generation progresses to include recently generated records.""" |
There was a problem hiding this comment.
nit: Clarify this starts with the same value as initial_prefill? Do we need to keep initial_prefill as a separate member variable at all? Part of several comments about the differences between initial_prefill, current_prefill, and processor_prefix and understanding what logic applies depending on whether the group has successfully generated a record or not.
There was a problem hiding this comment.
It has been removed. RecordPromptState.prefix stores only the incomplete initial record, while RecordPromptState.history stores accepted generated records. The explicit using_prefix flag controls which one is active, so there is no duplicate initial_prefill/current_prefill state.
Signed-off-by: seayang <seayang@nvidia.com>
| modified_params, effective_samples_per_prompt = self._build_modified_sampling_params( | ||
| sampling_params, len(active_states) | ||
| sampling_params, | ||
| len(active_states), | ||
| max_prompt_tokens=max(len(token_ids) for token_ids in prompt_token_ids), | ||
| ) |
There was a problem hiding this comment.
When active groups have different rolling-history lengths, this derives one completion budget from the longest prompt and applies it to every group. Shorter groups consequently receive truncated, invalid JSON completions and can exhaust their retries despite having available context; when the longest prompt fills the context window, the resulting GenerationError aborts the entire run before shorter groups generate.
Knowledge Base Used: Generation
There was a problem hiding this comment.
agent (
review-pr): Confirmed; this remains blocking
A single SamplingParams object is capped using the longest active prompt, then applied to every prompt in the batch. Shorter prompts lose usable completion space. The base parameters are also capped using _get_prompt_token_count(), so changing only this call may retain the shared limit. Please compute a completion budget for each prompt and pass an aligned SamplingParams sequence, or otherwise batch groups by compatible budgets.
There was a problem hiding this comment.
The fix is clear. _build_modified_sampling_params is called once with max(...) over all prompt lengths, and the returned single SamplingParams is used for every prompt. vLLM's LLM.generate accepts a list[SamplingParams] aligned to the prompt list, so you can give each prompt its own budget.
The minimal change inside _generate_parallel_groups:
| modified_params, effective_samples_per_prompt = self._build_modified_sampling_params( | |
| sampling_params, len(active_states) | |
| sampling_params, | |
| len(active_states), | |
| max_prompt_tokens=max(len(token_ids) for token_ids in prompt_token_ids), | |
| ) | |
| # Build token prompts that reproduce the training BOS/EOS boundary. | |
| prompt_token_ids = [ | |
| self._build_prompt_token_ids(state.prompt_state.prompt_segments) for state in active_states | |
| ] | |
| prompts = [TokensPrompt(prompt_token_ids=token_ids) for token_ids in prompt_token_ids] | |
| group_batches: dict[TimeSeriesGroupValue, Batch] = { | |
| state.group_id: Batch(processor=self.processor) for state in active_states | |
| } | |
| # Derive shared knobs (n, temperature, …) from the longest prompt, | |
| # then build a per-prompt SamplingParams so each group gets the full | |
| # remaining context rather than being capped to the longest prompt's budget. | |
| _, effective_samples_per_prompt = self._build_modified_sampling_params( | |
| sampling_params, | |
| len(active_states), | |
| max_prompt_tokens=max(len(t) for t in prompt_token_ids), | |
| ) | |
| per_prompt_params = [ | |
| self._build_modified_sampling_params( | |
| sampling_params, | |
| len(active_states), | |
| max_prompt_tokens=len(token_ids), | |
| )[0] | |
| for token_ids in prompt_token_ids | |
| ] |
Then pass per_prompt_params instead of modified_params to llm.generate:
outputs = self.llm.generate(
prompts=prompts,
sampling_params=per_prompt_params, # list aligned to prompts
lora_request=self.lora_req,
)One remaining caveat: _get_prompt_token_count() (used in generate() to size the base SamplingParams) still returns the longest initial prefix length. That base object is now only used to seed sampling_params before it reaches _build_modified_sampling_params, so the per-prompt clamp in _build_modified_sampling_params will always win — but it's worth a comment there so the two-level capping is obvious to future readers.
There was a problem hiding this comment.
remained unaddressed for now..
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/developer-guide/example-generation.md (1)
469-473: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument the full artifact migration requirement.
Lines 469-473 state that only artifacts with a different saved schema order require retraining. The PR objective states that previously saved time-series artifacts no longer support generation because the new generation path requires the group registry and prompt metadata. Users with older artifacts whose column order matches can follow this page and still encounter a generation failure. State that all artifacts created before this change require retraining, or document the exact metadata or version condition that remains compatible.
Proposed documentation update
-Older artifacts whose saved schema uses another order -must be retrained; generation fails explicitly instead of silently changing -prompting behavior. +Previously saved time-series artifacts must be retrained. They do not contain +the registry and prompt metadata required by the current generation path, so +generation fails explicitly instead of silently changing prompting behavior.As per path instructions: prioritize documentation issues that can break user workflows and verify documented behavior against the supplied contract.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 697dfeee-8027-4a87-8d9d-de303e58f93b
📒 Files selected for processing (19)
docs/developer-guide/example-generation.mdsrc/nemo_safe_synthesizer/TIMESERIES_README.mdsrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/preflight/checks/dataframe.pysrc/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/training/timeseries_preprocessing.pytests/data_processing/test_assembler.pytests/data_processing/test_prompt_tokens.pytests/data_processing/test_timeseries_validation.pytests/generation/test_timeseries_backend.pytests/generation/test_timeseries_prompting.pytests/llm/test_metadata.pytests/smoke/test_nss_timeseries_gpu.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/nemo_safe_synthesizer/training/timeseries_preprocessing.py
- src/nemo_safe_synthesizer/preflight/checks/dataframe.py
- tests/smoke/test_nss_timeseries_gpu.py
- src/nemo_safe_synthesizer/TIMESERIES_README.md
Included review availability: Your plan includes up to 12 reviews per rolling hour; 11 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: Greptile Review
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.14)
- GitHub Check: End-user Wheel Install
- GitHub Check: Smoke Tests
🧰 Additional context used
📓 Path-based instructions (17)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pysrc/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/data_processing/test_timeseries_validation.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/llm/metadata.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.pydocs/developer-guide/example-generation.md
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounit
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pytests/data_processing/test_timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pytests/generation/test_timeseries_backend.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Useuvfor Python dependency management and execution; never usepipor rawpython. Target Python 3.11–3.14 and use modern syntax such asX | Y,list[str], andSelf.
Run project tooling throughmisetasks or wrapper scripts intools/; do not invokeruffortydirectly. Useuv runfor Python execution.
Put durable implementation guidance in public function and class docstrings; put local invariants in source comments.
Use Python 3.11–3.14-compatible modern type syntax, includingX | Y,list[str], andSelf.
**/*.py: Use American English spelling in Python code, comments, and documentation; usefrom __future__ import annotationsin every module.
UseBaseSettingsfor environment/CLI settings; preferAliasChoicesfor fields accepting both Python and environment-variable names.
Pydantic model fields must includeField(description=...); prefer assignment-styleField()and useAnnotatedonly for additional metadata or constraints.
Use@dataclass(frozen=True)for immutable value objects and validators, andfield(default_factory=...)for mutable defaults; never use mutable default values directly.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal-only constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Do not useprint()for operational library output; use the approved logger,click.echo()for CLI output, orsys.stdout.write()for raw tool output.
Use loggerextra={}for metrics, counts, durations, and other data intended for machine querying; use f-strings for human-readable context.
Raise known errors through the Safe Synthesizer custom hierarchy, using dual inheritance where callers should also catch a built-in exception.
UseX | Y, built-in generic types,Selffor fluent returns, collection ABCs for arguments,Protocolfor structural boundaries, and avoidAny.
Pre...
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pysrc/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/data_processing/test_timeseries_validation.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/llm/metadata.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
**/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitpytest marker instead of the deprecatedunit_testmarker; async tests do not need@pytest.mark.asynciobecauseasyncio_mode = auto.
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pytests/data_processing/test_timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pytests/generation/test_timeseries_backend.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: ReadAGENTS.local.mdif it exists and give its instructions top priority.
When a task matches a repository-specific skill, read the corresponding skill under.agents/skills/instead of duplicating its workflow instructions.
Do not commit unless the user explicitly asks for a commit or PR work.
When committing, require DCO sign-off and GPG signing usinggit commit --signoff --gpg-sign(or-s -S); never manually addSigned-off-byor use--no-gpg-sign.
Use feature branches based onmain; branch names commonly include an issue-number prefix such as<author>/123-short-name.
For recurring testing, building, syncing, bootstrapping, worktree, and GitHub workflows, use the matching skill under.agents/skills/.
For a full GPU/development environment, useuv sync --frozen --extra cu129 --extra engine --group dev; bareuv sync --frozenis incomplete and can causety, import-check, and GPU-test failures.End files with a newline, contain no trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
All contributions must be signed off to certify that you have the right to submit the code.
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pysrc/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/data_processing/test_timeseries_validation.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/llm/metadata.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.pydocs/developer-guide/example-generation.md
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pysrc/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/data_processing/test_timeseries_validation.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/llm/metadata.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.pydocs/developer-guide/example-generation.md
**/*.{md,py,sh,toml,yml,yaml,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Follow the detailed language and file-format conventions defined in
STYLE_GUIDE.md.
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pysrc/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/data_processing/test_timeseries_validation.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/llm/metadata.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.pydocs/developer-guide/example-generation.md
tests/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
tests/**/*.py: Use absolute imports in tests.
Name test filestest_*.py, classesTest*, and functionstest_<module>_<expected_behavior>; prefix fixtures withfixture_and give each fixture a one-line purpose docstring.
Use function-scoped fixtures by default, bareassert,pytest.raises(match=...),pytest.approx(), andpytest.mark.parametrizefor input combinations.
Usetmp_pathfor file operations, mock only external boundaries, avoid shared mutable state and order dependencies, and mark CUDA-dependent tests appropriately.
tests/**/*.py: Every test must have exactly one category marker:unit,smoke, ore2e; useslowandrequires_gpuas modifiers where applicable.
Usepytest.importorskipfor optional dependencies that require specific extras, such assentence_transformersandvllm.
Use the sharedfixture_mock_processororfixture_mock_processor_without_valid_recordsfixtures for processor mocks, withParsedResponsefieldsvalid_records,invalid_records,errors, andprompt_number.
Useload_test_dataset(filename)andload_test_dataframe(filename)for loading shared test datasets.
Keep tokenizers function-scoped unless there is a specific reason to change scope;fixture_session_cache_diris session-scoped.
Tests should mirror the source structure, such astests/training/,tests/generation/, and related module directories.
print()is permitted in tests because Ruff ruleT201is suppressed for thetests/directory.
Do not import directly from another ordinary file undertests/; share methods through relative imports fromconftest.pywhen needed.
When debugging NSS logs with pytest, use-sor--capture=no; use-n0for visible output when xdist is involved.
tests/**/*.py: New features include tests
Bug fixes include regression tests
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pytests/data_processing/test_timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pytests/generation/test_timeseries_backend.py
⚙️ CodeRabbit configuration file
Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pytests/data_processing/test_timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pytests/generation/test_timeseries_backend.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include the required SPDX copyright and Apache-2.0 license headers, using comment syntax appropriate to the file format.
All source files (
.py,.sh,.yaml,.yml,.md) require SPDX copyright headers.
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pysrc/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/data_processing/test_timeseries_validation.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/llm/metadata.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.pydocs/developer-guide/example-generation.md
**/*.{py,pyi}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{py,pyi}: Although the default development/runtime interpreter is Python 3.13, source code must remain Python 3.11 syntax-compatible until the NMP platform moves its base Python version to 3.12.
Do not use Python 3.12-only syntax such as PEP 695typestatements or bracketed generic class/function parameters in shared package code yet.
Files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pysrc/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pytests/data_processing/test_timeseries_validation.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/llm/metadata.pytests/generation/test_timeseries_backend.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
UseTYPE_CHECKINGguards for heavy imports such aspandas,torch, andtransformers; use relative imports insrc/.
Do not add__init__.pyto test directories; every directory undersrc/containing Python files must have one.
Comments should explain why rather than narrate what; do not add redundant docstrings, defensiveexcept Exceptionaround trusted internal calls, unjustified type ignores, or casts/Anyto hide type errors.
Usepathlib.Pathinstead ofos.path, and use explicitif/raisevalidation instead ofassertin library code.just write Google-style docstrings in
src/nemo_safe_synthesizer/and they will appear on the next build.
Files:
src/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
⚙️ CodeRabbit configuration file
Review library code against STYLE_GUIDE.md. Focus on behavior, API contracts, error handling, resource cleanup, typing, logging, and user-facing failures. Public APIs and nontrivial functions need Google-style docstrings.
Files:
src/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
src/nemo_safe_synthesizer/training/**/*.py
⚙️ CodeRabbit configuration file
Review training changes for dataset preprocessing, model path handling, artifact writes, LoRA/DP behavior, GPU memory usage, reproducibility, and cleanup on failure.
Files:
src/nemo_safe_synthesizer/training/huggingface_backend.py
src/nemo_safe_synthesizer/data_processing/**/*.py
⚙️ CodeRabbit configuration file
Review for data-contract regressions. Check input/training/test/synthetic naming, group boundaries, token-budget math, record ordering, schema and column validation, nullable dtypes, and deterministic behavior.
Files:
src/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.py
src/nemo_safe_synthesizer/generation/**/*.py
⚙️ CodeRabbit configuration file
Review generation changes for retry loops, stopping conditions, invalid record handling, regex/structured output contracts, backend teardown, memory cleanup, and vLLM assumptions.
Files:
src/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
**/*.{md,markdown}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown}: Bold is acceptable only in markdown tables where it's the conventional way to mark header-like cells in the body
Use##headers to segment markdown sections instead of bold text
Use--(em-dash) instead of-(hyphen) for asides in markdown
Files:
docs/developer-guide/example-generation.md
docs/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/writing-docs.mdc)
docs/**/*.md: Use MkDocs Material admonition syntax (!!! note, !!! warning, ??? tip) for highlighting important information and collapsible sections in documentation
Use MkDocs Material tabs syntax (=== "Label") to present alternative views or language-specific examples in documentation
Use code block syntax with title and highlight line parameters (title="filename", hl_lines="2 3") for code examples in documentation
Use Mermaid diagram syntax (```mermaid flowchart, etc.) for visualizations in documentationClassify documentation using Diataxis and use MkDocs Material syntax for admonitions, tabs, titled code blocks, and highlights.
docs/**/*.md: Create or edit the.mdfile under the appropriatedocs/subdirectory.
Add the page to thenav:section ofmkdocs.ymlso it appears in the sidebar.
Files:
docs/developer-guide/example-generation.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Do not use decorative bold text in body content; use single backticks for inline code and
--for asides. In Python docstrings, use double backticks and MkDocs autorefs rather than Sphinx roles.
Files:
docs/developer-guide/example-generation.md
docs/**
⚙️ CodeRabbit configuration file
Review documentation as MkDocs Material content. Check Diataxis fit, accurate commands, internal links, code fences, and markdown style from STYLE_GUIDE.md.
Files:
docs/developer-guide/example-generation.md
🧠 Learnings (6)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pytests/data_processing/test_timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pytests/generation/test_timeseries_backend.py
📚 Learning: 2026-07-27T22:07:22.590Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 673
File: tests/pii_replacer/test_edit.py:327-327
Timestamp: 2026-07-27T22:07:22.590Z
Learning: When tests read structured logging context from Python `logging.LogRecord` instances, don’t access `record.ctx` directly (it isn’t declared on `LogRecord` and will break static typing). Instead, use `getattr(record, "ctx", default)` (or an appropriate fallback) to safely handle cases where `ctx` may or may not be attached. This applies even if Ruff rule `B009` isn’t enabled in the repo.
Applied to files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pytests/data_processing/test_timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pytests/generation/test_timeseries_backend.py
📚 Learning: 2026-07-29T17:12:32.642Z
Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 654
File: tests/config/test_parameters.py:116-118
Timestamp: 2026-07-29T17:12:32.642Z
Learning: In Pydantic validation tests (e.g., models configured with `from_attributes`), when asserting failures from `model_validate(...)`, assert the structured error details (such as `ValidationError.errors()[0]["type"]`, e.g. `"model_attributes_type"`) rather than relying on the human-readable error message text. This keeps tests stable even if wording changes, while still verifying the correct validation rule is triggered.
Applied to files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pytests/data_processing/test_timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pytests/generation/test_timeseries_backend.py
📚 Learning: 2026-08-05T19:07:15.856Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 679
File: tests/conftest.py:89-96
Timestamp: 2026-08-05T19:07:15.856Z
Learning: In the Safe Synthesizer test suite, treat CPU and CUDA installation profiles as the supported pytest collection profiles because they install PyTorch. A bare installation without PyTorch is an incomplete, unsupported profile. GPU-marked test modules may import PyTorch before tests/conftest.py::pytest_collection_modifyitems executes, so do not require that collection hook to prevent such imports.
Applied to files:
tests/data_processing/test_prompt_tokens.pytests/llm/test_metadata.pytests/data_processing/test_timeseries_validation.pytests/data_processing/test_assembler.pytests/generation/test_timeseries_prompting.pytests/generation/test_timeseries_backend.py
📚 Learning: 2026-08-06T15:42:45.969Z
Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 703
File: src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py:356-362
Timestamp: 2026-08-06T15:42:45.969Z
Learning: In NeMo Safe Synthesizer Python code, do not flag diagnostics solely because they report raw DataFrame column names. This is an established convention used in preflight warnings and validation errors. Flag such reporting only when an explicit repository-wide logging or privacy policy requires redaction, or when the diagnostic includes cell values, records, or other sensitive data.
Applied to files:
src/nemo_safe_synthesizer/sdk/library_builder.pysrc/nemo_safe_synthesizer/training/huggingface_backend.pysrc/nemo_safe_synthesizer/data_processing/timeseries_validation.pysrc/nemo_safe_synthesizer/generation/timeseries_prompting.pysrc/nemo_safe_synthesizer/data_processing/assembler.pysrc/nemo_safe_synthesizer/data_processing/prompt_tokens.pysrc/nemo_safe_synthesizer/llm/metadata.pysrc/nemo_safe_synthesizer/generation/timeseries_backend.py
📚 Learning: 2026-06-04T16:14:09.868Z
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 526
File: tests/generation/test_vllm_backend.py:399-509
Timestamp: 2026-06-04T16:14:09.868Z
Learning: In this repo, only apply `pytest.mark.vllm` to smoke tests under `tests/smoke/` that actually run real vLLM GPU generation and therefore require per-file process isolation (e.g., `test-smoke-gpu-*` Makefile targets). Do not apply `pytest.mark.vllm` to unit-style tests under `tests/generation/` that merely import `vllm_backend` but never instantiate a real vLLM engine and never call `.generate()` (GPU not required). Note that `tests/conftest.py` auto-marks these as `unit` via `pytest_collection_modifyitems`, and `vllm` is not among the auto-mark categories—so if a test in `tests/generation/` has `vllm`, it should be treated as a review issue unless it meets the real GPU generation criteria above.
Applied to files:
tests/generation/test_timeseries_prompting.pytests/generation/test_timeseries_backend.py
🪛 LanguageTool
docs/developer-guide/example-generation.md
[style] ~451-~451: ‘new records’ might be wordy. Consider a shorter alternative.
Context: ... history and may generate one or more new records. 2. The backend parses and validates ea...
(EN_WORDINESS_PREMIUM_NEW_RECORDS)
🪛 Ruff (0.16.1)
tests/data_processing/test_prompt_tokens.py
[error] 33-33: Possible hardcoded password assigned to argument: "bos_token"
(S106)
[error] 35-35: Possible hardcoded password assigned to argument: "eos_token"
(S106)
🔇 Additional comments (13)
docs/developer-guide/example-generation.md (1)
262-266: LGTM!Also applies to: 448-462
src/nemo_safe_synthesizer/data_processing/timeseries_validation.py (1)
52-53: LGTM!Also applies to: 475-487
src/nemo_safe_synthesizer/training/huggingface_backend.py (1)
660-660: LGTM!Also applies to: 762-764
tests/data_processing/test_timeseries_validation.py (1)
152-166: LGTM!src/nemo_safe_synthesizer/sdk/library_builder.py (1)
328-329: LGTM!src/nemo_safe_synthesizer/generation/timeseries_prompting.py (1)
79-82: 🗄️ Data Integrity & IntegrationKeep schema-driven numeric coercion. The fixture’s
e_idands_indexcolumns are integer-valued, andmake_json_schemaassigns them theintegertype. The test manually assignsnumber, so its0.0prefix does not represent a production mismatch.> Likely an incorrect or invalid review comment.src/nemo_safe_synthesizer/llm/metadata.py (1)
10-10: LGTM!Also applies to: 44-46, 64-64, 350-358
src/nemo_safe_synthesizer/data_processing/assembler.py (1)
44-47: LGTM!Also applies to: 174-178, 202-206, 749-751, 963-985
tests/data_processing/test_assembler.py (1)
779-805: LGTM!tests/llm/test_metadata.py (1)
734-749: LGTM!src/nemo_safe_synthesizer/data_processing/prompt_tokens.py (1)
1-53: LGTM!tests/data_processing/test_prompt_tokens.py (1)
1-89: LGTM!tests/generation/test_timeseries_prompting.py (1)
12-15: LGTM!Also applies to: 77-109, 147-148, 187-188
| logger.warning( | ||
| f"Group '{state.group_id}' skipped after {state.generation_attempts} attempts without reaching " | ||
| f"the configured stop timestamp." | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the raw group value from this warning.
If group_id is an account, customer, or device identifier, Line 721 writes source data to operational logs. Log an aggregate or a non-sensitive group ordinal instead. Put attempt counts in extra={}.
Proposed fix
logger.warning(
- f"Group '{state.group_id}' skipped after {state.generation_attempts} attempts without reaching "
- f"the configured stop timestamp."
+ "A time-series group was skipped after reaching the generation attempt limit.",
+ extra={"generation_attempts": state.generation_attempts},
)Based on learnings, diagnostics that include cell values warrant a privacy review. As per path instructions, identifiers are high-risk data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.warning( | |
| f"Group '{state.group_id}' skipped after {state.generation_attempts} attempts without reaching " | |
| f"the configured stop timestamp." | |
| ) | |
| logger.warning( | |
| "A time-series group was skipped after reaching the generation attempt limit.", | |
| extra={"generation_attempts": state.generation_attempts}, | |
| ) |
Sources: Path instructions, Learnings
There was a problem hiding this comment.
agent (
review-pr): Confirmed; avoid adding another raw identifier log
The added warning writes the source group value directly to operational logs. Please replace it with a non-sensitive group ordinal and put attempt counts in structured extra fields. The same value also appears in existing warning, debug, progress-summary, and completion logs. Those sites should be cleaned up here or tracked explicitly as follow-up work.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository instructions ---'
if [ -f AGENTS.local.md ]; then
cat AGENTS.local.md
fi
if [ -f STYLE_GUIDE.md ]; then
sed -n '1,240p' STYLE_GUIDE.md
fi
printf '%s\n' '--- changed logging code and group-id references ---'
rg -n -C 4 --type py \
'(logger\.(debug|info|warning|error|exception)\(|group_id|next_group)' \
src/nemo_safe_synthesizer/generation/timeseries_backend.py
printf '%s\n' '--- current PR diff for the backend ---'
git diff --unified=20 HEAD^ HEAD -- src/nemo_safe_synthesizer/generation/timeseries_backend.py
printf '%s\n' '--- related test structure and logging assertions ---'
ast-grep outline tests/generation/test_timeseries_backend.py --items all --type function
rg -n -C 4 --type py \
'(caplog|logger|GroupState|_process_group_result|_generate_parallel_groups)' \
tests/generation/test_timeseries_backend.py testsLength of output: 176620
@binaryaaron, the inspection confirms the issue.
src/nemo_safe_synthesizer/generation/timeseries_backend.py exposes raw group values in:
- The new attempt-limit warning.
- The existing invalid-output warnings.
- The activation debug log.
- The per-group parallel progress summary.
- The completion log.
- The missing-prefix
GenerationError.
The fix should stay in this PR. GroupState should carry a stable, one-based group_ordinal assigned from the saved group registry order. The implementation should use that ordinal in human-readable messages. It should place generation_attempts, retry counts, record counts, thresholds, and progress values in extra={}. It must not add group_id to structured fields.
Add focused tests in tests/generation/test_timeseries_backend.py. The tests should use a sentinel group identifier and verify that it is absent from captured warning, debug, summary, completion, and error output. The tests should also verify the expected ordinal and attempt count in structured log context.
Thanks for the guidance.
🧬 Code Graph Analysis Results
src/nemo_safe_synthesizer/generation/timeseries_backend.py
-
Lines 693–764 —
_process_group_result
Incrementsstate.generation_attemptsand emits warnings containing the rawstate.group_idfor attempt-limit failures, zero-valid batches, high-invalid-fraction batches, and consecutive-batch failures. These warnings include attempt counters that are currently interpolated into message strings. -
Lines 766–816 —
_log_parallel_batch_summary
Builds per-group progress lines containing the rawstate.group_id, valid-record counts, and progress totals, then emits them through an informational summary log. -
Lines 818–952 —
_generate_parallel_groups
Initializes group states and emits logs containing group-related progress. Specifically:- Lines 862–865: debug log includes the raw
next_groupidentifier when activating a group. - Lines 924–928: completion log includes the raw
state.group_idand group completion counts. - Lines 941–949: invokes
_log_parallel_batch_summary, which logs raw group identifiers as described above.
- Lines 862–865: debug log includes the raw
-
Lines 482–504 —
_init_group_state
Accepts agroup_idand uses it to initializeGroupState; itsGenerationErrormessage also interpolates the raw group identifier when no prefix exists. -
Lines 164–205 —
GroupState
Definesgroup_idas the sourceTimeSeriesGroupValueand tracksgeneration_attempts,low_valid_fraction_count, valid/invalid record totals, and completion/failure state. These fields provide the structured values relevant to logging without exposing the identifier.
src/nemo_safe_synthesizer/observability.py
- Lines 774–789 —
get_logger
Returns the packageCategoryLogger, using structlog after observability initialization and a standard-library logger otherwise. This is the logger implementation used bytimeseries_backend.py.
✏️ Learnings added
Learnt from: binaryaaron
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 708
File: src/nemo_safe_synthesizer/generation/timeseries_backend.py:720-723
Timestamp: 2026-08-17T17:18:28.668Z
Learning: In `src/nemo_safe_synthesizer/generation/timeseries_backend.py`, `TimeSeriesGroupValue` values originate from the saved training group registry and can be sensitive identifiers. Do not include raw group values in operational logs, structured log fields, or user-facing generation errors. Use a stable non-sensitive group ordinal for diagnostics and put counts, thresholds, and progress metrics in `extra={}`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: zywind
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 703
File: src/nemo_safe_synthesizer/pii_replacer/data_editor/detect.py:356-362
Timestamp: 2026-08-06T15:42:45.969Z
Learning: In NeMo Safe Synthesizer Python code, do not flag diagnostics solely because they report raw DataFrame column names. This is an established convention used in preflight warnings and validation errors. Flag such reporting only when an explicit repository-wide logging or privacy policy requires redaction, or when the diagnostic includes cell values, records, or other sensitive data.
You are interacting with an AI system.
There was a problem hiding this comment.
Addressed. Each group now receives a stable one-based ordinal derived from the saved registry order. Raw group values were removed from warnings, activation/debug output, progress summaries, completion logs, and missing-prefix errors. Counts, thresholds, retries, and progress are emitted through structured extra fields without including group_id. Added tests using a sentinel identifier to verify that it never appears in diagnostics.
| reached_stop = self._has_reached_stop_time([r.parsed for r in retained_records if r.parsed is not None]) | ||
| self._update_group_state(state, retained_records) | ||
| patience = self.config.generation.patience | ||
| attempt_limit = max(1, state.expected_records) + patience |
There was a problem hiding this comment.
agent (
review-pr): Count consecutive no-progress attempts
generation_attempts increases for every batch, including batches that produce valid records. A group that alternates valid and invalid batches can reach expected_records + patience while still advancing and without ever exhausting the consecutive-invalid patience counter. The threshold-crossing batch may also contain valid records, but the FAILED path does not add that batch to GenerationBatches, so those records are discarded. Track consecutive no-progress attempts and reset the counter after progress, or remove this separate cap and rely on the existing consecutive-failure logic.
There was a problem hiding this comment.
Addressed. I replaced the absolute generation_attempts cap with a consecutive no-progress counter. It resets whenever an accepted timestamp advances and only fails after patience consecutive batches without progress. Progress is evaluated after data actions, and the threshold-crossing batch is retained so valid records are not discarded. Added regression coverage for both reset and failure behavior.
|
Thanks all the feedback!! Here's a summary of what has changed: Review feedback addressed
|
3 participants
Summary
Notes:
Impact:
First Round Review feedback addressed
prefix: incomplete initial JSON record.history: accepted generated records used in later prompts.RecordPromptState: manages prefix/history state.Pre-Review Checklist
Ensure that the following pass:
mise run format && mise run checkor via prek validation.mise run testpasses locallymise run test:e2epasses locallymise run test:ci-containerpasses locally (recommended)/syncon this PR to trigger a run (auto-triggers on ready-for-review)Pre-Merge Checklist
Other Notes
Summary by CodeRabbit