Skip to content

test(verify): compare the charts the two paths drew - #8509

Draft
kz930 wants to merge 9 commits into
apache:mainfrom
kz930:feat/verify-compare-two-charts
Draft

kz930 wants to merge 9 commits into
apache:mainfrom
kz930:feat/verify-compare-two-charts

Conversation

@kz930

@kz930 kz930 commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

A visualization operator produces no table, so the table comparison has nothing to read. It writes a plotly figure and an HTML page, and this compares those.

A chart's meaning is in the numbers behind it, so the comparison reads those rather than the rendered picture, which would fail over a layout detail that carries none. The figure is compared trace by trace.

The page needs two things normalized away first, neither of which is markup the operator chose: a Styler id, which pandas regenerates on every run, and the line ending, which on Windows is CRLF for a file Python opened in text mode while the same markup carried through JSONL keeps the LF the engine wrote.

#8359 compares two tables, which is a different question and shares no code with this one.

Any related issues, documentation, discussions?

Part of #8325, 7 of 27; that issue lists the set in order.

Closes #8508, the task this change is the whole of.

How was this PR tested?

VisualizationHtmlComparatorSpec pins both normalizations: two pages differing only in a Styler id compare equal, and so do two differing only in line endings, while a page differing in the markup itself does not.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5)

🤖 Generated with Claude Code

A chart's meaning is in the numbers behind it, so the comparison reads
those rather than the rendered picture, which would fail over a layout
detail that carries none. Plotly writes both a JSON figure and an HTML
page, and each needs its own reading: the figure is compared trace by
trace, and the page has a Styler id regenerated per run and a line ending
chosen by whichever platform wrote the file, neither of which is markup
the operator chose.

Split out of apache#8359 on review. That change compares two tables, which is a
different question and shares no code with this one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added feature platform Non-amber Scala service paths labels Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @tanishqgandhi1908
    You can notify them by mentioning @tanishqgandhi1908 in a comment.

@codecov-commenter

codecov-commenter commented Sep 11, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.51%. Comparing base (1fbd346) to head (739b316).
⚠️ Report is 83 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8509      +/-   ##
============================================
- Coverage     93.69%   92.51%   -1.19%     
- Complexity     4826     4922      +96     
============================================
  Files          1209     1223      +14     
  Lines         49871    51029    +1158     
  Branches       6099     6254     +155     
============================================
+ Hits          46727    47208     +481     
- Misses         1652     2258     +606     
- Partials       1492     1563      +71     
Flag Coverage Δ *Carryforward flag
access-control-service 77.38% <ø> (-2.81%) ⬇️
agent-service 99.32% <ø> (ø) Carriedforward from e47995c
amber 88.59% <ø> (-1.33%) ⬇️ Carriedforward from e47995c
computing-unit-managing-service 60.41% <ø> (-16.74%) ⬇️
config-service 87.37% <ø> (+0.24%) ⬆️
file-service 81.53% <ø> (-2.12%) ⬇️ Carriedforward from e47995c
frontend 96.16% <ø> (ø) Carriedforward from e47995c
notebook-migration-service 83.73% <ø> (ø)
pyamber 98.47% <ø> (ø) Carriedforward from e47995c
workflow-compiling-service 74.09% <ø> (-3.10%) ⬇️ Carriedforward from e47995c

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

kz930 and others added 2 commits September 12, 2026 00:51
The uuid pattern matched anywhere in the page, so two tables whose cells
read T_dead and T_beef compared equal and a real difference in exported
data went unreported. Replace the uuid only where a Styler writes it, in
the id attribute and the selector that targets it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A cell is not escaped, so a value reading `#T_dead` looked like a CSS
selector and two tables disagreeing about it still compared equal. The
uuid is now replaced inside the `<style>` element and inside tags, which
is everywhere a Styler writes one and nowhere the table speaks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@carloea2

Copy link
Copy Markdown
Contributor

There is also a build dependency missing in this PR: VisualizationJsonComparator imports PythonWorkerPool from WorkflowOperator test sources, but WorkflowCompilingService does not depend on those test classes. The current CI job fails to compile that import. Please include the test dependency here so this PR builds independently. CI: https://github.com/apache/texera/actions/runs/34804943402/job/103854922027

VisualizationJsonComparator reuses PythonWorkerPool, which lives in
workflow-operator's test sources, so the module needs it in test scope.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kz930

kz930 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Good catch, thanks. Added the test-scope dependency in dfa14dc, so the PR builds on its own now. The same line is in #8357 with identical wording, so the two merge without a conflict.

@github-actions github-actions Bot added dependencies Pull requests that update a dependency file common labels Sep 14, 2026
@github-actions

