fix(safe-synthesizer): fail fast with a clear error on non-tabular input#860
Conversation
A job whose fileset has nested list/dict columns (e.g. function-calling JSONL like xlam-function-calling-60k) crashed deep inside the library's preflight with an opaque `preflight.check_crash: TypeError: unhashable type: 'list'` — the ConstantColumnCheck calls `nunique()`, which cannot hash list-valued cells. Validate the loaded DataFrame in `run_config` before handing it to the SafeSynthesizer and raise a `ParameterError` naming the offending columns and explaining that flat tabular data is required. Safe Synthesizer is a tabular synthesizer, so nested columns are never valid input. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: mschwab <[email protected]>
|
📝 WalkthroughWalkthroughSafe Synthesizer validates loaded DataFrames for nested values in object columns before synthesis. Invalid columns raise ChangesFlat tabular validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/nemo-safe-synthesizer/tests/unit/test_local_run.py (1)
71-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
run_config()integration path.These tests call the private validator directly, so they would still pass if the new call at
__main__.pyLine 321 were removed. Add a regression test that passes nested data throughrun_config()and verifiesParameterErroris raised before Safe Synthesizer runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-safe-synthesizer/tests/unit/test_local_run.py` around lines 71 - 91, Extend the tests around _validate_flat_tabular_data with a run_config() integration regression test that supplies nested tabular data, asserts task_main.ParameterError, and verifies Safe Synthesizer is not invoked. Exercise the public run_config() path rather than calling the private validator directly, using the existing test helpers and mocking conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/nemo-safe-synthesizer/tests/unit/test_local_run.py`:
- Around line 71-91: Extend the tests around _validate_flat_tabular_data with a
run_config() integration regression test that supplies nested tabular data,
asserts task_main.ParameterError, and verifies Safe Synthesizer is not invoked.
Exercise the public run_config() path rather than calling the private validator
directly, using the existing test helpers and mocking conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0228e1fb-88a0-4c7c-8e5e-30ec190b9792
📒 Files selected for processing (2)
plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/tasks/safe_synthesizer/__main__.pyplugins/nemo-safe-synthesizer/tests/unit/test_local_run.py
Summary
A Safe Synthesizer job whose input fileset has nested list/dict columns (e.g. function-calling JSONL like
xlam-function-calling-60k) crashed deep inside the library's preflight with an opaque, unactionable error:Root cause: the library's
ConstantColumnCheckrunsdata[col].dropna().nunique(), which can't hash list-valued cells. Safe Synthesizer is a tabular synthesizer, so nested columns are never valid input — but nothing told the user that.Change
Validate the loaded DataFrame in
run_config()(the common chokepoint for both platform and local runs) before handing it toSafeSynthesizer, and raise aParameterErrorthat names the offending columns and explains the requirement:Only
object-dtype columns are scanned (numeric/bool are skipped), so the check is cheap.Notes
ParameterErrorso it surfaces the same way as other bad-input errors.ConstantColumnCheckcrash is a latent bug innemo_safe_synthesizer0.1.7 (it should guard unhashable columns like its sibling checks do) — worth a separate upstream report.tydiagnostics in this file (lines 220/416/440) are untouched by this change and present onmain.Test
test_validate_flat_tabular_data_rejects_nested_columns(raises naming the columns) andtest_validate_flat_tabular_data_allows_flat_columns(flat data passes). Fulltest_local_run.pysuite green.🤖 Generated with Claude Code
Summary by CodeRabbit