feat(ux): PII plan & result preview - #678
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
area:build-dist
area:sdk-cli
area:evaluation
area:pii
area:tests
area:docs
labels
223064d to
6ec7066
Compare
49c38bd to
baaf407
Compare
76052e1 to
cbea3e1
Compare
d4d85dd to
5b7b975
Compare
46ae96b to
2e65768
Compare
7a35de1 to
76b1a7d
Compare
2e65768 to
cd9ddc7
Compare
bb2cfbb to
ee88bb7
Compare
cd9ddc7 to
96938e9
Compare
20b06cd to
ec90e75
Compare
b1b3c97 to
b1dc3db
Compare
9c263ae to
542a5b9
Compare
cd980ef to
c457f43
Compare
2c092cf to
591c349
Compare
c457f43 to
8102024
Compare
Notebook widgets for reviewing/editing the replacement plan (YAML + diagram with section help) and browsing original vs replaced rows, wired through SafeSynthesizer so previewed plans persist and re-arm process_data when edited. Signed-off-by: nina-xu <19981858+nina-xu@users.noreply.github.com>
8102024 to
3c83b6d
Compare
binaryaaron
left a comment
binaryaaron
left a comment
There was a problem hiding this comment.
Agent-assisted review: generated by @binaryaaron's Agent and reviewed by @binaryaaron.
| import yaml | ||
|
|
||
| from nemo_safe_synthesizer.config.data import DataParameters | ||
| from nemo_safe_synthesizer.config.pii_replacement import ( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift | 👤 Author review required
Repair stale stack symbols before further preview review.
PR 678 does not collect against the current PR 672 head. It still imports config.pii_replacement, constructs ReplacePiiConfig, and uses the removed gender persona attribute. The focused stack check reports ModuleNotFoundError and NameError. Please rebase and update these symbols before further preview review.
| logger.runtime.info(f"Wrote previewed PII replacement plan to {plan_path}") | ||
|
|
||
| if self._data_processed and updated_plan != self._applied_pii_plan: | ||
| self._data_processed = False |
There was a problem hiding this comment.
🎯 Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift | 👤 Author review required
Invalidate processed data after plan edits.
_apply_previewed_plan() changes the plan and clears only _data_processed. train() ignores that flag and can consume the old _training_df, so it can train on data produced by a different plan. Please invalidate the complete processing result and make train() require a current committed result.
| cfg = config_from_replace_pii(self._nss_config.replace_pii) | ||
| plan = resolve_plan( | ||
| self._nss_config.replace_pii, | ||
| self._data_source, |
There was a problem hiding this comment.
🎯 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift | 🧭 Human decision required
Preview the training split and require explicit adoption.
Preview discovery runs against _data_source, while process_data() runs replacement after the holdout split. The preview can therefore inspect test rows and produce a plan that differs from the training operation. Widget construction also adopts the plan at once. Please use the exact training split and require an explicit accept action before config or files change.
| ] | ||
|
|
||
| [project.optional-dependencies] | ||
| notebook = [ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win | 🤖 Agent-fix candidate
Make the notebook extra sufficient for its documented import.
The tutorial asks users to install nemo-safe-synthesizer[notebook], which installs anywidget and ipywidgets without datasets. Importing SafeSynthesizer imports datasets at module load, so that documented setup cannot start. Please make the notebook extra sufficient for the documented import and add a clean-install contract test.
| original_row = original_df.loc[[idx], cols] | ||
| transformed_row = transformed_aligned.loc[[idx], cols] | ||
| pair = pd.concat([original_row, transformed_row], axis=0) | ||
| pair.index = pd.MultiIndex.from_arrays( |
There was a problem hiding this comment.
🎯 Stability & Availability | 🟡 Minor | ⚡ Quick win | 🤖 Agent-fix candidate
Preserve row identity in result preview.
Result preview reindexes by labels and then assigns a two-element MultiIndex. Duplicate input indexes can produce extra rows and raise ValueError: Length mismatch. Please use stable positional row identity in the comparison frame and add a duplicate-index regression.
| self._replace_pii_config = cfg | ||
| self._nss_config = self._nss_config.model_copy(update={"replace_pii": cfg}) | ||
|
|
||
| plan_path = save_plan_to_path(updated_plan, self._workdir.run_dir / PII_REPLACEMENT_PLAN_FILENAME) |
There was a problem hiding this comment.
🎯 Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift | 👤 Author review required
Make plan adoption atomic.
_apply_previewed_plan() mutates both config fields before it writes the plan file. If persistence fails, memory contains an adopted plan that storage does not contain. Please validate and persist the draft first, then commit config and invalidate derived processing state as one SDK operation. Add a failing-write regression that proves the prior state remains intact.
| pairs: list[tuple[str, str]] = [] | ||
| seen: set[tuple[str, str]] = set() | ||
| for ent in free_text_entities: | ||
| if ent.get("column") != column: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | 🏗️ Heavy lift | 👤 Author review required
Preserve scope in free-text highlighting.
_free_text_pairs_for_column() keeps only the column when it selects replacements. The renderer then highlights those pairs in every displayed row, even when the replacement belongs to one record or group. Please retain scope and row identity in the render input and add a two-row scoped example.
| - [Safe Synthesizer 101](safe-synthesizer-101.ipynb) -- learn the fundamentals | ||
| - [Differential Privacy](differential-privacy.ipynb) -- enable differential privacy guarantees | ||
| - [Time-Series Financial Transactions](time-series-financial-transactions.ipynb) -- synthesize grouped transaction histories | ||
| - [PII Replacement](pii-replacement.ipynb) -- optionally preview/edit a PII plan, then continue the pipeline |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win | 👤 Author review required
Make the tutorial match the public API and privacy model.
The tutorial hardcodes /root/datasets/patient_events.csv, reads private _training_df, configures managed personas while describing Faker, and shows original rows without a notebook-output warning. These examples give users a setup that does not match the public API or privacy model. After the stack repair, please use a portable data path and public result, align the backend prose with the code, and warn users before rendering or saving original PII.
2 participants
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