chore: revive semantic similarity test - #593
Conversation
WalkthroughDefers SentenceTransformer import to runtime initialization with TYPE_CHECKING for static types, stubs TextSemanticSimilarity in multimodal report tests to avoid dependency, and adds e2e assertions that the evaluation report HTML includes “Synthetic Quality Score” and “Text Semantic Similarity”. ChangesLazy-loaded TextSemanticSimilarity and test adaptation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR makes
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to making an optional dependency truly optional, with no effect on core pipeline behavior when the package is present. The source change is additive and backward-compatible: when sentence_transformers is installed everything behaves exactly as before, and when it is absent the code now degrades gracefully instead of crashing at import time. The test changes replace a coarse importorskip guard with a precise monkeypatch stub, improving isolation without weakening coverage. No logic paths are removed or bypassed. No files require special attention. Important Files Changed
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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)
src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.py (1)
243-257:⚠️ Potential issue | 🟠 MajorFail fast when
sentence_transformersis unavailable instead of retrying the import.In
TextSemanticSimilarity._init_sentence_transformer_model(),from sentence_transformers import SentenceTransformerruns inside theRetrying(...)block (no customretry=filter). Tenacity’s default retries on anyException, so a missing dependency (ImportError) backs off up to ~367s before returningNone; the caller then raises whenstm is None, meaning text semantic similarity reporting can stall and then fail for the component (and it’s called once pertext_fieldsentry).Smallest fix: import
SentenceTransformeroutside the retry loop (or configureRetrying(retry=...)to excludeImportError) so only model construction/loading failures are retried.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e3823f7d-7de7-49e2-b361-4fbcb53aa7ef
📒 Files selected for processing (3)
src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pytests/e2e/test_safe_synthesizer.pytests/evaluation/reports/test_multimodal_report.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Smoke Tests
- GitHub Check: Typecheck
- GitHub Check: Analyze (Python)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{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/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pytests/evaluation/reports/test_multimodal_report.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: Python source files must use ruff for formatting and linting. Run mise run format to auto-fix formatting and linting issues, and mise run check for read-only quality checks
Use ty for Python type checking. Run mise run check to execute type checking on all tracked files
**/*.py: UseField(description=...)as the canonical field docstring for Pydantic models.
Use assignment style (type = Field(default=..., description="...")) as the default for Pydantic model fields rather thanAnnotated-only patterns.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Use@dataclass(frozen=True)for immutable value objects and validators. Mutable@dataclassacceptable for builders, accumulators, and pipeline state.
Usefield(default_factory=list)for mutable defaults in dataclasses, never= [].
UseStrEnumfor string-valued enums used in configs/serialization. PlainEnumfor internal-only named constants.
UseX | YnotOptional[X]orUnion[X, Y]in type hints.
Uselist[str]notList[str],dict[str, int]notDict[str, int]in type hints.
UseSelffor fluent method returns in type hints.
Use collection ABCs for function arguments (Sequence,Mapping,Iterable) so callers can pass any compatible container; concrete types for return values.
UseProtocolfor structural subtyping when you need duck-typing boundaries.
AvoidAnyin type hints -- preferobject, generics, orProtocol.
UseTYPE_CHECKINGguards for heavy imports (pandas,torch,transformers); not needed for stdlib or lightweight imports.
Prefer `m...
Files:
tests/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pytests/evaluation/reports/test_multimodal_report.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/e2e/test_safe_synthesizer.pytests/evaluation/reports/test_multimodal_report.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/e2e/test_safe_synthesizer.pytests/evaluation/reports/test_multimodal_report.py
⚙️ CodeRabbit configuration file
tests/**:Testing Guide
Comprehensive testing reference for Safe-Synthesizer developers. Covers commands, markers, test data, fixtures, and gotchas.
Read First
tests/conftest.py-- auto-marking,load_test_dataset/load_test_dataframe,fixture_mock_processorpatternpytest.ini-- markers, asyncio, timeouttests/evaluation/conftest.py-- most complex: Faker-basedmake_df, nullable dtype conversiontests/generation/conftest.py-- JSONL/schema fixtures,fixture_valid_iris_dataset_jsonl_and_schemaRunning 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 -n0Test runner:
uv run --frozen pytest -n auto --dist loadscope -vv...
Files:
tests/e2e/test_safe_synthesizer.pytests/evaluation/reports/test_multimodal_report.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (.py, .sh, .yaml, .yml, .md) must include SPDX copyright headers. Use mise run format to add them automatically
Files:
tests/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pytests/evaluation/reports/test_multimodal_report.py
tests/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
tests/**/*.py: New features must include tests before submitting a PR
Bug fixes must include regression tests before submitting a PR
tests/**/*.py: Use file namingtest_*.py, class namingTest*, function namingtest_<module>_<expected_behavior>for test files.
Use function-scoped fixtures by default. Session scope only when empirically justified by test runtime.
Use bareassertas primary assertion style;pytest.raises()withmatch=for exceptions;pytest.approx()for floating-point comparisons.
Docstrings are optional for simple tests, recommended for complex/e2e tests explaining purpose.
Markers are auto-assigned by path viapytest_collection_modifyitems(/e2e/->e2e,/smoke/->smoke, default ->unit). Use explicit markers:@pytest.mark.slow,@pytest.mark.requires_gpu,@pytest.mark.timeout().
Usetmp_pathfixture for file operations in tests, never write to the repo tree.
Mark CUDA-dependent tests with@pytest.mark.e2e,@pytest.mark.smoke, or@pytest.mark.requires_gpu.
Mock only external boundaries in tests, not internal implementation details.
Ensure test isolation: no shared mutable state or execution-order dependencies between tests. Include required setup in the test or a fixture.
Use@pytest.mark.parametrizefor testing multiple input combinations rather than copy-pasting similar tests.
tests/**/*.py: Assign exactly one of three pytest category markers (unit, smoke, e2e) to every test function
Markers 'slow' and 'requires_gpu' modify the three category markers (unit, smoke, e2e) to indicate when tests should be run or if separate pytest invocations are required
Use 'pytest.mark.noautouse' to skip autouse fixtures for specific tests
Auto-marking via pytest_collection_modifyitems assigns category markers based on test file path: '/e2e/' → 'e2e', '/smoke/' → 'smoke', no match → 'unit', only if no category marker is already present
Use load_test_dataset(filename) from root conftest.py to load HuggingFace Dataset...
Files:
tests/e2e/test_safe_synthesizer.pytests/evaluation/reports/test_multimodal_report.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/e2e/test_safe_synthesizer.pytests/evaluation/reports/test_multimodal_report.py
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright headers in all source files using hash-comments for
.py,.sh,.yaml,.ymlfiles.
Files:
tests/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pytests/evaluation/reports/test_multimodal_report.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/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pytests/evaluation/reports/test_multimodal_report.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.mdfor 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
uvfor everything -- neverpipor rawpython. 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 intools/instead of runningruffortydirectly. Useuv runfor Python execution. When in doubt, inspectmise tasksandpytest --markers.The canonical
uv synccommand for a full GPU/dev environment is:uv sync --frozen --extra cu129 --extra engine --group devBare
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/e2e/test_safe_synthesizer.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pytests/evaluation/reports/test_multimodal_report.py
src/nemo_safe_synthesizer/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
src/nemo_safe_synthesizer/**/*.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
Write Python docstrings in Google style format so they auto-generate into API reference pages via mkdocstrings and gen-files plugins
Files:
src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Useobservability.get_logger(__name__)-- neverlogging.getLogger()orstructlog.get_logger()directly.
Use category loggers:.runtimefor internals,.userfor progress/results,.systemfor system events.
Never useprint()for operational output. Useclick.echo()for CLI output orsys.stdout.write()for raw output in tools.
Useextra={}for data that downstream tools should query or aggregate in log calls (metrics, counts, durations). f-strings are fine for human-readable context.
Raise from the custom error hierarchy with dual inheritance:SafeSynthesizerError(base),UserError,DataError,ParameterError,GenerationError,InternalError.
Prefer clamping/saturation over raising when out-of-range inputs shouldn't crash the system -- return a bounded value with a log warning.
Error messages must precisely match the actual error condition. Use!rfor repr of interpolated pieces to clearly identify them.
Use relative imports insrc/(from ..observability import get_logger), absolute imports intests/(from nemo_safe_synthesizer.observability import get_logger).
Usepathlib.Pathinstead ofos.path. Tolerateos.pathonly in vendored/tooling scripts.
Do not useprint()statements in library code. Useget_logger(__name__)fromobservability.pyorclick.echo()for CLI.print()is fine in tests, standalone scripts, and tooling.
Do not useassertfor validation in library code. Useif/raisefor input validation.assertis fine in tests wherepytestrelies on it.
Files:
src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.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/evaluation/components/text_semantic_similarity.py
src/nemo_safe_synthesizer/evaluation/**/*.py
⚙️ CodeRabbit configuration file
Treat evaluation changes as correctness-sensitive. Check metric inputs, holdout usage, privacy metric semantics, report data shape, missing-data handling, and whether unavailable metrics fail or degrade intentionally.
Files:
src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.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/e2e/test_safe_synthesizer.pytests/evaluation/reports/test_multimodal_report.py
🔇 Additional comments (1)
tests/e2e/test_safe_synthesizer.py (1)
43-47: LGTM!Also applies to: 75-75, 99-99
Signed-off-by: mkornfield <mkornfield@nvidia.com>
e6af7d3 to
e5852f7
Compare
Summary
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
Tests
Refactor