Skip to content

fix: render degenerate judge histograms instead of raising (#905) - #919

Open
chethanuk wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
chethanuk:fix/issue-905-ship
Open

fix: render degenerate judge histograms instead of raising (#905)#919
chethanuk wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
chethanuk:fix/issue-905-ship

Conversation

@chethanuk

Copy link
Copy Markdown

Summary

DatasetProfilerResults.to_report() raises on four judge-profile shapes that model_validate() accepts, so a schema-valid analysis result cannot be rendered. All four are one crash family on one render path (create_rich_histogram_table and its two callers), so they're fixed together.

Related Issue

Fixes #905
Closes #903

Changes

  • create_rich_histogram_table (config/utils/visualization.py): max(data.values()) raised ValueError on an empty mapping, one line before the existing max_count <= 0 guard that already handled "nothing to draw". Changed to max(data.values(), default=0), and an empty mapping now renders a no data placeholder row instead of an empty table.
  • create_judge_score_summary_table (config/analysis/utils/reporting.py): took a histogram by attribute (histogram.categories), so a MissingValue — which the field is typed to allow — raised AttributeError a frame earlier than the fix above. Parameter widened to CategoricalHistogramData | MissingValue; a MissingValue now builds an empty mapping instead.
  • JudgeScoreProfilerResults.create_report_section (config/analysis/column_profilers.py): indexed self.score_distributions.histograms[score_name] unguarded, while score_distributions is itself typed ... | MissingValue — raising AttributeError when missing and KeyError when a summary names a score the histogram map omits. Both now fall back to MissingValue.CALCULATION_FAILED, which the guard above renders.
  • Not touched: CategoricalHistogramData's list constraints. Adding min_length=1 would make CategoricalDistribution.from_series raise on an empty Series, which _load_stage_analysis catches and turns into None — trading a visible crash for a silently discarded analysis.

Testing

  • Two parametrized tests added, driven through the public to_report() entry point both reporters used. On unmodified main with the tests applied: 5 failed, 58 passedValueError at visualization.py:265, AttributeError at reporting.py:174, AttributeError and KeyError at column_profilers.py:146, matching both issues verbatim.
  • With the fix, same command: 63 passed.
  • uv run --group dev pytest packages/data-designer-config/tests652 passed (main: 644; the +8 is exactly the new cases).
  • uv run --group dev pytest packages/data-designer-engine/tests2257 passed.
  • make check-all-fix clean, tree unmodified.

Checklist

  • Follows commit message conventions
  • Commits are signed off (DCO)
  • Unit regression coverage added

…Mo#905)

DatasetProfilerResults.to_report() raised on four judge-profile shapes that
model_validate() accepts, so a schema-valid analysis result could not be
rendered. create_rich_histogram_table called max() on a bare mapping, so an
empty categorical histogram raised ValueError one line before the existing
max_count <= 0 guard. Its only caller passed a MissingValue straight through
to .categories, and create_report_section indexed
score_distributions.histograms[score_name] unguarded, so a missing
score_distributions or a summary naming a score the histogram map omits
raised AttributeError and KeyError.

Guard all four at the shared points every caller routes through: max() takes
default=0 and empty data renders a placeholder row, the histogram mapping is
built empty for a MissingValue with the parameter widened to
CategoricalHistogramData | MissingValue, and the per-score lookup falls back
to MissingValue.CALCULATION_FAILED.

CategoricalHistogramData's list constraints are left alone: adding
min_length=1 would make CategoricalDistribution.from_series raise on an empty
Series, which _load_stage_analysis catches and turns into None, trading a
visible crash for a silently discarded analysis.

Covered by parametrized tests driven through the public to_report() entry
point both reporters used; they fail on unmodified main with the four
exceptions above.

Fixes NVIDIA-NeMo#905
Closes NVIDIA-NeMo#903

Signed-off-by: ChethanUK <chethanuk@outlook.com>
@chethanuk
chethanuk requested a review from a team as a code owner September 5, 2026 04:51
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Linked Issue Check

Issue #905 has not been triaged yet. A maintainer needs to review
the issue and add the triaged label for this check to pass.

You can continue working on the PR in the meantime. The check will
re-run automatically once the issue is triaged.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the DCO ✍️ ✅
Posted by the DCO Assistant Lite bot.

@greptile-apps

greptile-apps Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the missing and empty histogram variants accepted by the schema now render without changing populated-histogram behavior.

Summary

  • Safely computes histogram scaling for empty mappings and displays a no-data placeholder.
  • Converts missing judge histogram values into empty renderable histogram data.
  • Falls back gracefully when the complete distribution or an individual score histogram is unavailable.
  • Adds regression coverage through the public report API and direct histogram rendering tests.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Judge score summary] --> B{Distribution available?}
    B -- No --> C[MissingValue fallback]
    B -- Yes --> D{Histogram available?}
    D -- No --> C
    D -- Yes --> E[Category-count mapping]
    C --> F[Empty mapping]
    E --> G[Histogram table]
    F --> H[No data row]
    G --> I[Rendered report section]
    H --> I
Loading

Reviews (2) · Last reviewed commit: "Merge branch 'main' into fix/issue-905-s..."

@chethanuk

Copy link
Copy Markdown
Author

I have read the DCO document and I hereby sign the DCO.

How do I sign it?

@nabinchha

Copy link
Copy Markdown
Contributor

I have read the DCO document and I hereby sign the DCO.

How do I sign it?

Instructions are here: #919 (comment)

@chethanuk

Copy link
Copy Markdown
Author

I have read the DCO document and I hereby sign the DCO.

@chethanuk

Copy link
Copy Markdown
Author

recheck

@nabinchha

Copy link
Copy Markdown
Contributor

Thanks for putting this together, @chethanuk!

Summary

This change makes judge-profile report generation tolerate the empty, missing, and incomplete histogram shapes accepted by the schema. The crash handling matches the PR's stated intent, but the new empty-state row needs one rendering correction before merge.

Findings

Warnings — Worth addressing

packages/data-designer-config/src/data_designer/config/utils/visualization.py:271 — Empty-state markup is rendered literally in saved reports

  • What: The placeholder is added as the plain string "[dim]no data[/dim]", while generate_analysis_report() deliberately renders with markup=False. Through the public DatasetProfilerResults.to_report() path, the saved HTML therefore contains and displays the literal text [dim]no data[/dim] rather than a dimmed no data label.
  • Why: The PR fixes the exceptions, but every repaired empty/missing-histogram case now produces visibly malformed report output on the exact path under test. The current tests miss this because they only check the report size and the value-column cells.
  • Suggestion: Pass a styled renderable instead, e.g. table.add_row(Text("no data", style="dim"), "") (Text is already imported), and extend the saved-report assertion to verify "no data" is present without the Rich control tags.

What Looks Good

  • Putting default=0 on the shared histogram maximum handles empty input at the narrowest reusable boundary.
  • The MissingValue union and .get() fallback align report rendering with the shapes the Pydantic models already accept, without tightening the serialized schema.
  • The parametrized public-path tests cover empty, missing, incomplete, and populated cases, and the full config suite remains green.

Verdict

Needs changes — render the empty-state label with a Text object and lock the saved output down with an assertion so the repaired path does not expose Rich markup tags.


This review was generated by an AI assistant.

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.

Report generation crashes on empty categorical histograms Judge-score report generation crashes on missing or incomplete score distributions

2 participants