Skip to content

refactor: remove legacy DD sync model paths - #359

Merged
andreatnvidia merged 4 commits into
mainfrom
andreatnvidia/refactor/remove-sync-validation
Nov 3, 2026
Merged

andreatnvidia merged 4 commits into
mainfrom
andreatnvidia/refactor/remove-sync-validation

Conversation

@andreatnvidia

@andreatnvidia andreatnvidia commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

DataDesigner removed its sync engine in NVIDIA-NeMo/DataDesigner#767, but Anonymizer retained a duplicate sync detection path and four synchronous model calls in rewrite evaluation and repair. These calls now use await model.agenerate() directly on DD's async loop, with trace attribution preserved across awaits.

Related Issue

No linked issue required: maintainer-owned cleanup following DataDesigner's sync engine removal.

Plan Document

No plan required: internal engine migration to existing DD async execution, preserving the public API and pipeline logic.

Summary

  • Remove detection's thread-pool validation path, sync bridge, unused custom-column factory, and obsolete sync-vs-async benchmark. Direct sync calls to the chunked-validation plugin fail immediately with a clear error.
  • Convert quality re-answer, privacy re-answer, quality comparison, and repair generators to async.
  • Await model-backed generators in the combined rewrite graph and preserve column trace context through async execution.
  • Cover all four standalone and combined generators through DD's real dispatcher, timeout/cancellation propagation, and sync/async custom-column trace attribution.
  • Rely on DD's model admission and per-request HTTP timeouts; remove the bridge's additional thread-pool ceiling and whole-call watchdog.

Type of Change

  • Refactoring

Contributor Checklist

  • Conventional PR title and maintainer-owned no-issue reason documented
  • No-plan reason documented
  • Public API impact checked; bundled skill update not needed
  • No real PII, secrets, or private endpoints added

Validation

  • TMPDIR=/private/tmp .venv/bin/pytest -q --tb=short --basetemp=/private/tmp/a359-pytest-short: 1,605 passed. The short temporary path avoids macOS Unix-socket path constraints.
  • Compared pre-conversion base fea5a60 with this PR through a scripted HTTP provider: six scenarios in both standalone and combined rewrite modes (12 paired comparisons). Complete public outputs, internal trace dataframes, request counts, failed records, correction attempts, and trace column coverage matched.
  • Scenarios: normal rewrite with repair and no-entity passthrough; malformed responses in all four converted calls; transient HTTP 429; permanent malformed-response failure; validator-pool failover; 40 long documents under concurrent load. Measured in-flight requests stayed within the configured cap of two per model.
  • Live evaluator/repair smoke: two runs over three records, all four converted columns exercised, 12 successful async model-call traces, no failed records.
  • Fresh independent Claude review of 373b5bb: no actionable findings.
  • .venv/bin/ruff check --fix . && .venv/bin/ruff format .
  • VIRTUAL_ENV="$PWD/.venv" .venv/bin/ty check --error-on-warning
  • .venv/bin/python tools/codestyle/copyright_fixer.py --check .
  • uv lock --check and git diff --check

Successful async traces were preserved; the old failed synchronous-probe traces disappeared. Full detection/rewrite end-to-end comparisons used the scripted provider; live coverage focused on evaluation and repair.

The format, copyright, and aggregate commit-hook wrappers were skipped after running their equivalent checks directly because those wrappers invoke uv run. Remaining applicable hooks and DCO signoff passed.

Documentation and Artifacts

  • Internal docstrings and plan references updated; no public documentation or notebook changes needed
  • Relevant validation for benchmark removal and model-call changes listed above

Signed-off-by: Andre Manoel <amanoel@nvidia.com>
@andreatnvidia
andreatnvidia requested review from a team as code owners November 3, 2026 16:12
@greptile-apps

greptile-apps Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issues were found.

Summary

The PR removes legacy synchronous detection paths and changes rewrite evaluation and repair to await model calls.

  • Entity validation runs through the async entry point.
  • Rewrite evaluation and repair await their model results.
  • Async custom-column model calls keep their column trace label.
  • The sync-versus-async detection benchmark is gone.

No new actionable issues were found in the latest changes.

Reviews (4) · Last reviewed commit: "test(rewrite): cover async failures for ..." · Reviewed by Greptile

Signed-off-by: Andre Manoel <amanoel@nvidia.com>
Signed-off-by: Andre Manoel <amanoel@nvidia.com>
@andreatnvidia andreatnvidia changed the title refactor(detection): remove legacy DD sync validation path refactor: remove legacy DD sync model paths Nov 3, 2026
Signed-off-by: Andre Manoel <amanoel@nvidia.com>

@lipikaramaswamy lipikaramaswamy left a comment

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.

Approved after reviewing the current head, 373b5bb312ae22f48a9eaa6281a9f5c56eb37c82.

Review

No actionable correctness issues found. The chunked-validation plugin now uses the existing async path exclusively and rejects direct synchronous calls before model dispatch. The additional rewrite changes consistently await the quality re-answer, privacy re-answer, quality comparison, and repair model calls; the combined graph awaits these generators, and trace context remains active across awaits.

Validation on the current head

Independently ran:

PYTHONPATH=<isolated PR source> python -m pytest tests/engine tests/interface tests/test_measurement.py -q --tb=short

Result: 1,101 passed. Also reviewed the new standalone/combined dispatcher coverage, timeout and cancellation propagation tests, and sync/async trace-attribution coverage. git diff --check passed for the follow-up changes.

Live integration evidence

A full public Anonymizer.preview() rewrite workflow passed on the earlier revision 9e103e3e33020a32ffe62dc0e8f4d87689c18242, using live GLiNER2 and the default OpenRouter model selections:

  • Two entirely fabricated input records, both returned; zero failed records.
  • Validation limited to two candidates per chunk: five chunks started and completed, with five concurrently active; zero synchronous chunked-validation plugin calls.
  • The rewrite workflow completed in 125.31 seconds, produced the expected rewrite and metric columns, and preserved the no-PII record unchanged.
  • Evaluation passed at iteration 0, so this live run did not exercise repair.

This live evidence covers the earlier detection cleanup; the subsequent rewrite/trace changes were reviewed and tested on the current head as described above, but were not rerun against live providers.

Merge coordination

Please coordinate with #339 and #305 (and its dependent #265): they modify the removed sync validation code/tests, and #305 adds an augmentation sync entry point using the deleted bridge. Keep their intended behavior in the async paths when resolving overlaps.

@andreatnvidia
andreatnvidia merged commit aca29fc into main Nov 3, 2026
20 checks passed
@andreatnvidia
andreatnvidia deleted the andreatnvidia/refactor/remove-sync-validation branch November 3, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants