refactor(llm): centralize model-family metadata initialization

[binaryaaron]

Summary

• centralize model-family prompt and RoPE metadata while standardizing tokenizer types
• derive special-token IDs from tokenizers and reject required BOS tokens that resolve to the unknown-token ID
• recognize canonical meta-llama/Llama-3.2-* model IDs and use Llama's native BOS token
• keep unsupported RoPE overrides from bypassing model-family policy

Test plan

• uv run --frozen pytest tests/llm/test_metadata.py -n 0 -q — 74 passed
• formatting, lint, type checking, and copyright checks
• full unit suite — 1,487 passed; one unrelated parallel vLLM teardown race passed when rerun alone

Made with Cursor

Summary by CodeRabbit

• New Features
  • Centralized model metadata initialization across supported model families, including smarter default config/tokenizer handling and prompt setup.
  • Added family-level controls for prompt template and BOS/EOS prompt injection, with improved automatic model detection using family naming markers.
  • Introduced rope_scaling_factor as a convenient input for RoPE configuration.
• Bug Fixes
  • Unsupported RoPE scaling inputs are now ignored safely.
  • Updated Llama 3.2 BOS handling and added stronger validation when required BOS tokens can’t be resolved.
• Tests
  • Expanded coverage for Granite, Llama 3.2, autoconfig pass-through, RoPE behavior, and BOS resolution failure cases.

[kendrickb-nvidia]

suggestion: This should have a comment/explanation that the token id here is what's injected by mock_tokenizer and does not reflect the actual token ids of the named model that would be used running NSS.

[nina-xu]

haha this is a good suggestion. it scared me when i saw this change before seeing kendrick's comment haha

[Copilot]

Pull request overview

This PR is a follow-up to the metadata standardization work, fixing a LLMPromptConfig.from_tokenizer fallback bug and aligning tokenizer types/usage across ModelMetadata subclasses to avoid silently missing BOS/EOS configuration and hardcoded special-token IDs.

Changes:

• Fix LLMPromptConfig.from_tokenizer to use the resolved tokenizer instance for getattr(...) fallbacks.
• Standardize tokenizer parameter types to PreTrainedTokenizer | None, consistently pass tokenizer= into from_tokenizer, and derive <|im_start|> token IDs dynamically for Llama32/SmolLM2/SmolLM3.
• Update unit tests’ mock tokenizer to support convert_tokens_to_ids, and adjust documentation about generation special-token output behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
tests/llm/test_metadata.py	Updates expected BOS token IDs and extends the tokenizer mock to support dynamic ID lookup.
src/nemo_safe_synthesizer/llm/metadata.py	Fixes tokenizer fallback bug, unifies tokenizer typing, and removes hardcoded `<
src/nemo_safe_synthesizer/generation/AGENTS.md	Updates generation “Gotchas” docs around special-token outputs and EOS handling.

---

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

[Copilot]

This description of need_special_token_outputs is inaccurate: the stated condition "not TabularDataProcessor ... or not TimeSeriesDataProcessor" would be true for essentially any concrete processor, and it also doesn't match the code. In generation/vllm_backend.py, the condition is need_special_token_outputs = not isinstance(self.processor, TabularDataProcessor), while timeseries_backend.py sets skip_special_tokens=True/include_stop_str_in_output=False unconditionally. Please update this doc line to reflect the actual logic per backend.

Suggested change

- `need_special_token_outputs` — `True` when processor is not `TabularDataProcessor` (VllmBackend) or not `TimeSeriesDataProcessor` (TimeseriesBackend). When True: `skip_special_tokens=False`, `include_stop_str_in_output=True`. `ignore_eos` is always `False` (native EOS stopping). Needed for GroupedDataProcessor BOS/EOS.

- `need_special_token_outputs` — in `VllmBackend`, this is `True` when the processor is not `TabularDataProcessor`, and then we use `skip_special_tokens=False` and `include_stop_str_in_output=True` (needed e.g. for `GroupedDataProcessor` BOS/EOS). In `TimeseriesBackend`, special tokens are always skipped and stop strings are always excluded (`skip_special_tokens=True`, `include_stop_str_in_output=False`), independent of this flag. `ignore_eos` is always `False` (native EOS stopping).

[nina-xu]

thanks aaron. due to my lack of context for this fix: what is the behavior this is fixing? the PR description states the root cause for the bug, not the resulting behavior

[kendrickb-nvidia]

Is this PR still needed, or were these fixes made in some other recent PRs? I feel like I've seen these changes already.

[codecov]

Codecov Report

❌ Patch coverage is 99.20635% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/nemo_safe_synthesizer/llm/metadata.py	98.94%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

---

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

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Refactors ModelMetadata to centralize model-family initialization, adds declarative family metadata through ModelFamilyMetadata, updates model-name matching and RoPE handling, and expands tests for Granite, Llama 3.2, BOS resolution, autoconfig passthrough, and unsupported RoPE scaling.

Changes

Model family metadata centralization

Layer / File(s)	Summary
Centralized ModelMetadata initialization and resolution src/nemo_safe_synthesizer/llm/metadata.py	Adds shared family defaults and initialization for prompt configuration, configuration/tokenizer loading, BOS resolution, RoPE normalization, and model-marker matching.
Declarative model-family wiring src/nemo_safe_synthesizer/llm/metadata.py	Adds ModelFamilyMetadata and converts eight model families to declarative prompt, token, marker, and RoPE settings with forwarding constructors.
Metadata detection and initialization tests tests/llm/test_metadata.py	Adds Granite and canonical Llama 3.2 detection and initialization coverage, plus tests for BOS failures, autoconfig preservation, and unsupported RoPE scaling.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested labels: test

Suggested reviewers: nina-xu

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: centralizing model-family metadata initialization.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch binaryaaron/fix-metadata-tokenizer-types

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0e1ad640-08d7-4e47-8b7c-aa06fd142baa

📥 Commits

Reviewing files that changed from the base of the PR and between 8a86f8b and 645d430.

📒 Files selected for processing (2)

• src/nemo_safe_synthesizer/llm/metadata.py
• tests/llm/test_metadata.py

📜 Review details

⏰ Context from checks skipped due to timeout. (1)

• GitHub Check: Greptile Review

🧰 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/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.py

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Place durable implementation guidance in function and class docstrings for public contracts and source comments for local invariants
Target Python 3.11–3.13 with modern syntax (X | Y, list[str], Self). Python 3.14+ is not supported

**/*.py: Source code must remain Python 3.11 syntax-compatible; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic class/function parameters in shared package code
Use ruff for formatting and linting via mise run format and mise run check tasks; run formatting before committing
Use ty for type checking via mise run check task; ensure type hints are present and valid
New features must include tests; bug fixes must include regression tests

**/*.py: Use BaseSettings for env/CLI settings, prefer AliasChoices for per-field dual naming, and use env_prefix only for simple settings classes with a shared prefix.
Use Field(description=...) as the canonical field docstring for Pydantic models, and always include it.
Use assignment-style Field(default=..., description="...") as the default for model fields; prefer it over Annotated unless extra metadata is needed.
Use Annotated only when the field carries additional metadata beyond Field() (for example validators, reusable constrained aliases, nested-type constraints, or discriminated unions).
When Annotated is used, place defaults as bare assignments (= value), except default_factory, which should still use assignment-style Field(default_factory=...).
For immutable value objects and validators, prefer @dataclass(frozen=True); use mutable dataclasses only for builders, accumulators, and pipeline state.
Use field(default_factory=list) for mutable defaults; never use = [].
Use StrEnum for string-valued enums used in configs or serialization; use plain Enum for internal-only named constants.
Use observability.get_logger(__name__) for logging; do not call logging.getLogger() or structlog.get_logger() direc...

Files:

• tests/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.py

**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use the unit marker instead of the deprecated unit_test marker for test identification

Files:

• tests/llm/test_metadata.py

tests/**

📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)

tests/**: Mirror src/ directory structure in tests/ directory for test organization
Auto-mark tests by directory: tests/e2e/ → e2e, tests/smoke/ → smoke, otherwise default to unit

Mirror source code directory structure in tests directory (e.g., tests/training/, tests/generation/ parallel to source structure)

Files:

• tests/llm/test_metadata.py

---

⚙️ CodeRabbit configuration file

tests/**:

Testing Guide

Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.

Read First

1. tests/conftest.py -- auto-marking, load_test_dataset/load_test_dataframe, fixture_mock_processor pattern
2. pytest.ini -- markers, asyncio, timeout
3. tests/evaluation/conftest.py -- most complex: Faker-based make_df, nullable dtype conversion
4. tests/generation/conftest.py -- JSONL/schema fixtures, fixture_valid_iris_dataset_jsonl_and_schema

Running Tests

All mise test tasks, grouped by scope:

mise run test                              # Unit (excludes slow, e2e, and smoke)
mise run test:unit-slow                    # Unit tests including slow (excludes e2e and smoke)
mise run test:smoke                        # CPU smoke tests (~few min, no GPU required)
mise run test:smoke:gpu                    # All staged GPU smoke tests (requires CUDA)
mise run test:smoke:gpu:train-only
mise run test:smoke:gpu:generation
mise run test:smoke:gpu:resume
mise run test:smoke:gpu:structured-generation
mise run test:smoke:gpu:timeseries
mise run test:smoke:gpu:smollm2
mise run test:e2e                          # All e2e (requires CUDA) -- runs default + dp
mise run test:e2e:default                  # e2e default (no-DP) tests only
mise run test:e2e:dp                       # e2e DP tests only
mise run test:ci                           # CI unit tests with coverage (excludes slow, e2e, gpu, smoke)
mise run test:ci-slow                      # CI slow tests with coverage
mise run test:ci-container                 # CI tests in a Linux container (Docker/Podman)

Run a single test:

uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0

Test runner: uv run --frozen pytest -n auto --dist loadscope -vv...

Files:

• tests/llm/test_metadata.py

**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers; mise run format adds them automatically

Files:

• tests/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.py

tests/**/*.py

📄 CodeRabbit inference engine (tests/TESTING.md)

tests/**/*.py: Auto-mark tests based on file path: tests under /e2e/ get e2e marker, tests under /smoke/ get smoke marker, all others get unit marker (only if no category marker already present)
Every test should have exactly one category marker: unit, smoke, or e2e
Use pytest.mark.requires_gpu modifier on tests that need CUDA hardware
Use pytest.mark.vllm on tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Use pytest.mark.slow on long-running tests
Use pytest.mark.smollm2 for SmolLM2 Hub download tests to enable process isolation
Use pytest.mark.noautouse to skip autouse fixtures for specific tests
Use load_test_dataset(filename) helper to load test datasets from tests/stub_datasets/ as HuggingFace Dataset objects
Use load_test_dataframe(filename) helper to load test data files from tests/stub_datasets/ as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(), pd.BooleanDtype()) before assigning np.nan values
Use fake.seed_instance(seed) and random.seed(seed) together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them in conftest.py and import them using relative imports (e.g., from .conftest import train_with_sdk); note that importing from other test files like tests/cli/helpers.py does not work
Use fixture_mock_processor or fixture_mock_processor_without_valid_records for mocking ParsedResponse objects with valid_records, invalid_records, errors, and prompt_number fields
Use pytest.importorskip to gate tests on optional dependencies that require specific extras (e.g., sentence_transformers, vllm)
Run vLLM tests with separate pytest invocations (one per file) using -n 0 (single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruff T201 is suppressed for tests/ directory) and should...

Files:

• tests/llm/test_metadata.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/llm/test_metadata.py

**/*

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*: Every source file must include the required SPDX copyright and license header; use HTML comments for Markdown, hash comments for .py, .sh, .yaml, and .yml, and hash-comment headers inside YAML frontmatter for Markdown files with frontmatter.
Ensure files end with a newline and have no trailing whitespace; use a single space between sentences.

Files:

• tests/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.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/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.py

**

⚙️ CodeRabbit configuration file

**:

AGENTS.md

Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.

This project loads local developer preferences from @AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.

Skills

Repo-specific skills live in .agents/skills/; see .agents/README.md for the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.

Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in tests/TESTING.md.

Repo Conventions

See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).

Use uv for everything -- never pip or raw python. Python 3.11–3.13 with modern syntax (X | Y, list[str], Self). Python 3.14+ is not supported.

Common commands: mise run test (unit tests), mise run format (auto-fix formatting + lint + copyright), mise run check (read-only local quality checks), mise run validate (pre-PR quality, lock, and CI unit checks), mise run typecheck (ty only). Always use mise tasks or the wrapper scripts in tools/ instead of running ruff or ty directly. Use uv run for Python execution. When in doubt, inspect mise tasks and pytest --markers.

The canonical uv sync command for a full GPU/dev environment is:

uv sync --frozen --extra cu129 --extra engine --group dev

Bare uv sync --frozen (without extras) installs an incomplete environment -- ty, import checks, and GPU tests will fail.

Feature branches off main. Branch names often include an issue number prefix (e.g., <author>/123-short-name).

Do ...

Files:

• tests/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.py

src/nemo_safe_synthesizer/**/*.py

📄 CodeRabbit inference engine (CONTRIBUTING.md)

API reference pages are auto-generated from Python docstrings using Google-style format; write docstrings in src/nemo_safe_synthesizer/ and they will appear in the reference/

Files:

• src/nemo_safe_synthesizer/llm/metadata.py

src/**/*.py

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

src/**/*.py: Use relative imports inside src/, for example from ..observability import get_logger.
Every directory under src/ that contains Python files must include an __init__.py file.

Files:

• src/nemo_safe_synthesizer/llm/metadata.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/llm/metadata.py

🧠 Learnings (1)

📚 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/llm/test_metadata.py

🪛 Ruff (0.15.20)

src/nemo_safe_synthesizer/llm/metadata.py

[error] 796-796: Possible hardcoded password assigned to: "bos_token"

(S105)

---

[error] 897-897: Possible hardcoded password assigned to: "bos_token"

(S105)

---

[error] 927-927: Possible hardcoded password assigned to: "bos_token"

(S105)

🔇 Additional comments (2)

src/nemo_safe_synthesizer/llm/metadata.py (1)

280-293: LGTM!

Also applies to: 356-432, 574-576, 739-757, 767-784, 793-808, 822-835, 845-857, 867-881, 894-915, 924-940, 953-957

tests/llm/test_metadata.py (1)

35-35: LGTM!

Also applies to: 99-99, 132-165, 312-312

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Suppress the Ruff S105 false positives for BOS token literals.

These are tokenizer special tokens, not secrets, but Ruff flags assignments to names containing token. Add targeted # noqa: S105 comments or a narrow per-file ignore so mise run check does not fail.

Also applies to: 897-897, 927-927

🧰 Tools

🪛 Ruff (0.15.20)

[error] 796-796: Possible hardcoded password assigned to: "bos_token"

(S105)

Source: Linters/SAST tools

[greptile-apps]

Greptile Summary

This PR centralizes model-family initialization into a single ModelMetadata.__init__ using class-level declarations (prompt_template, bos_token, supports_rope_scaling, model_name_markers), eliminating the copy-pasted __init__ bodies from each concrete subclass. It also adds dynamic BOS-token ID resolution with an unknown-token guard, adds canonical llama-3.2 name markers so real HF model IDs are recognized, and makes unsupported RoPE scaling bypass impossible through the new centralized path.

• Centralized family metadata: Class-level ClassVar attributes replace per-subclass __init__ boilerplate; a new ModelFamilyMetadata shim adapts the keyword-only ModelMetadata.__init__ for positional callers.
• Dynamic BOS resolution: _prompt_config_from_tokenizer now validates convert_tokens_to_ids against unk_token_id, rejecting tokens missing from the vocabulary instead of silently using the unknown-token ID.
• Llama 3.2 fix: Old hardcoded <|im_start|> / 151644 (a Qwen-tokenizer token) is replaced by reading the native BOS from the loaded tokenizer, so canonical meta-llama/Llama-3.2-* paths now resolve correctly.

Confidence Score: 5/5

Safe to merge; the refactoring consolidates well-understood logic into a central path and all changed behaviour is directly covered by the expanded test suite.

The centralization of family-level prompt and RoPE metadata eliminates repeated boilerplate without changing semantics for any model family. The BOS-token validation addition and the Llama 3.2 native-BOS fix both correct pre-existing defects. The 74-test metadata suite and the full 1,487-test run both pass. The only observation is an edge-case silent drop of rope_scaling_factor when rope_scaling=None is already in the kwarg dict, which requires an unusual contradictory call pattern to trigger.

No files require special attention. metadata.py contains the bulk of the logic change but the new centralized __init__ path is straightforward and well-tested.

Important Files Changed

Filename	Overview
src/nemo_safe_synthesizer/llm/metadata.py	Core refactoring: introduces centralized init, class-level family attrs, dynamic BOS resolution with unk-token guard, rope scaling consolidation, and canonical Llama-3.2 name markers. Logic is sound and consistent.
tests/llm/test_metadata.py	Adds tests for Granite detection/init, canonical Llama-3.2 ID recognition, unknown-BOS rejection, autoconfig passthrough, and rope-scaling bypass prevention. Coverage aligns well with all newly centralized code paths.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant FamilySubclass as FamilySubclass.__init__
    participant MFM as ModelFamilyMetadata.__init__
    participant MM as ModelMetadata.__init__
    participant LCT as _load_config_and_tokenizer
    participant PCT as _prompt_config_from_tokenizer
    participant RSF as _rope_scaling_from_factor
    participant Pydantic as Pydantic BaseModel

    Caller->>FamilySubclass: "__init__(model_name_or_path, tokenizer?, rope_scaling_factor?, **kwargs)"
    FamilySubclass->>MFM: super().__init__(...)
    MFM->>MM: "super().__init__(model_name_or_path=..., ...)"
    MM->>MM: data.setdefault(rope_parameters_location, cls.rope_parameters_location_default)

    alt prompt_config is None
        alt autoconfig is None OR tokenizer is None
            MM->>LCT: _load_config_and_tokenizer(model_name_or_path, tokenizer)
            LCT-->>MM: loaded_autoconfig, tokenizer
        end
        MM->>PCT: _prompt_config_from_tokenizer(model_name_or_path, tokenizer)
        note over PCT: If cls.bos_token is str: resolve ID via convert_tokens_to_ids, validate != unk_token_id
        PCT-->>MM: LLMPromptConfig
    end

    alt not supports_rope_scaling AND rope_scaling in data
        MM->>MM: "warn + data[rope_scaling] = None"
    end

    alt rope_scaling_factor provided AND rope_scaling NOT yet in data
        MM->>RSF: _rope_scaling_from_factor(rope_scaling_factor)
        note over RSF: If not supports_rope_scaling: warn + return None
        RSF-->>MM: rope_scaling value or None
    end

    MM->>Pydantic: "super().__init__(model_name_or_path=..., prompt_config=..., **data)"
    Pydantic->>Pydantic: populate_derived_fields validator
    Pydantic-->>Caller: ModelMetadata instance

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant FamilySubclass as FamilySubclass.__init__
    participant MFM as ModelFamilyMetadata.__init__
    participant MM as ModelMetadata.__init__
    participant LCT as _load_config_and_tokenizer
    participant PCT as _prompt_config_from_tokenizer
    participant RSF as _rope_scaling_from_factor
    participant Pydantic as Pydantic BaseModel

    Caller->>FamilySubclass: "__init__(model_name_or_path, tokenizer?, rope_scaling_factor?, **kwargs)"
    FamilySubclass->>MFM: super().__init__(...)
    MFM->>MM: "super().__init__(model_name_or_path=..., ...)"
    MM->>MM: data.setdefault(rope_parameters_location, cls.rope_parameters_location_default)

    alt prompt_config is None
        alt autoconfig is None OR tokenizer is None
            MM->>LCT: _load_config_and_tokenizer(model_name_or_path, tokenizer)
            LCT-->>MM: loaded_autoconfig, tokenizer
        end
        MM->>PCT: _prompt_config_from_tokenizer(model_name_or_path, tokenizer)
        note over PCT: If cls.bos_token is str: resolve ID via convert_tokens_to_ids, validate != unk_token_id
        PCT-->>MM: LLMPromptConfig
    end

    alt not supports_rope_scaling AND rope_scaling in data
        MM->>MM: "warn + data[rope_scaling] = None"
    end

    alt rope_scaling_factor provided AND rope_scaling NOT yet in data
        MM->>RSF: _rope_scaling_from_factor(rope_scaling_factor)
        note over RSF: If not supports_rope_scaling: warn + return None
        RSF-->>MM: rope_scaling value or None
    end

    MM->>Pydantic: "super().__init__(model_name_or_path=..., prompt_config=..., **data)"
    Pydantic->>Pydantic: populate_derived_fields validator
    Pydantic-->>Caller: ModelMetadata instance

Loading

_{Reviews (6): Last reviewed commit: "test(llm): cover unsupported rope scalin..." | Re-trigger Greptile}

[greptile-apps]

Missing unknown-token guard on dynamic BOS id lookup

convert_tokens_to_ids with a single string always returns an int, even when the token is not in the tokenizer's vocabulary — in that case it silently returns unk_token_id (often 0 or 3). The isinstance(bos_token_id, int) guard only catches callers who mistakenly pass a list; it does not catch an unrecognised token. A model loaded with a mismatched or stripped tokenizer (no <|im_start|> in the vocab) would proceed with the wrong id and produce silent, hard-to-diagnose prompt corruption. Could you add a check like if bos_token_id == tokenizer.unk_token_id: raise ValueError(...) after the existing isinstance guard, to catch the case where the token is not in the vocabulary?

[mckornfield]

is this a valid problem? i.e., how far would you get with a bad token

[greptile-apps]

Yes, it's a real problem, but the failure mode is subtle rather than explosive.

If convert_tokens_to_ids returns unk_token_id (e.g. 0 or 3), inference still proceeds — the wrong token's embedding gets prepended to every prompt. For instruction-tuned models, the BOS token is often load-bearing in the chat template (e.g. <|begin_of_text|> for Llama). Swapping it silently for <unk> means:

• The model receives a structurally malformed prompt on every call
• Output quality degrades — sometimes subtly (slightly off-format responses), sometimes significantly (refusals, repetition, incoherence), depending on how sensitive the model is to that token
• No error is raised, no log is emitted — you'd need to inspect the actual token IDs being sent to notice

The isinstance(bos_token_id, int) guard is actually a no-op here, since convert_tokens_to_ids with a single string always returns an int. So the bug the PR description claims to fix ("reject required BOS tokens that resolve to the unknown-token ID") is not actually implemented in the shown diff.

The realistic trigger: a Llama-family class loaded with a stripped or mismatched tokenizer (common in quantized model repos that omit special_tokens_map.json or use a generic tokenizer). The check I suggested would surface this immediately at init time rather than letting it silently corrupt every synthetic sample downstream.

Worth adding — the fix is a one-liner:

Suggested change

	match cls.bos_token:
	case str() as bos_token:
	bos_token_id = tokenizer.convert_tokens_to_ids(bos_token)
	if not isinstance(bos_token_id, int):
	raise ValueError(f"Tokenizer did not resolve BOS token {bos_token!r} to an integer token id")
	prompt_kwargs["bos_token"] = bos_token
	prompt_kwargs["bos_token_id"] = bos_token_id
	if not isinstance(bos_token_id, int):
	raise ValueError(f"Tokenizer did not resolve BOS token {bos_token!r} to an integer token id")
	if bos_token_id == tokenizer.unk_token_id:
	raise ValueError(
	f"BOS token {bos_token!r} resolved to unk_token_id ({tokenizer.unk_token_id}); "
	"the tokenizer vocabulary does not contain this token"
	)

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

[greptile-apps]

Typo in the inline comment — "tockenizer" should be "tokenizer".

Suggested change

	expected_bos_token_id=1, # this is the token_id that the mock_tockenizer injects, not what a real model would use
	expected_bos_token_id=1, # this is the token_id that the mock_tokenizer injects, not what a real model would use

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[greptile-apps]

User-provided autoconfig silently discarded when prompt_config is omitted

If a caller passes a pre-loaded autoconfig=custom_config to any model-family subclass but does not also pass prompt_config, the custom config is immediately overwritten by the freshly HF-loaded one from _load_config_and_tokenizer. The parameter is consumed from **data and bound to the explicit autoconfig argument before this block runs, so there is no way for the caller to avoid the discard. The old per-subclass code had identical behaviour, so this is not a regression, but it is now a single place where a guard like if autoconfig is None: around the load call could unify behaviour.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/nemo_safe_synthesizer/llm/metadata.py (1)

776-783: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Redundant per-family __init__ overrides.

ModelFamilyMetadata.__init__ (Line 748) already forwards model_name_or_path, tokenizer, and rope_scaling_factor into ModelMetadata. Each family subclass (Granite here, plus Llama32, Mistral, Nemotron, Qwen, SmolLM2, SmolLM3, TinyLlama) redeclares a byte-for-byte identical forwarding constructor, which adds no behavior and must be copied for every new family. Dropping these overrides lets the declarative ClassVars alone define each family and keeps the intended "add a subclass, set class vars" extension model clean.

♻️ Proposed simplification (applies to each family class)

 class Granite(ModelFamilyMetadata):
     prompt_template: ClassVar[str] = "user\n {instruction} {schema} \n assistant\n{prefill}"
     add_bos_token_to_prompt: ClassVar[bool] = False
-
-    def __init__(
-        self,
-        model_name_or_path: str,
-        tokenizer: PreTrainedTokenizerBase | None = None,
-        rope_scaling_factor: float | int | None = None,
-        **kwargs,
-    ) -> None:
-        super().__init__(model_name_or_path, tokenizer=tokenizer, rope_scaling_factor=rope_scaling_factor, **kwargs)

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 02fe8737-827f-4160-9ec5-715b7c3a52cb

📥 Commits

Reviewing files that changed from the base of the PR and between 645d430 and 95aace8.

📒 Files selected for processing (2)

• src/nemo_safe_synthesizer/llm/metadata.py
• tests/llm/test_metadata.py

📜 Review details

⏰ Context from checks skipped due to timeout. (3)

• GitHub Check: Greptile Review
• GitHub Check: Format
• GitHub Check: Detect changes

🧰 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/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.py

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Place durable implementation guidance in function and class docstrings for public contracts and source comments for local invariants
Target Python 3.11–3.13 with modern syntax (X | Y, list[str], Self). Python 3.14+ is not supported

**/*.py: Source code must remain Python 3.11 syntax-compatible; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic class/function parameters in shared package code
Use ruff for formatting and linting via mise run format and mise run check tasks; run formatting before committing
Use ty for type checking via mise run check task; ensure type hints are present and valid
New features must include tests; bug fixes must include regression tests

**/*.py: Use BaseSettings for env/CLI settings, prefer AliasChoices for per-field dual naming, and use env_prefix only for simple settings classes with a shared prefix.
Use Field(description=...) as the canonical field docstring for Pydantic models, and always include it.
Use assignment-style Field(default=..., description="...") as the default for model fields; prefer it over Annotated unless extra metadata is needed.
Use Annotated only when the field carries additional metadata beyond Field() (for example validators, reusable constrained aliases, nested-type constraints, or discriminated unions).
When Annotated is used, place defaults as bare assignments (= value), except default_factory, which should still use assignment-style Field(default_factory=...).
For immutable value objects and validators, prefer @dataclass(frozen=True); use mutable dataclasses only for builders, accumulators, and pipeline state.
Use field(default_factory=list) for mutable defaults; never use = [].
Use StrEnum for string-valued enums used in configs or serialization; use plain Enum for internal-only named constants.
Use observability.get_logger(__name__) for logging; do not call logging.getLogger() or structlog.get_logger() direc...

Files:

• tests/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.py

**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use the unit marker instead of the deprecated unit_test marker for test identification

Files:

• tests/llm/test_metadata.py

tests/**

📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)

tests/**: Mirror src/ directory structure in tests/ directory for test organization
Auto-mark tests by directory: tests/e2e/ → e2e, tests/smoke/ → smoke, otherwise default to unit

Mirror source code directory structure in tests directory (e.g., tests/training/, tests/generation/ parallel to source structure)

Files:

• tests/llm/test_metadata.py

---

⚙️ CodeRabbit configuration file

tests/**:

Testing Guide

Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.

Read First

1. tests/conftest.py -- auto-marking, load_test_dataset/load_test_dataframe, fixture_mock_processor pattern
2. pytest.ini -- markers, asyncio, timeout
3. tests/evaluation/conftest.py -- most complex: Faker-based make_df, nullable dtype conversion
4. tests/generation/conftest.py -- JSONL/schema fixtures, fixture_valid_iris_dataset_jsonl_and_schema

Running Tests

All mise test tasks, grouped by scope:

mise run test                              # Unit (excludes slow, e2e, and smoke)
mise run test:unit-slow                    # Unit tests including slow (excludes e2e and smoke)
mise run test:smoke                        # CPU smoke tests (~few min, no GPU required)
mise run test:smoke:gpu                    # All staged GPU smoke tests (requires CUDA)
mise run test:smoke:gpu:train-only
mise run test:smoke:gpu:generation
mise run test:smoke:gpu:resume
mise run test:smoke:gpu:structured-generation
mise run test:smoke:gpu:timeseries
mise run test:smoke:gpu:smollm2
mise run test:e2e                          # All e2e (requires CUDA) -- runs default + dp
mise run test:e2e:default                  # e2e default (no-DP) tests only
mise run test:e2e:dp                       # e2e DP tests only
mise run test:ci                           # CI unit tests with coverage (excludes slow, e2e, gpu, smoke)
mise run test:ci-slow                      # CI slow tests with coverage
mise run test:ci-container                 # CI tests in a Linux container (Docker/Podman)

Run a single test:

uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0

Test runner: uv run --frozen pytest -n auto --dist loadscope -vv...

Files:

• tests/llm/test_metadata.py

**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers; mise run format adds them automatically

Files:

• tests/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.py

tests/**/*.py

📄 CodeRabbit inference engine (tests/TESTING.md)

tests/**/*.py: Auto-mark tests based on file path: tests under /e2e/ get e2e marker, tests under /smoke/ get smoke marker, all others get unit marker (only if no category marker already present)
Every test should have exactly one category marker: unit, smoke, or e2e
Use pytest.mark.requires_gpu modifier on tests that need CUDA hardware
Use pytest.mark.vllm on tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Use pytest.mark.slow on long-running tests
Use pytest.mark.smollm2 for SmolLM2 Hub download tests to enable process isolation
Use pytest.mark.noautouse to skip autouse fixtures for specific tests
Use load_test_dataset(filename) helper to load test datasets from tests/stub_datasets/ as HuggingFace Dataset objects
Use load_test_dataframe(filename) helper to load test data files from tests/stub_datasets/ as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(), pd.BooleanDtype()) before assigning np.nan values
Use fake.seed_instance(seed) and random.seed(seed) together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them in conftest.py and import them using relative imports (e.g., from .conftest import train_with_sdk); note that importing from other test files like tests/cli/helpers.py does not work
Use fixture_mock_processor or fixture_mock_processor_without_valid_records for mocking ParsedResponse objects with valid_records, invalid_records, errors, and prompt_number fields
Use pytest.importorskip to gate tests on optional dependencies that require specific extras (e.g., sentence_transformers, vllm)
Run vLLM tests with separate pytest invocations (one per file) using -n 0 (single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruff T201 is suppressed for tests/ directory) and should...

Files:

• tests/llm/test_metadata.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/llm/test_metadata.py

**/*

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*: Every source file must include the required SPDX copyright and license header; use HTML comments for Markdown, hash comments for .py, .sh, .yaml, and .yml, and hash-comment headers inside YAML frontmatter for Markdown files with frontmatter.
Ensure files end with a newline and have no trailing whitespace; use a single space between sentences.

Files:

• tests/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.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/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.py

**

⚙️ CodeRabbit configuration file

**:

AGENTS.md

Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.

This project loads local developer preferences from @AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.

Skills

Repo-specific skills live in .agents/skills/; see .agents/README.md for the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.

Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in tests/TESTING.md.

Repo Conventions

See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).

Use uv for everything -- never pip or raw python. Python 3.11–3.13 with modern syntax (X | Y, list[str], Self). Python 3.14+ is not supported.

Common commands: mise run test (unit tests), mise run format (auto-fix formatting + lint + copyright), mise run check (read-only local quality checks), mise run validate (pre-PR quality, lock, and CI unit checks), mise run typecheck (ty only). Always use mise tasks or the wrapper scripts in tools/ instead of running ruff or ty directly. Use uv run for Python execution. When in doubt, inspect mise tasks and pytest --markers.

The canonical uv sync command for a full GPU/dev environment is:

uv sync --frozen --extra cu129 --extra engine --group dev

Bare uv sync --frozen (without extras) installs an incomplete environment -- ty, import checks, and GPU tests will fail.

Feature branches off main. Branch names often include an issue number prefix (e.g., <author>/123-short-name).

Do ...

Files:

• tests/llm/test_metadata.py
• src/nemo_safe_synthesizer/llm/metadata.py

src/nemo_safe_synthesizer/**/*.py

📄 CodeRabbit inference engine (CONTRIBUTING.md)

API reference pages are auto-generated from Python docstrings using Google-style format; write docstrings in src/nemo_safe_synthesizer/ and they will appear in the reference/

Files:

• src/nemo_safe_synthesizer/llm/metadata.py

src/**/*.py

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

src/**/*.py: Use relative imports inside src/, for example from ..observability import get_logger.
Every directory under src/ that contains Python files must include an __init__.py file.

Files:

• src/nemo_safe_synthesizer/llm/metadata.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/llm/metadata.py

🧠 Learnings (1)

📚 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/llm/test_metadata.py

🪛 Ruff (0.15.20)

tests/llm/test_metadata.py

[error] 131-131: Possible hardcoded password assigned to argument: "expected_bos_token"

(S106)

---

[error] 754-754: Possible hardcoded password assigned to: "unk_token"

(S105)

🔇 Additional comments (6)

src/nemo_safe_synthesizer/llm/metadata.py (2)

357-434: LGTM!

---

604-621: LGTM!

tests/llm/test_metadata.py (4)

996-1006: 🎯 Functional Correctness | ⚡ Quick win

Still missing rope_scaling_factor coverage for the unsupported-family path.

This test parametrizes only raw rope_scaling=2, but training.rope_scaling_factor also flows into these constructors via _rope_scaling_from_factor. A prior review on this same range already requested adding {"rope_scaling_factor": 2} as an additional parametrized case so regressions in that helper are caught for unsupported families too; the diff here still only exercises the raw-kwarg path.

---

94-94: LGTM!

Also applies to: 153-165

---

742-744: LGTM!

---

747-760: LGTM!

[mckornfield]

kind of a nit with the PR/commit, this is centralizing more than simplifying yeah? (although arguably they're similar)

[mckornfield]

is this a valid problem? i.e., how far would you get with a bad token

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

tests/llm/test_metadata.py (1)

127-132: 📐 Maintainability & Code Quality | 🟡 Minor | 💤 Low value

Document that expected_bos_token_id=10 is the mock-injected id.

mock_tokenizer.bos_token_id = 10 (Line 312) drives the Llama32 scenario's expected_bos_token_id=10; this is a test artifact, not Llama-3.2's real BOS id. Add a short inline note so future readers do not mistake it for the model's actual token id.

Also applies to: 312-312

🧹 Nitpick comments (1)

src/nemo_safe_synthesizer/llm/metadata.py (1)

780-980: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Redundant per-subclass __init__ forwarders.

Granite, Llama32, Mistral, Nemotron, Qwen, SmolLM2, SmolLM3, and TinyLlama each redefine an __init__ that is byte-for-byte identical to ModelFamilyMetadata.__init__ (same signature, same super().__init__(...) forwarding). Since these families now differ only by ClassVars, the forwarders can be dropped so each class inherits ModelFamilyMetadata.__init__, removing eight identical bodies. The class docstrings already carry the constructor Args:, so IDE/mkdocstrings hover is unaffected. This aligns with the PR's stated goal of simplifying the metadata objects.

♻️ Example: Granite reduced to declarations only

 class Granite(ModelFamilyMetadata):
     """Metadata for IBM Granite model family.

     Args:
         model_name_or_path: HuggingFace model identifier or local path.
         tokenizer: Optional pre-loaded tokenizer.
         rope_scaling_factor: Optional RoPE scaling factor.
         **kwargs: Forwarded to [`ModelMetadata`][nemo_safe_synthesizer.llm.metadata.ModelMetadata].
     """

     prompt_template: ClassVar[str] = "user\n {instruction} {schema} \n assistant\n{prefill}"
     add_bos_token_to_prompt: ClassVar[bool] = False
-
-    def __init__(
-        self,
-        model_name_or_path: str,
-        tokenizer: PreTrainedTokenizerBase | None = None,
-        rope_scaling_factor: float | int | None = None,
-        **kwargs,
-    ) -> None:
-        super().__init__(model_name_or_path, tokenizer=tokenizer, rope_scaling_factor=rope_scaling_factor, **kwargs)

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b9a6d333-48b8-46cc-92cb-098f3aa7907f

📥 Commits

Reviewing files that changed from the base of the PR and between 246820f and 71062f6.

📒 Files selected for processing (2)

• src/nemo_safe_synthesizer/llm/metadata.py
• tests/llm/test_metadata.py

📜 Review details

⏰ Context from checks skipped due to timeout. (9)

• GitHub Check: CodeRabbit
• GitHub Check: Unit Tests (3.12)
• GitHub Check: Unit Tests (3.11)
• GitHub Check: Unit Tests (3.13)
• 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:

• src/nemo_safe_synthesizer/llm/metadata.py
• tests/llm/test_metadata.py

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Place durable implementation guidance in function and class docstrings for public contracts and source comments for local invariants
Target Python 3.11–3.13 with modern syntax (X | Y, list[str], Self). Python 3.14+ is not supported

**/*.py: Source code must remain Python 3.11 syntax-compatible; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic class/function parameters in shared package code
Use ruff for formatting and linting via mise run format and mise run check tasks; run formatting before committing
Use ty for type checking via mise run check task; ensure type hints are present and valid
New features must include tests; bug fixes must include regression tests

**/*.py: Use BaseSettings for env/CLI settings, prefer AliasChoices for per-field dual naming, and use env_prefix only for simple settings classes with a shared prefix.
Use Field(description=...) as the canonical field docstring for Pydantic models, and always include it.
Use assignment-style Field(default=..., description="...") as the default for model fields; prefer it over Annotated unless extra metadata is needed.
Use Annotated only when the field carries additional metadata beyond Field() (for example validators, reusable constrained aliases, nested-type constraints, or discriminated unions).
When Annotated is used, place defaults as bare assignments (= value), except default_factory, which should still use assignment-style Field(default_factory=...).
For immutable value objects and validators, prefer @dataclass(frozen=True); use mutable dataclasses only for builders, accumulators, and pipeline state.
Use field(default_factory=list) for mutable defaults; never use = [].
Use StrEnum for string-valued enums used in configs or serialization; use plain Enum for internal-only named constants.
Use observability.get_logger(__name__) for logging; do not call logging.getLogger() or structlog.get_logger() direc...

Files:

• src/nemo_safe_synthesizer/llm/metadata.py
• tests/llm/test_metadata.py

**/*.{py,sh,yaml,yml,md}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers; mise run format adds them automatically

Files:

• src/nemo_safe_synthesizer/llm/metadata.py
• tests/llm/test_metadata.py

src/nemo_safe_synthesizer/**/*.py

📄 CodeRabbit inference engine (CONTRIBUTING.md)

API reference pages are auto-generated from Python docstrings using Google-style format; write docstrings in src/nemo_safe_synthesizer/ and they will appear in the reference/

Files:

• src/nemo_safe_synthesizer/llm/metadata.py

src/**/*.py

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

src/**/*.py: Use relative imports inside src/, for example from ..observability import get_logger.
Every directory under src/ that contains Python files must include an __init__.py file.

Files:

• src/nemo_safe_synthesizer/llm/metadata.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/llm/metadata.py

**/*

📄 CodeRabbit inference engine (STYLE_GUIDE.md)

**/*: Every source file must include the required SPDX copyright and license header; use HTML comments for Markdown, hash comments for .py, .sh, .yaml, and .yml, and hash-comment headers inside YAML frontmatter for Markdown files with frontmatter.
Ensure files end with a newline and have no trailing whitespace; use a single space between sentences.

Files:

• src/nemo_safe_synthesizer/llm/metadata.py
• tests/llm/test_metadata.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/llm/metadata.py
• tests/llm/test_metadata.py

**

⚙️ CodeRabbit configuration file

**:

AGENTS.md

Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.

This project loads local developer preferences from @AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.

Skills

Repo-specific skills live in .agents/skills/; see .agents/README.md for the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.

Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in tests/TESTING.md.

Repo Conventions

See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).

Use uv for everything -- never pip or raw python. Python 3.11–3.13 with modern syntax (X | Y, list[str], Self). Python 3.14+ is not supported.

Common commands: mise run test (unit tests), mise run format (auto-fix formatting + lint + copyright), mise run check (read-only local quality checks), mise run validate (pre-PR quality, lock, and CI unit checks), mise run typecheck (ty only). Always use mise tasks or the wrapper scripts in tools/ instead of running ruff or ty directly. Use uv run for Python execution. When in doubt, inspect mise tasks and pytest --markers.

The canonical uv sync command for a full GPU/dev environment is:

uv sync --frozen --extra cu129 --extra engine --group dev

Bare uv sync --frozen (without extras) installs an incomplete environment -- ty, import checks, and GPU tests will fail.

Feature branches off main. Branch names often include an issue number prefix (e.g., <author>/123-short-name).

Do ...

Files:

• src/nemo_safe_synthesizer/llm/metadata.py
• tests/llm/test_metadata.py

**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use the unit marker instead of the deprecated unit_test marker for test identification

Files:

• tests/llm/test_metadata.py

tests/**

📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)

tests/**: Mirror src/ directory structure in tests/ directory for test organization
Auto-mark tests by directory: tests/e2e/ → e2e, tests/smoke/ → smoke, otherwise default to unit

Mirror source code directory structure in tests directory (e.g., tests/training/, tests/generation/ parallel to source structure)

Files:

• tests/llm/test_metadata.py

---

⚙️ CodeRabbit configuration file

tests/**:

Testing Guide

Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.

Read First

1. tests/conftest.py -- auto-marking, load_test_dataset/load_test_dataframe, fixture_mock_processor pattern
2. pytest.ini -- markers, asyncio, timeout
3. tests/evaluation/conftest.py -- most complex: Faker-based make_df, nullable dtype conversion
4. tests/generation/conftest.py -- JSONL/schema fixtures, fixture_valid_iris_dataset_jsonl_and_schema

Running Tests

All mise test tasks, grouped by scope:

mise run test                              # Unit (excludes slow, e2e, and smoke)
mise run test:unit-slow                    # Unit tests including slow (excludes e2e and smoke)
mise run test:smoke                        # CPU smoke tests (~few min, no GPU required)
mise run test:smoke:gpu                    # All staged GPU smoke tests (requires CUDA)
mise run test:smoke:gpu:train-only
mise run test:smoke:gpu:generation
mise run test:smoke:gpu:resume
mise run test:smoke:gpu:structured-generation
mise run test:smoke:gpu:timeseries
mise run test:smoke:gpu:smollm2
mise run test:e2e                          # All e2e (requires CUDA) -- runs default + dp
mise run test:e2e:default                  # e2e default (no-DP) tests only
mise run test:e2e:dp                       # e2e DP tests only
mise run test:ci                           # CI unit tests with coverage (excludes slow, e2e, gpu, smoke)
mise run test:ci-slow                      # CI slow tests with coverage
mise run test:ci-container                 # CI tests in a Linux container (Docker/Podman)

Run a single test:

uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0

Test runner: uv run --frozen pytest -n auto --dist loadscope -vv...

Files:

• tests/llm/test_metadata.py

tests/**/*.py

📄 CodeRabbit inference engine (tests/TESTING.md)

tests/**/*.py: Auto-mark tests based on file path: tests under /e2e/ get e2e marker, tests under /smoke/ get smoke marker, all others get unit marker (only if no category marker already present)
Every test should have exactly one category marker: unit, smoke, or e2e
Use pytest.mark.requires_gpu modifier on tests that need CUDA hardware
Use pytest.mark.vllm on tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Use pytest.mark.slow on long-running tests
Use pytest.mark.smollm2 for SmolLM2 Hub download tests to enable process isolation
Use pytest.mark.noautouse to skip autouse fixtures for specific tests
Use load_test_dataset(filename) helper to load test datasets from tests/stub_datasets/ as HuggingFace Dataset objects
Use load_test_dataframe(filename) helper to load test data files from tests/stub_datasets/ as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(), pd.BooleanDtype()) before assigning np.nan values
Use fake.seed_instance(seed) and random.seed(seed) together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them in conftest.py and import them using relative imports (e.g., from .conftest import train_with_sdk); note that importing from other test files like tests/cli/helpers.py does not work
Use fixture_mock_processor or fixture_mock_processor_without_valid_records for mocking ParsedResponse objects with valid_records, invalid_records, errors, and prompt_number fields
Use pytest.importorskip to gate tests on optional dependencies that require specific extras (e.g., sentence_transformers, vllm)
Run vLLM tests with separate pytest invocations (one per file) using -n 0 (single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruff T201 is suppressed for tests/ directory) and should...

Files:

• tests/llm/test_metadata.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/llm/test_metadata.py

🧠 Learnings (1)

📚 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/llm/test_metadata.py

🪛 Ruff (0.15.20)

src/nemo_safe_synthesizer/llm/metadata.py

[error] 920-920: Possible hardcoded password assigned to: "bos_token"

(S105)

---

[error] 950-950: Possible hardcoded password assigned to: "bos_token"

(S105)

tests/llm/test_metadata.py

[error] 131-131: Possible hardcoded password assigned to argument: "expected_bos_token"

(S106)

---

[error] 817-817: Possible hardcoded password assigned to: "unk_token"

(S105)

🔇 Additional comments (3)

tests/llm/test_metadata.py (2)

1074-1088: 🎯 Functional Correctness | ⚡ Quick win

Also cover the rope_scaling_factor entry point for unsupported families.

This parametrization only passes the raw rope_scaling=2 kwarg, but the user-facing config path forwards training.rope_scaling_factor into these constructors, exercising _rope_scaling_from_factor. Parametrize the kwargs (e.g. add {"rope_scaling_factor": 2}) so a regression in that branch is caught too.

Source: Path instructions

---

805-823: LGTM!

Also applies to: 1039-1056, 1074-1088

src/nemo_safe_synthesizer/llm/metadata.py (1)

371-452: LGTM!

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Suppress Ruff S105 for the bos_token ClassVar literals.

bos_token = "<|im_start|>" (here and Line 950) trips Ruff S105 (hardcoded-password) even though it is a tokenizer special token, failing mise run check. Add a targeted # noqa: S105 or a narrow per-file ignore.

🧰 Tools

🪛 Ruff (0.15.20)

[error] 920-920: Possible hardcoded password assigned to: "bos_token"

(S105)

Source: Linters/SAST tools

[kendrickb-nvidia]

suggestion: Can we better distinguish these class variables which act as default values from the actual instance values in the prompt_config field? I'm worried we'll use these top-level values with the names we expect, and not the prompt_config.template instance value and similar and lead to hard to find bugs where our overrides aren't picked up.

Prefix with default_? Indicate internal only with _ or _default prefix? Docstrings so if someone does read the docstring it tells them not to use these class vars?

[kendrickb-nvidia]

Suggested change

	Centralises the shared HuggingFace loading path for declarative
	Centralizes the shared HuggingFace loading path for declarative

[kendrickb-nvidia]

question: What does having this class in the hierarchy provide us?