Repository navigation
feat(ess): give a provisioned ChartArtifact chart its plot specs - #810
Merged
Merged
Conversation
This was referenced Sep 7, 2026
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to derive plot specifications from client-supplied metric names when creating charts on the Dashboard, including helper functions to slugify and deduplicate plot IDs. It also updates the observer to record the server-assigned chart back to the artifact and adds comprehensive unit tests. The feedback highlights a potential memory allocation issue where the slice and map capacities are allocated based on the unbounded client-supplied list, which could lead to high memory overhead or OOM issues.
AntonTyb
force-pushed
the
feat/ess-chart-artifact-plot-specs
branch
from
September 8, 2026 08:35
d552c4b to
3860827
Compare
AntonTyb
marked this pull request as ready for review
September 8, 2026 08:43
andrzejfudro
previously approved these changes
Sep 8, 2026
AntonTyb
force-pushed
the
feat/ess-chart-artifact-plot-specs
branch
from
September 8, 2026 15:59
8000
3860827 to
57b55f0
Compare
andrzejfudro
previously approved these changes
Sep 9, 2026
Base automatically changed from
fix/ess-chart-provisioning-lifecycle
to
main
September 9, 2026 07:51
A chart was provisioned with no plots at all. That is not an empty chart to the
data engine, it is a chart with nothing to query, so it rendered blank however
much data the work unit had logged.
Each metric name now becomes two plots -- a summary box holding the latest value
and a step-indexed scatter curve -- laid out two to a row so the summary boxes
sit side by side.
Two details worth the reviewer's attention:
- The metric list is client-supplied and the data engine runs one query per
plot, so maxProvisionedMetrics caps the expansion at 20 metrics. Without it a
single GetChartData fans out in proportion to whatever the client sent.
- Plot IDs are slugified metric names, so distinct metrics can collide
("train/loss" and "train_loss"). uniquePlotID resolves collisions by skipping
IDs already handed out rather than counting occurrences: a metric whose own
name ends in a number would otherwise collide with a generated suffix, and
["foo", "foo_2", "foo"] would yield foo_2 twice. The UI keys plot data by ID,
so a duplicate makes two plots draw the same series.
TAG=agy
CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
CreateChart's response was discarded, so the artifact kept a list of metric names and no way to find the chart that draws them -- the server-assigned resource name is the only link between the two. Storing the returned Chart on the payload closes that gap, and the status write that already runs on the success path persists it. TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
plotSpecsForMetrics reserved capacity for 2*len(metricNames) plots while the loop eight lines below stops at 2*maxProvisionedMetrics, so the hint contradicted the bound it sat above: a client sending a million metric names reserved two million pointers to hold forty. Reserve what the loop can actually keep. TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
Review round 3 on #810. Metrics past maxProvisionedMetrics were dropped silently. The chart renders without them and nothing else records the truncation, so the only symptom is a metric the caller named having no plot. plotSpecsForMetrics takes a context and warns with the counts instead. TAG=agy CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f
AntonTyb
force-pushed
the
feat/ess-chart-artifact-plot-specs
branch
from
September 9, 2026 07:51
57b55f0 to
0ac8c41
Compare
There was a problem hiding this comment.
🤖 Incremental PR Approval
Clean rebase detected with zero semantic patch lines changed.
Preserving prior approval by @andrzejfudro on commit 57b55f0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Stacked on #809 (base branch
fix/ess-chart-provisioning-lifecycle). Slice 2 of 4 from thesplit of #710, requested in review.
With provisioning now surviving and authenticating, the chart it provisions is still
unqueryable and unfindable. Two more
dashboard_observer.godefects.1. The provisioned chart had no plots
OnArtifactCreatedcalledCreateChartwith onlyTitle,XidandWid.dataengine.FetchChartDataiterateschart.GetPlots(), so a chart with no plots yields nodata no matter how many metrics the work unit logged — the chart is not empty-looking, it is
unqueryable.
The observer now derives plot specs from the artifact's
metric_names: per metric, aSUMMARY_BOXfor the latest value and a step-indexedSCATTERcurve, on a two-columnGRIDlayout.
Metric names are path-like (
chart_demo/loss), so they are slugified into plot IDs, with acounter suffix disambiguating names that slugify alike (
train/lossvstrain_loss). Thenumber of metrics expanded is capped at 20 — the list is client-supplied and the data engine
runs one query per plot, so an unbounded list turns a single
GetChartDatainto aproportionally expensive fan-out.
2. The artifact never recorded which chart was provisioned for it
The
CreateChartresponse was discarded, soChartArtifact.chartstayed empty and a readerwas left holding a list of metric names with no way to find the chart that draws them. The
observer now stores the created
Chart(which carries the server-assigned resource name) onthe artifact payload, persisted by the status write slice 1 made reliable.
The alternative considered and rejected was a UI-side
ListChartson the work unit parent —that is a by-parent heuristic, whereas this gives artifact → chart identity, and it is what
the
OUTPUT_ONLY chartfield in the proto was there for.Testing
gofmtclean,golangci-lintexit 0.Not run locally: Go tests, for the Bazel-mapped import path reason described on slice 1. CI is
their first real run.
New tests in
dashboard_observer_test.go:TestPlotSpecsForMetrics— derived plot IDs, types, metrics and layout; the emptymetric_namescase; blank-name skipping; slug collisions; the placeholder slug for a namewith no usable characters; and the cap.
..._DerivesPlotsFromMetricNamesand..._NoPlotsWithoutMetricNames— the specs reachCreateChart, and an artifact naming no metrics does not get a plotless chart.The
uniquePlotIDcollision case (["foo", "foo_2", "foo"]handed outfoo_2twice, so theUI — which keys plot data by ID — drew the same series into both plots) was checked to fail
against the pre-fix implementation.
Notes for the reviewer
real plots, observable through the API, but nothing renders it until slice 3.
Stack
feat/chart-artifact-example(#676)fix/ess-chart-provisioning-lifecyclefeat/ess-chart-artifact-plot-specsfeat/ui-chart-artifact-viewfeat/ui-chart-artifact-view-statesSlices 1 and 2 together reproduce #710's backend byte-for-byte
(
git diff origin/feat/chart-artifact-ui-view feat/ess-chart-artifact-plot-specs -- cmd/experiment_state_server/is empty).
TAG=agy
CONV=96ac94bd-c2b9-4753-b54f-b0b6b02d801f