Repository navigation
Conversation
Accept pandas Styler as a valid table output type. TableOutputHandler
now converts a Styler into a DataFrame whose styled cells are
serialized as {value, bgcolor, color, fontWeight, textAlign} dicts —
a portable, JSON-safe schema consumed by the documentation UI and
report renderers.
StatefulHTMLRenderer renders structured cells back to HTML for
in-notebook display so notebook output matches portal rendering
without leaking raw {'value': ...} JSON.
Adds a how-to notebook demonstrating styled-cell output from a
custom test, plus tests covering the Styler conversion path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR SummaryThis PR introduces significant improvements to the handling and rendering of custom test tables within the ValidMind framework. The key changes include:
These changes aim to offer improved flexibility and consistency in how styled test tables are processed, displayed, and logged, leading to a better user experience when working with visual test outputs. Test Suggestions
|
|
@copilot resolve the merge conflicts in this pull request |
…le-summary-table-cells # Conflicts: # tests/test_results.py Co-authored-by: cachafla <21595+cachafla@users.noreply.github.com>
Resolved the merge conflicts and merged |
|
|
- decorator.py: @vm.test / @vm.scorer docstrings list `pandas Styler` as a valid table return type (these feed the generated Quarto API reference) - ResultTable: document the structured cell schema so the wire format shared with the platform is written down in the repo - run_e2e_notebooks.py: run style_custom_test_tables.ipynb in CI Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
panchicore
left a comment
There was a problem hiding this comment.
Review: validmind/validmind-library PR #497 at cea6188 (merge-base 7e0238d)
Verdict: Request changes (pending human confirmation)
Computed by the calibrated-review skill, run by Luis (panchicore).
Summary
- What changed: Tests can now return a
pandas.Stylerinstead of aDataFrame; styled cells are converted into portable{value, bgcolor, color, fontWeight, textAlign}dicts that the docs UI, HTML reports, and DOCX exports (companion PRs backend#3040, frontend#2470) will render. - Result: One blocking correctness bug: any test that returns a
Stylersilently loses the existing 4-decimal rounding for every numeric cell in the result table, not just the styled ones. - Main thing to know:
_convert_stylercalls.astype(object)on the whole DataFrame before placing any styled dict, soResultTable.__post_init__'sself.data.round(4)becomes a no-op for the entire table the moment a test returns aStyler— even for columns and cells that were never styled. - Next step: Round numeric values before/independently of the
astype(object)conversion (e.g. round the source DataFrame first, or round each extracted scalar when building the cell dict), then add a regression test with an unformatted numeric column that has no styling at all.
Blocking findings
Styler conversion silently defeats table-wide value rounding
- Type: correctness
- Direction: risky
- What happens: A test returns
df.style.map(...)where the CSS rule only touches one column (or even zero cells, if the condition never matches)._convert_stylerdoesdf = styler.data.copy(deep=True).astype(object)unconditionally, which converts every column — styled or not — to object dtype.ResultTable.__post_init__then callsself.data.round(4), but pandas'.round()silently skips non-numeric (object) dtype columns instead of raising. The result: full, unrounded floating-point precision reachesResultTable.serialize()for every cell in the table, including cells and columns that were never styled at all. - Why it matters: This contradicts the PR's own claim ("no breaking changes — existing tests... are unaffected") in spirit: any customer who adopts the new Styler feature on one column of a table gets a silent precision regression across the whole table, with no error and no visual cue in the notebook. It also inflates the JSON payload sent to the platform (the exact "serialization size" concern flagged for this review) since every float now serializes with full precision instead of 4 decimals.
- Diagram:
flowchart TD
A["Test returns df.style.map(...)\n(styles only column 'styled_col')"] --> B["_convert_styler:\ndf.astype(object) on ENTIRE frame"]
B --> C["only cells matching the CSS rule\nbecome {value, bgcolor, ...} dicts"]
C --> D["ResultTable.__post_init__:\nself.data.round(4)"]
D --> E["pandas .round() skips\nobject-dtype columns silently"]
E --> F["untouched_col AND unstyled cells\nin styled_col keep full float precision"]
- Evidence:
validmind/tests/output.py:112—df = styler.data.copy(deep=True).astype(object)runs before any styling is known to apply, converting every column.validmind/vm_models/result/result.py:144—self.data = self.data.round(4)is the existing rounding contract that silently no-ops on object dtype.- Ran the actual
TableOutputHandler._convert_styler+ResultTablefrom the repo at the PR head (script below) against a two-column DataFrame where onlystyled_colhas a styling rule anduntouched_colhas none:→df = pd.DataFrame({ "styled_col": [1.23456789, 2.3456789, 3.0], "untouched_col": [10.111111, 20.222222, 30.333333], }) styler = df.style.map(lambda v: "background-color: red" if v > 2 else "", subset=["styled_col"]) rt = ResultTable(data=TableOutputHandler()._convert_styler(styler))
rt.data.at[0, "untouched_col"]is10.111111(expected10.1111if rounding still applied);rt.serialize()["data"][0]is{'styled_col': 1.23456789, 'untouched_col': 10.111111}— full precision, unrounded, on a column that has zero styled cells. tests/test_results.py:138-172(test_table_output_handler_converts_pandas_styler) is the only Styler test added by this PR; it uses a Styler formatter (.format({"Observed": "{:.1%}"})) that turns the value into a string before it ever reaches.round(), so it never exercises an unformatted numeric column and does not catch this.uv run python -m unittest tests.test_results -v→ 32/32 pass, confirming the existing suite does not detect this regression.
- Suggested fix: Round the source data before or independently of the object-dtype conversion — e.g.
styler.data.copy(deep=True).round(4).astype(object)(round while still numeric, then widen dtype), or round each scalar right before building the cell dict / plain value in_convert_styler. Either way, add a test with a numeric column that has zero styled cells alongside a styled column, asserting it still comes out rounded.
Questions for a human
-
Owner: cachafla
-
Question:
_convert_stylerrelies on three underscore-prefixed pandas internals —Styler._compute(),Styler.ctx, andStyler._display_funcs(validmind/tests/output.py:110-136). These aren't part of pandas' public API contract, so a future pandas point release inside the repo's ownpandas (>=2.0.3,<3.0.0)constraint (pyproject.toml) could rename or restructure them without a deprecation warning, breaking every customer notebook that returns a styled table with anAttributeErrordeep inside test execution. Is the team OK accepting that maintenance risk, or should this pin a narrower pandas range / add a version guard with a clear error message? -
Context: I could not install pandas 2.0.3 in this environment to test the low end of the declared range (build fails under Python 3.14 due to missing
pkg_resources), so I can't show it actually breaks anywhere in the supported range — verified only that it works on the currently pinned 2.3.3. Per the skill's reachability rule, "I could not construct the failing scenario" downgrades this from a blocking finding to a question rather than a defect. -
Owner: cachafla (and the backend/frontend reviewers on the sibling PRs)
-
Question: The
ResultTabledocstring now states plainly that "CSS values are passed through verbatim from the test author" (validmind/vm_models/result/result.py:135-136) — i.e., this library does zero sanitization ofbgcolor/color/etc. before they're serialized and handed to the platform. That's a reasonable design (the library is the producer, not the renderer), but it means backend#3040 and frontend#2470 are the only places that can stop a malicious or malformed CSS string from reaching an HTML/DOCX document. Has that expectation been confirmed with those two PRs' reviewers, since this PR doesn't itself require it? -
Context: Within this repo, the one place styled cells are rendered to HTML (
validmind/vm_models/html_renderer.py:24-51, in-notebook display) does escape correctly — both the CSS value and the display value go throughhtml.escape()before being placed in astyle="..."attribute — so the library's own rendering is safe. The verbatim pass-through only becomes a risk in the two repos this review is explicitly not covering.
Accepted risks
-
What I checked: Whether the new
Styler/DataFramedual acceptance inTableOutputHandler.can_handle/process(validmind/tests/output.py:105-106,172-177) breaks the "no breaking changes" claim for existingDataFrame,dict,list, and scalar test outputs. -
Why it is acceptable: Confirmed the
isinstance(table_data, pd.DataFrame)branch and_convert_simple_typeare untouched; ran the fulltests/test_results.pysuite (uv run python -m unittest tests.test_results -v→ 32/32 pass) with no regressions outside the Styler path. The claim holds for pre-existing table types. -
What I checked: Whether
escape=FalseinStatefulHTMLRenderer.render_table'sto_html()call (validmind/vm_models/html_renderer.py:194) lets a raw scalar test value inject HTML/script into a notebook. -
Why it is acceptable: That
escape=Falsesetting predates this PR (present unchanged invalidmind/vm_models/html_renderer.pyat the merge-base) and applies to plain scalar cells exactly as it did before — this PR doesn't touch that exposure, it only adds a formatter (_render_table_cell) that, for the new dict-shaped styled cells, actively escapes both the CSS values and the display value before insertion. So this PR makes the escaping story strictly better for the cells it touches and changes nothing for the cells it doesn't. Out of scope to fix here. -
What I checked: Whether a plain
DataFramecould reach a code path that specifically expects aStyler(or vice versa), given the task's "what if the wrong type is passed" framing. -
Why it is acceptable: No such path exists —
TableOutputHandlerandTABLE_TYPES(validmind/tests/load.py:24) accept both types interchangeably and dispatch byisinstanceinside the same function, so there's no "Styler expected but got DataFrame" branch to break.
Verification performed
git merge-base cea6188f0939920f74954adddf82f2db4f7a3b85 origin/main→7e0238d6c9bef513f010ce6eb613dad6e079454e(matchesbaseRefOidreported bygh pr view).gh pr view 497 --repo validmind/validmind-library --json ...→ confirmed headcea6188f09..., no existing reviews, authorcachafla≠ current userpanchicore(not a self-review).git worktree add <tmpdir> cea6188f0939920f74954adddf82f2db4f7a3b85— reviewed at a pinned, isolated checkout rather than switching branches in place.uv syncin the worktree → resolved env with pandas2.3.3(within the repo'spandas (>=2.0.3,<3.0.0)constraint), jinja2, etc.uv run python -m unittest tests.test_results -v→ 32/32 passed (includes the PR's newtest_table_output_handler_converts_pandas_styler).uv run python -m unittest tests.test_results -k styler -v→ 1/1 passed (isolated).- Ran a standalone probe script importing the actual
TableOutputHandlerandResultTablefrom the PR head to reproduce the rounding regression (see Blocking findings evidence above). - Verified
notebooks/how_to/tests/custom_tests/style_custom_test_tables.ipynbis valid JSON (json.loadsucceeds); did not execute it end-to-end (no ValidMind API credentials in this environment) — PR claim that CI runs it viascripts/run_e2e_notebooks.pyis NOT CHECKED beyond confirming the notebook was added toNOTEBOOKS_TO_RUN. - Checked repo root for
AGENTS.md/CLAUDE.md/CONTRIBUTING.mdconventions files — none exist in validmind-library, so no repo-specific convention violations to check against beyond what's inferred from the code itself. - Confirmed
escape=Falseinhtml_renderer.py'srender_tablepredates this PR by diffing against the merge-base — not a regression introduced here. - PR claim "No breaking changes — existing tests that return
pd.DataFrame,dict, or scalar tables are unaffected" — CHECKED, holds (see Accepted risks). - Attempted to install pandas
2.0.3(the low end of the declared support range) to test private-API stability at that version — failed to build under Python 3.14 (ModuleNotFoundError: No module named 'pkg_resources'); NOT CHECKED at that version, only at the currently pinned 2.3.3.
_convert_styler widened the whole frame to object dtype before ResultTable.__post_init__ ran .round(4), which silently skips object columns. Round while the data is still numeric; formatters keep seeing the original value. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks @panchicore. Addressed in 6095c6f. Rounding regression (blocking): Private pandas API ( CSS verbatim pass-through: confirmed with the sibling PRs. The library only produces; backend#3040's allowlist ( |
…sion it caught A regression test for unstyled tables found that render_table's new per-column formatter returned raw numbers, which pandas rejects (TypeError: object of type 'numpy.float64' has no len()). Attach the formatter only to columns that hold a styled cell, and return strings from it, so plain columns render exactly as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Follow-up in b71a600: added a regression test for the untouched path, and it caught a real bug.
34/34 passing. |
panchicore
left a comment
There was a problem hiding this comment.
🌮
Review: validmind/validmind-library PR #497 at b71a600 (merge-base 7e0238d)
Verdict: Merge with recommendations
Computed by the calibrated-review skill, run by Luis (panchicore). This is a delta re-review: the prior CHANGES_REQUESTED review (2026-09-14T15:26:33Z, at head cea6188f0) requested one blocking fix; this re-review checks commits 6095c6fb and b71a600f3 against that finding and reviews them for new regressions.
Summary
- What changed since the last review:
6095c6fbfixes the rounding regression from the prior review.b71a600f3adds a regression test for plain (non-Styler)DataFrametables, which caught and fixed a second, pre-existing bug in the same feature: in-notebook HTML rendering crashed for any table with a numeric column, Styler or not. - Result: The one blocking finding from the prior review is resolved and verified against the actual code, not just the author's claim. No new blocking findings.
- Main thing to know: The fix for the crash bug (
b71a600f3) introduces one narrow, cosmetic formatting inconsistency in the in-notebook HTML view — non-blocking, included below as a fix-now item. - Next step: Optional: apply the fix-now formatting nit. Otherwise this is ready to merge; the two risk-appetite questions from the prior review remain open for a human to sign off on (author has responded to both, see below).
Previous blocking findings — status
- Styler conversion silently defeats table-wide value rounding — RESOLVED. Verified at the code:
_convert_stylernow rounds while the frame is still numeric (validmind/tests/output.py:116,source.round(4).astype(object)) before theobject-dtype cast that previously madeResultTable.__post_init__'s.round(4)a silent no-op. Re-ran the original review's reproduction probe against the fixed code (two-column frame, only one column styled):untouched_colnow comes out10.1111(was10.111111), and the styled cell'svaluealso rounds to2.3457. The author's new testtest_table_output_handler_styler_keeps_rounding(tests/test_results.py:196-219) covers the same scenario and passes.
Fix now
- What: In a numeric column that mixes styled and unstyled cells, unstyled cells now render via Python's
str()in the in-notebook HTML view instead of pandas' default column-consistent decimal padding, so they can show different precision than their neighbors (e.g.1.0next to100.25instead of1.00). - Why: It's a real, verified formatting inconsistency — not a data problem (the underlying value and the JSON
serialize()payload are correct either way), and it's confined to this repo's own in-notebook display; the docs UI/HTML/DOCX exports render from the{value, bgcolor, ...}schema independently in the companion backend/frontend PRs, unaffected by this. Low cost to clean up while the file is already open in this PR. - Where:
validmind/vm_models/html_renderer.py:24-28(_render_table_cell'sstr(value)fallback) andvalidmind/vm_models/html_renderer.py:189-193(formatter now applied per-column instead of per-cell). - Suggested fix: Format the fallback value with the same precision pandas would use for the column (e.g. via the column's own default formatter) instead of a bare
str(), or accept it as intentional and note it in a code comment.
Questions for a human
-
Owner: cachafla
-
Question: (carried over, unresolved)
_convert_stylerstill relies on three underscore-prefixed pandas internals (Styler._compute(),.ctx,._display_funcs, unchanged by this delta —validmind/tests/output.py:110,117,127,138). Is the team OK accepting the maintenance risk of a future pandas point release renaming these without a deprecation warning? -
Context: In his 16:50:31Z comment, cachafla confirmed he's accepting this risk rather than narrowing the
pandas (>=2.0.3,<3.0.0)range, reasoning that a break would surface as anAttributeErrorat test-run time and be easy to spot in the e2e notebook run. I did not independently re-verify this (it wasn't part of what changed in this delta); flagging so a human explicitly signs off rather than this being implicitly accepted by silence. -
Owner: cachafla (and the backend/frontend reviewers on the sibling PRs)
-
Question: (carried over, unresolved by this repo) Has the CSS verbatim pass-through — this library does zero sanitization of
bgcolor/color/etc. before serializing them — been confirmed safe with the backend (backend#3040) and frontend (frontend#2470) reviewers, since those are the only places that can stop a malicious/malformed CSS string from reaching an HTML/DOCX document? -
Context: In the same comment, cachafla says he cross-checked 16 boundary cases (hex colors of various lengths, rgb/rgba, hsl/hsla,
deg, negative hue, named colors,javascript:,red;x) against backend's allowlist and frontend's color2k parser, and both agree on every one. This claim is about code in two other repos outside this review's scope — I did NOT check it; it's presented here as the author's claim, not verified evidence.
Accepted risks
-
What I checked: Whether the rounding fix (
source.round(4).astype(object), dropping the previous explicit.copy(deep=True)) introduces an aliasing bug that mutates the caller's originalDataFrame/Styler.data. -
Why it is acceptable:
.round()returns a newDataFramerather than a view, so the subsequent.astype(object)anddf.iat[...] = ...mutations never touchstyler.data. Verified with a probe: after calling_convert_styler, bothdf["styled_col"](the original source frame) andstyler.data["styled_col"]remain[1.23456789, 5.0], unmutated. -
What I checked: Whether the plain-
DataFramecrash fix (b71a600f3) changes behavior for tables that were already working (Styler-styled tables, and plain tables with no numeric columns). -
Why it is acceptable: Ran the full
tests/test_results.pysuite at the PR head — 34/34 pass, including the pre-existing Styler HTML test and the two new regression tests. The formatter is now attached only to columns containing at least one dict (styled) cell, so plain tables get no formatter at all and fall back to pandas' untouched default rendering, which is what the newtest_table_output_handler_plain_dataframe_is_unchangedasserts byte-for-byte for the cells it checks.
Verification performed
gh api repos/validmind/validmind-library/pulls/497→ confirmed headb71a600f3be7e7974f220f2d3158b2ecfafcea0a, base7e0238d6c9bef513f010ce6eb613dad6e079454e(same merge-base as the prior review — base branch has not moved).gh api repos/validmind/validmind-library/pulls/497/reviews→ read panchicore'sCHANGES_REQUESTEDreview body in full (one blocking finding, two non-blocking questions).gh api repos/validmind/validmind-library/issues/497/comments→ read both of cachafla's response comments (16:50:31Z, 17:10:25Z) verbatim.git worktree add --detach <path> b71a600f3...in an isolated clone (never checked out in a shared/main working copy) → reviewed at the pinned head.git diff cea6188f09... b71a600f3...→ confirmed onlyvalidmind/tests/output.py,validmind/vm_models/html_renderer.py,tests/test_results.pychanged since the prior review.uv syncin the worktree → resolved environment (pandas 2.3.3, same as the prior review).uv run python -m unittest tests.test_results -v→ 34/34 passed.- Reproduced the prior review's rounding probe against the fixed code →
untouched_colnow10.1111(rounded), styled cellvaluenow2.3457(rounded) — matches the expected fix. - Wrote and ran a probe confirming the pre-existing plain-
DataFrameHTML crash was real: checked out the prior review's head (cea6188f0, before either new commit) in a second isolated worktree and ranTableOutputHandler().process(df, result); result.to_html()on a plain two-column numericDataFrame→TypeError: object of type 'numpy.float64' has no len(). Confirms this bug predates the delta (was already in the code reviewed on 2026-09-14, not introduced by the new commits) and thatb71a600f3's fix is addressing a real, previously-unflagged defect, not a regression it created. - Wrote and ran a probe on a mixed styled/unstyled numeric column → confirmed the fix-now formatting inconsistency (
1.0viastr()vs pandas' default1.00padding) is real and reproducible. - Wrote and ran a probe confirming no aliasing/mutation bug from dropping the explicit
.copy(deep=True)in_convert_styler. - PR-body claim "34/34 passing" (17:10:25Z comment) — CHECKED, matches.
- Author's claim of cross-checking 16 CSS boundary cases against the backend/frontend sibling PRs — NOT CHECKED (out of scope: requires reading code in two other repos).
- Author's claim of accepting the private-pandas-API maintenance risk — NOT CHECKED beyond confirming the code is unchanged from the prior review; this is a risk-appetite decision for a human, not a code claim to verify.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the re-review. Took the "accept it as intentional and note it in a code comment" option in e69208c. A column that contains a styled cell is object dtype, so there is no column-wide float padding for the plain cells to match; the comment in The pandas private-API risk stays accepted as stated in my earlier comment. |
What and why?
Lets test authors style cells in their test summary tables — color-coded status pills, severity shading, bold thresholds — by returning a
pandas.Stylerfrom a test. Styled cells are serialized into a portable, JSON-safe schema that the documentation UI, HTML reports, and DOCX exports all render consistently.This PR is the library piece. The backend (HTML/DOCX rendering) and frontend (UI rendering) pieces are in companion PRs.
Schema (shared across the three repos): cells may be a scalar or a dict of the form
What changed
validmind/tests/load.py—pandas.io.formats.style.Styleris now an acceptedTABLE_TYPESoutput from a test.validmind/tests/output.py—TableOutputHandleracceptsStylerand converts it via a new_convert_stylerhelper. Each cell that has CSS styles applied (background-color/background,color,font-weight,text-align) becomes a{value, bgcolor, color, fontWeight, textAlign}dict; the formatted display value (per the Styler's formatters) is preserved undervalue. Unstyled cells stay as plain scalars.validmind/vm_models/html_renderer.py—StatefulHTMLRendererrenders structured cells back to HTML for in-notebook display, so notebook output matches portal rendering instead of showing raw{'value': ...}JSON. CSS values are HTML-escaped.notebooks/how_to/tests/custom_tests/style_custom_test_tables.ipynb— a how-to demonstrating styled-cell output from a custom test.tests/test_results.py— covers the Styler conversion path (formatter preservation, all four CSS properties, HTML rendering).validmind/tests/decorator.py— the@vm.test/@vm.scorerdocstrings listed onlylist of dictionaries or a pandas DataFrameas valid table outputs; they now listStylertoo and point atResultTablefor the cell schema. These docstrings are the source for the generated Quarto API reference on docs.validmind.ai.validmind/vm_models/result/result.py—ResultTabledocstring documents the structured cell schema (shape, partial keys, mixed scalar/dict columns, formatter behavior, verbatim CSS values) so the wire format is written down in the repo and not only in PR descriptions.scripts/run_e2e_notebooks.py— the new how-to is added toNOTEBOOKS_TO_RUNso CI executes and logs it end to end.How to test
notebooks/how_to/tests/custom_tests/style_custom_test_tables.ipynb.pandas.Styler(e.g.,df.style.map(lambda v: "background-color: #EAF4FF" if ...)).TestResult— styled cells should be dicts of the shape above; unstyled cells remain scalars.{'value': ...}JSON.pytest tests/test_results.py -k styler.python -c "import validmind as vm; help(vm.test)"should listpandas Styleras a table output, andhelp(vm.vm_models.result.ResultTable)should describe the styled-cell schema.What needs special review?
_convert_stylermapping from CSS properties to schema keys (style_key_map) — this defines what styling is portable. We currently supportbackground[-color],color,font-weight,text-align. Anything else is dropped.ResultTabledocstring must stay in sync with what the backend and frontend actually render — it is now the canonical written description of the contract._display_funcsbefore being placed in the cell dict, so user-applied number formatting is preserved.Dependencies, breaking changes, and deployment notes
backend— https://github.com/validmind/backend/pull/3040frontend— https://github.com/validmind/frontend/pull/2470pd.DataFrame,dict, or scalar tables are unaffected.Release notes
Tests can now return a
pandas.Stylerto apply cell-level styling (background color, text color, bold, alignment) to summary tables — the styling is preserved consistently in the documentation UI, HTML reports, and DOCX exports. See the newstyle_custom_test_tableshow-to notebook.Checklist