Skip to content

chore: final code quality cleanup #521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mckornfield merged 1 commit into from
May 27, 2026
Merged

chore: final code quality cleanup #521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/nemo_safe_synthesizer/data_processing/actions/dates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ def fit_and_transform_dates(
``transform_dates`` for reversal.
"""
date_min_dict = {}
object_cols = [col for col, col_type in df.dtypes.iteritems() if col_type == "object"]
object_cols = [col for col, col_type in df.dtypes.items() if col_type == "object"]
result_df = df.copy() if not inplace else df
for object_col in object_cols:
no_nans = result_df[object_col].dropna(axis=0).reset_index(drop=True)
Expand All @@ -425,7 +425,7 @@ def fit_and_transform_dates(
if inferred_format:
try:
inferred_format = inferred_format.replace("!", "")
dates = pd.to_datetime(result_df.loc[:, object_col], format=inferred_format)
dates = pd.to_datetime(result_df[object_col], format=inferred_format)
min_date = dates.min()
result_df[object_col] = (dates - min_date).dt.total_seconds()
date_min_dict[object_col] = {
Expand Down
1 change: 0 additions & 1 deletion src/nemo_safe_synthesizer/evaluation/components/text_semantic_similarity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,6 @@ def _init_sentence_transformer_model() -> SentenceTransformer | None:
return SentenceTransformer("distiluse-base-multilingual-cased-v2")
except RetryError:
return None
return None

@staticmethod
def _get_embedding_vectors(
Expand Down
2 changes: 1 addition & 1 deletion src/nemo_safe_synthesizer/pii_replacer/ner/labels.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def any_label_configured(self, labels: list[str]) -> bool:
def _matches_any_regex(self, label: str):
return any(regex.match(label) for regex in self._label_regexes)

def explicit_lables(self) -> set[str]:
def explicit_labels(self) -> set[str]:
return self._explicit_labels
Comment on lines +70 to 71

@coderabbitai coderabbitai Bot May 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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."

@binaryaaron binaryaaron May 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit hilarious

@coderabbitai coderabbitai Bot May 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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 Entity enum whose member keys are used as built-in labels):
class Entity(Enum):
    ABA_ROUTING_NUMBER = ( ... )
    AGE = ( ... )
    ...
    DOMAIN_NAME = ( ... )
    EMAIL_ADDRESS = ( ... )
    ...
    # (many more enum members)


@classmethod
Expand Down
1 change: 0 additions & 1 deletion tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,6 @@ def load_test_dataframe(filename: str, datasets_dir: Path) -> pd.DataFrame:

case _:
raise ValueError(f"Unknown dataset format: {dataset_path.suffix}")
raise AssertionError("unreachable")


@pytest.fixture(scope="session")
Expand Down
6 changes: 5 additions & 1 deletion tools/codestyle/copyright_fixer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,11 @@ def _rel(filepath: str) -> str:
return filepath

if check:
missing = [f for f in files if _read_head(f) and not _has_header(_read_head(f))]
missing: list[str] = []
for f in files:
head = _read_head(f)
if head and not _has_header(head):
missing.append(f)
if missing:
typer.echo(f"Found {len(missing)} file(s) missing copyright headers:")
for f in missing:
Expand Down
Loading