github-actions Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 7 worse · ⚪ 6 noise (<±5%) · 0 without baseline

Compared against main 7b8422e benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 543 0.331 17,104/23,315/23,315 us 🔴 +19.7% / 🔴 +65.5%
🔴 bs=100 sw=10 sl=64 1,188 0.725 80,476/129,658/129,658 us 🔴 +33.9% / 🔴 +34.5%
🟢 bs=1000 sw=10 sl=64 1,448 0.884 685,916/771,314/771,314 us 🟢 -8.6% / 🟢 -22.6%
Baseline details

Latest main 7b8422e from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 543 tuples/sec 630 tuples/sec 888.52 tuples/sec -13.8% -38.9%
bs=10 sw=10 sl=64 MB/s 0.331 MB/s 0.385 MB/s 0.542 MB/s -14.0% -39.0%
bs=10 sw=10 sl=64 p50 17,104 us 14,292 us 11,467 us +19.7% +49.2%
bs=10 sw=10 sl=64 p95 23,315 us 22,628 us 14,087 us +3.0% +65.5%
bs=10 sw=10 sl=64 p99 23,315 us 22,628 us 17,519 us +3.0% +33.1%
bs=100 sw=10 sl=64 throughput 1,188 tuples/sec 1,277 tuples/sec 1,174 tuples/sec -7.0% +1.2%
bs=100 sw=10 sl=64 MB/s 0.725 MB/s 0.78 MB/s 0.716 MB/s -7.1% +1.2%
bs=100 sw=10 sl=64 p50 80,476 us 77,509 us 89,952 us +3.8% -10.5%
bs=100 sw=10 sl=64 p95 129,658 us 96,862 us 96,426 us +33.9% +34.5%
bs=100 sw=10 sl=64 p99 129,658 us 96,862 us 110,188 us +33.9% +17.7%
bs=1000 sw=10 sl=64 throughput 1,448 tuples/sec 1,447 tuples/sec 1,204 tuples/sec +0.1% +20.3%
bs=1000 sw=10 sl=64 MB/s 0.884 MB/s 0.883 MB/s 0.735 MB/s +0.1% +20.3%
bs=1000 sw=10 sl=64 p50 685,916 us 680,992 us 886,711 us +0.7% -22.6%
bs=1000 sw=10 sl=64 p95 771,314 us 843,478 us 928,644 us -8.6% -16.9%
bs=1000 sw=10 sl=64 p99 771,314 us 843,478 us 972,356 us -8.6% -20.7%
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,368.49,200,128000,543,0.331,17104.05,23315.19,23315.19
1,100,10,64,20,1684.11,2000,1280000,1188,0.725,80476.49,129658.06,129658.06
2,1000,10,64,20,13813.15,20000,12800000,1448,0.884,685915.58,771313.86,771313.86

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTML ID handling now preserves cell text. The comparator tests look good.

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTML comparison reads only the first nonempty runtime row. If native returns HTML A and B but standalone returns only A, this still passes. Visualization operators can emit one HTML result per input row. Please compare every row and add a multirow case.

The comparison read the first row of the runtime path's output and stopped,
so a run that drew a chart the exported script never drew still passed. The
two sides are now compared as sequences, and the failure says how many each
path drew and what the unmatched one held.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kz930

kz930 commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in fe7ac26: the two sides are compared as sequences now, so a run that drew a chart the exported script never drew fails instead of passing on its first row, and the message says how many each path drew. The multirow case is in, with the second chart named in the failure.

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTML comparison now checks every runtime row and rejects a missing extra result. The earlier finding is fixed. Looks good.

kz930 and others added 2 commits September 18, 2026 15:52
The comparison had no spec of its own, and the case worth holding is the
multi-figure one: an operator that draws a figure per input row agrees on
the first long after it has stopped agreeing, so a comparison that read one
figure and stopped called that a match. A figure the other path drew
differently, and one it never drew at all, are each their own test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comparison says "the run drew 2 and the exported script drew 1" where the
assertion was still reading the wording this spec was written against.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added figure comparison tests cover the revised chart handling. No new blocker found.

…own spec

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the current head. All eight focused comparator tests and scalafmtCheckAll pass with compare.py and the IntegrationTest annotation supplied from #8359. The earlier cell-text normalization finding remains fixed. The standalone branch still needs those dependency files, so this is not a merge-ready approval. Leaving it as a draft.

@carloea2 carloea2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 8 visualization comparator tests and scalafmtCheckAll pass with compare.py supplied from #8359. The earlier cell-text normalization finding stays fixed, and extra chart rows are rejected. Approved for the declared dependency order, not for merging before #8359.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common dependencies Pull requests that update a dependency file feature platform Non-amber Scala service paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compare the charts the two paths drew

3 participants