chore: final code quality cleanup - #521
Conversation
Signed-off-by: mkornfield <mkornfield@nvidia.com>
WalkthroughMulti-file maintenance PR with five independent changes: Pandas API deprecation fixes in date processing, code formatting cleanup in model initialization, method name typo correction in label evaluator, error handling improvement in test fixtures, and performance refactoring in copyright checking utilities. ChangesMaintenance and cleanup fixes across core modules
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
test
Greptile SummaryThis PR removes dead code and fixes minor code quality issues across five files with no functional changes to the production path.
Confidence Score: 5/5Safe to merge — all five changes are mechanical cleanups with no functional impact on production paths. Every change either removes provably unreachable code, fixes a deprecated API call, or corrects a typo in an uncalled method. The pandas iteritems() → items() migration is the most impactful change and it is straightforwardly correct. No logic is altered, no new paths are introduced, and the typo-fixed method has no external callers. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[fit_and_transform_dates] --> B["df.dtypes.items() ✅\n(was iteritems)"]
B --> C["result_df[col] ✅\n(was .loc[:, col])"]
D[_init_sentence_transformer_model] --> E{Retrying loop}
E -- success --> F[return SentenceTransformer]
E -- RetryError --> G[return None]
H["❌ dead return None removed"]
I[load_test_dataframe] --> J{match suffix}
J -- .csv/.parquet/.json/.jsonl --> K[return DataFrame]
J -- wildcard case --> L[raise ValueError]
M["❌ dead raise AssertionError removed"]
N[update_license_headers check mode] --> O[for f in files]
O --> P["head = _read_head(f)\n(called once ✅)"]
P -- missing header --> Q[missing.append]
Reviews (1): Last reviewed commit: "chore: final code quality cleanup" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00c240c3-3e4d-4af5-a281-effa76bc462a
📒 Files selected for processing (5)
src/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pytests/conftest.pytools/codestyle/copyright_fixer.py
💤 Files with no reviewable changes (2)
- src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.py
- tests/conftest.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). (6)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Smoke Tests
- GitHub Check: Analyze (Python)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
src/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pytools/codestyle/copyright_fixer.py
**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.py: Use American English spelling: 'initialize' not 'initialise', 'recognize' not 'recognise', 'color' not 'colour'.
Useobservability.get_logger(__name__)for logging, 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={}in logging for structured data that downstream tools should query or aggregate; use f-strings for human-readable context.
Raise from the custom error hierarchy with dual inheritance:SafeSynthesizerError(base),UserError,DataError,ParameterError,GenerationError,InternalError.
UseNSSBaseModelfor config/parameter models inconfig/which define user-facing configuration. Use rawBaseModelor module-specific bases for data transfer objects and internal structures.
UseBaseSettingsfor env/CLI settings. PreferAliasChoiceson individual fields when a field needs to respond to both its Python name and an env var name.
IncludeField(description=...)for Pydantic model fields as the canonical field docstring for API documentation and CLI help text.
Use assignment-styletype = Field(default=..., description="...")as the default for Pydantic model fields because type checkers understanddefault,default_factory, andaliasin assignment style.
UseAnnotatedonly when the field carries additional metadata beyondField()--ValueValidator,AutoParam,DependsOnValidator, reusable constrained type aliases, nested-type constraints, or discriminated unions.
Put defaults as bare assignment (= value), not insideField(default=...), when usingAnnotated. Exception: use assignment-styleField(default_factory=...)for defaults that cannot be expressed as bare assignments.
Use@dataclass(frozen=True)for immutable value objects and validators; mu...
Files:
src/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pytools/codestyle/copyright_fixer.py
src/**/*.py
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
src/**/*.py: Use relative imports insrc/(e.g.,from ..observability import get_logger).
Do not useprint()statements in library code. Useget_logger(__name__)fromobservability.pyorclick.echo()for CLI.
Do not useassertfor validation in library code. Useif/raisefor input validation.assertstatements can be stripped by-Oand must never guard correctness.
Files:
src/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.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/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.py
**/*.{py,sh,yaml,yml}
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Include SPDX copyright header at the top:
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.and# SPDX-License-Identifier: Apache-2.0. Themake formatcommand handles this automatically.
Files:
src/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pytools/codestyle/copyright_fixer.py
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Include a newline at the end of all files, never trailing whitespace. This is enforced bypre-commit.
Use line length of 120 characters for code, comments, and docstrings (configured inruff.toml).
Files:
src/nemo_safe_synthesizer/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pytools/codestyle/copyright_fixer.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/data_processing/actions/dates.pysrc/nemo_safe_synthesizer/pii_replacer/ner/labels.pytools/codestyle/copyright_fixer.py
src/nemo_safe_synthesizer/data_processing/**/*.py
⚙️ CodeRabbit configuration file
Review for data-contract regressions. Check input/training/test/synthetic naming, group boundaries, token-budget math, record ordering, schema and column validation, nullable dtypes, and deterministic behavior.
Files:
src/nemo_safe_synthesizer/data_processing/actions/dates.py
src/nemo_safe_synthesizer/pii_replacer/**/*.py
⚙️ CodeRabbit configuration file
Treat PII replacement changes as high-risk. Check entity coverage, replacement determinism, leakage of original values, handling of empty or multilingual text, and compatibility with optional dependencies.
Files:
src/nemo_safe_synthesizer/pii_replacer/ner/labels.py
tools/**
⚙️ CodeRabbit configuration file
Review tools as developer and CI infrastructure. Check that scripts use uv or Makefile wrappers instead of ad hoc python/pip commands, preserve read-only behavior for check targets, fail with clear messages, avoid hidden network or filesystem side effects, and stay consistent with STYLE_GUIDE.md and CONTRIBUTING.md. Tooling may use print() when it is a standalone script or intentional CLI output.
Files:
tools/codestyle/copyright_fixer.py
tools/codestyle/**
⚙️ CodeRabbit configuration file
Treat codestyle wrappers as CI-critical. Check consistency with Makefile targets, ruff.toml, ty configuration, copyright handling, staged-file behavior, read-only check modes, and whether fixes mutate only expected files.
Files:
tools/codestyle/copyright_fixer.py
🔇 Additional comments (2)
src/nemo_safe_synthesizer/data_processing/actions/dates.py (1)
419-419: LGTM!Also applies to: 428-428
tools/codestyle/copyright_fixer.py (1)
286-290: LGTM!
| def explicit_labels(self) -> set[str]: | ||
| return self._explicit_labels |
There was a problem hiding this comment.
Preserve backward compatibility for the renamed public accessor.
Line 70 introduces a public API rename (explicit_lables → explicit_labels) without a compatibility shim. That can break external callers immediately. Keep a deprecated alias that forwards to explicit_labels() for at least one release cycle.
Suggested minimal compatibility patch
class LabelEvaluator:
@@
def explicit_labels(self) -> set[str]:
return self._explicit_labels
+
+ def explicit_lables(self) -> set[str]:
+ """Backward-compatible alias for typoed accessor."""
+ return self.explicit_labels()As per coding guidelines, "When changing APIs (even small accessor renames), update docstrings/tests accordingly and keep behavior consistent with the error hierarchy guidance."
There was a problem hiding this comment.
this is a bit hilarious
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve the review comment on this PR
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOO0bYNs6OX5Wo"}) {
thread {
isResolved
}
}
}'Length of output: 6057
🏁 Script executed:
#!/bin/bash
# List review threads on PR `#521` to find the one for explicit_lables
gh api graphql -f query='
query {
repository(owner: "NVIDIA-NeMo", name: "Safe-Synthesizer") {
pullRequest(number: 521) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
body
author { login }
}
}
}
}
}
}
}'Length of output: 735
🏁 Script executed:
#!/bin/bash
# Use REST API to find coderabbitai's review comments on PR `#521`
gh api repos/NVIDIA-NeMo/Safe-Synthesizer/pulls/521/comments \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:100]}'Length of output: 560
@binaryaaron Yeah, keeping a deprecated alias for a typo'd name would just bless the bug in callers — completely agreed. The suggestion was misplaced here.
The GitHub API isn't reachable from the sandbox right now, so I can't resolve the comment programmatically. You can dismiss it manually, or just reply @coderabbitai resolve as a top-level PR comment and I'll take care of it.
(ᵔᴥᵔ)🐇 hops away from its own bad suggestion
🧬 Code Graph Analysis Results
Relevant snippet(s)
src/nemo_safe_synthesizer/pii_replacer/ner/labels.py
- Lines ~1–50 (built-in label ordering and normalization):
def get_built_in_labels() -> list[str]:
labels = [normalize_label(key) for key in Entity.__members__.keys()]
# FIXME(pm): This is hacky way to prioritize email_address transform, so that
# domain_name transform is later on the list
labels.remove("email_address")
return ["email_address"] + labels- Lines ~52–108 (user-configured label filtering logic):
class LabelEvaluator:
"""
Evaluates labels specified by the user in the config and provides a simple
interface other places in the code that use that label configuration.
One notable example is expanding wildcards from the label config (e.g. ``acme/*`` or ``*``).
"""
def __init__(self, explicit_labels: set[str], label_regexes: list[re.Pattern]):
self._explicit_labels = normalize_labels(explicit_labels)
self._label_regexes = label_regexes
def filter_labels(self, labels: list[str]) -> Iterator[str]:
"""
Filters provided list of labels against configured labels and label regexes.
Example::
evaluator = LabelEvaluator(explicit_labels=["test"], label_regexes=["^acme/.*$"])
filtered = evaluator.filter_labels(["test", "test_2", "acme/abc", "test/test"])
assert list(filtered) == "test", "acme/abc"
Args:
labels: List of labels to be filtered.
Returns: Filtered labels as they are calculated.
"""
# check explicit labels
for label in labels:
if normalize_label(label) in self._explicit_labels:
yield label
# check wildcard labels
if self._label_regexes:
for label in labels:
if self._matches_any_regex(label):
yield label
def any_label_configured(self, labels: list[str]) -> bool:
# checks if there is any item in the filtered list
return next(self.filter_labels(labels), None) is not None
def _matches_any_regex(self, label: str):
return any(regex.match(label) for regex in self._label_regexes)
def explicit_labels(self) -> set[str]:
return self._explicit_labels- Lines ~110–155 (wildcard parsing in config; regex compilation; warning on invalid specs):
`@classmethod`
def create_from_config(cls, config_labels: list[str]) -> LabelEvaluator:
"""
Loads labels defined by the user in the config.
Args:
config_labels: Labels configured by the users.
"""
explicit_labels = set([])
label_regexes: list[re.Pattern] = []
for label in config_labels:
if "*" not in label:
explicit_labels.add(label)
else:
# there is a wildcard
parts = label.split("/")
if len(parts) == 2:
namespace, entity = parts
# match all labels inside a namespace
label_regexes.append(re.compile(rf"^{namespace}/.+$", re.IGNORECASE))
elif len(parts) == 1:
# match all labels that don't have namespace
label_regexes.append(re.compile(r"^[^/]+$", re.IGNORECASE))
else:
logger.warning(f"Invalid label specification '{label}'. Skipping.")
return cls(explicit_labels, label_regexes)Relevant supporting snippets
src/nemo_safe_synthesizer/data_processing/records/base.py
- Lines ~189–196 (label normalization used by
labels.py):
def normalize_labels(labels: Iterable[str]) -> set[str]:
"""Normalize labels by converting them to lowercase."""
return {normalize_label(label) for label in labels}
def normalize_label(label: str) -> str:
"""Convert a single label to lowercase."""
return label.lower()src/nemo_safe_synthesizer/observability.py
- Lines ~768–783 (logger behavior used for warnings in
create_from_config):
def get_logger(name: str | None = None) -> CategoryLogger:
"""Return a category logger for structured logging.
Always pass ``__name__`` as the argument. After
``initialize_observability()`` is called, returns a structlog-based
logger with full formatting. Before initialization (e.g. when imported
as a library), returns a basic stdlib logger that integrates with the
parent application's logging configuration.
"""
if _INITIALIZED_OBSERVABILITY:
return CategoryLogger(structlog.get_logger(name))
# Return basic stdlib logger when logging hasn't been initialized
# This allows the package to be used as a library without taking over
# the parent application's logging configuration
return CategoryLogger(logging.getLogger(name))src/nemo_safe_synthesizer/pii_replacer/ner/entity.py
- Lines ~26–187 (the
Entityenum whose member keys are used as built-in labels):
class Entity(Enum):
ABA_ROUTING_NUMBER = ( ... )
AGE = ( ... )
...
DOMAIN_NAME = ( ... )
EMAIL_ADDRESS = ( ... )
...
# (many more enum members)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
binaryaaron
left a comment
binaryaaron
left a comment
There was a problem hiding this comment.
i doubt it'll be the final one
|
final forever times a million |
3 participants
Summary
Pre-Review Checklist
Ensure that the following pass:
make format && make checkor via prek validation.make testpasses locallymake test-e2epasses locallymake test-ci-containerpasses locally (recommended)/syncon this PR to trigger a run (auto-triggers on ready-for-review)Pre-Merge Checklist
Other Notes
Summary by CodeRabbit
Release Notes
Bug Fixes
Performance