feat!: stop one / experiments request from reading the whole store (Layer 3 - 10) - #1660
Open
Manik-Khajuria-5 wants to merge 64 commits into
Open
feat!: stop one / experiments request from reading the whole store (Layer 3 - 10)#1660Manik-Khajuria-5 wants to merge 64 commits into
Manik-Khajuria-5 wants to merge 64 commits into
Conversation
Add vis.experiment/log_metrics/finish_experiment client methods that POST to a new /experiments/log Tornado handler. The handler records metadata through ExperimentStore over the server's DataStore and mirrors the blob into in-memory env state so a later full-env save preserves it. Covers create/update, metric append+autocreate, and finish (finished/failed). Once an experiment is terminal, further log/metrics writes are rejected: the store raises ExperimentFinishedError and the handler maps it to 409 Conflict, so a finished run's recorded data cannot change after the fact. Validation returns 400 (bad action/params/status), 404 (finish without experiment), 409 (write to terminal). Adds end-to-end + client-shape tests.
Add py/visdom/experiments/query.py: a small, injection-safe query
language ('lr < 0.01 AND acc > 90') that tokenizes and parses into a
predicate AST evaluated as a pure Python walk over a dict (no eval/exec,
no SQL). Supports < <= > >= = != contains, AND/OR with correct
precedence, parentheses, and dtype-aware casting (numeric/string/bool).
build_record() flattens an Experiment into a queryable dict with bare
and namespaced (param./metric./tag.) keys.
Exported from the experiments package. Covered by
py/tests/test_query.py (45 tests: tokenizing, grammar, precedence,
type handling, malformed input, injection-style strings, build_record).
build_record() was annotated `experiment: Any`, justified in its docstring as keeping query.py free of storage dependencies. That rationale was wrong: models.py imports only stdlib and never touches store.py, so importing it costs nothing and cannot cycle. The real cost of `Any` was an invisible, uncheckable contract. Replace it with an ExperimentLike Protocol (structural, so Experiment satisfies it without importing or inheriting anything) plus a _KeyValueLike protocol for Param/Metric/Tag. params/metrics/tags are declared read-only via @Property: as plain attributes they are invariant, which rejects list[Param] against Sequence[_KeyValueLike] and would have failed the concrete Experiment. Collect metrics in one reverse pass instead of a set comprehension plus an O(keys x metrics) latest_metric() re-scan per key. Semantics are unchanged, including latest meaning last logged rather than highest step. Document the two behaviours the code relied on but never stated: bare and namespaced keys are duplicated deliberately (in-memory, per query; the persisted shape is still to_dict()), and bare-name precedence runs built-ins > params > metrics > tags. Tests 45 -> 48: bare-name precedence across a param/metric/tag collision, latest-metric ordering, and a non-Experiment fake proving the decoupling.
Adds the search layer on top of the L2-3 query parser, so the experiments logged by /experiments/log can actually be found again. ExperimentStore.search(query, sort_by, descending) filters experiments through the parser's predicate and sorts them (newest first by default). Records are built once per experiment and reused for both the filter and the sort, and a run missing the sort field always lands last rather than jumping to the front when the sort is reversed. Mixed-type fields sort without raising, since params are user-supplied. POST /experiments/search wires that to HTTP: it validates query/sort_by/ limit/offset/descending, pages the result, and reports the unpaged total so a caller can walk the pages. A malformed query is a 400 with the parser's reason. Queries stay evaluated in Python -- never eval'd, never SQL -- so an injection payload is a parse error, not an execution. Client side, vis.search_experiments(query, limit, offset, sort_by, descending) returns the server's reply; the JSON-decoding plumbing is factored out of _experiment_send so both endpoints share one path. Endpoint documented in openapi.yaml. Tests: py/tests/test_experiment_search.py (42) covering the store filter/sort, the endpoint e2e over a real Application, validation and paging, and the client message shape.
Remove the inline # comments PR-4 added and fold the useful context into docstrings instead, matching the experiments package style. No behaviour change.
Layer 2 continues: after search, ask what actually differs between the runs it found. New visdom/experiments/compare.py holds the diff itself, pure and free of storage the way query.py's build_record is: build_comparison() lines the experiments up and reports, per section (params/metrics/tags), the union of fields, the shared ones every run agrees on, the differing rest, and the per-run values. Metrics diff on their latest observation, the same value a search compares on, so a run found by "acc > 0.9" shows that acc here. A field only some runs carry is a difference rather than a consensus among those that have it. Two comparisons are deliberately not ==. Bools are not numbers, so amp=True and amp=1 differ, matching query.py's refusal to treat a bool as a number. NaN agrees with itself, since a metric NaN in every run is not a difference and calling it one would bury the real ones. ExperimentStore.compare() selects the runs and delegates. Selection is by name or by query, mutually exclusive: both or neither raises rather than guessing which was meant. env_ids compares in the order given, dedupes, and raises KeyError naming any id without an experiment -- a comparison silently missing a run it was asked for reads as a comparison of the rest. A query selects via search() ordered by sort_by/descending and capped by limit; matching nothing is an empty comparison, not an error. ExperimentCompareHandler (POST /experiments/compare) mirrors the search handler's shape and maps those to 400/404. It rejects a bare-string env_ids, which would otherwise iterate into a comparison of runs "r", "u", "n". The three request validators move from ExperimentSearchHandler statics to module level so both handlers share one copy. Client: vis.compare_experiments(env_ids=None, query=None, limit=None, sort_by=None, descending=True). Documented in openapi.yaml (compareExperiments + ExperimentComparisonSection schema). Tests: py/tests/test_experiment_compare.py (50 -- pure diff, store selection, endpoint e2e, validation, client shape); 414 py/tests pass. Verified live against a real server and client: both selection modes, limit, 404/400 paths, injection payload inert with data intact.
Fold the example's trailing # annotations into the prose instead, matching the experiments package style. No behaviour change.
The stubs cover experiment/log_metrics/finish_experiment but stop there, so search_experiments (L2-4) and compare_experiments (L2-5) were the two client methods a type checker could not see. env_ids is spelled Union[List[Text], Tuple[Text, ...]] rather than the obvious Sequence[Text]: a bare str is itself a Sequence[str], and compare_experiments rejects one at runtime, so Sequence would have a checker bless the very call the client raises TypeError on. A tuple is accepted, hence the union rather than a plain List. Both replies are the decoded JSON of their endpoint, named _ExperimentReply for the Mapping the existing experiment stubs return. Verified the stub parameter names and defaults against inspect.signature of the real methods; no type checker is configured in this repo, so nothing else guards the drift.
Compare took either env_ids or a query. The query mode was redundant: search already answers "which runs match?", so compare's copy of it was a second way to do the same thing, reachable only by duplicating search's syntax, sorting and paging into a second endpoint. It also carried a caveat that could mislead. limit truncated the compared set, so shared/differing were computed over only the runs that survived the cap -- correct, but easily read as a diff of everything matching. Compare is now purely "diff these runs": compare(env_ids). To compare a query's matches, search first and pass the ids on, which is one extra call and keeps the diff honest -- it always describes exactly the runs named. The two endpoints now have one job each. Removed with it: the mutual-exclusion checks (nothing to be exclusive with), sort_by/descending/limit on compare, and the QueryParseError path. env_ids is now required, so a missing one is a 400 rather than a fallback to query. The three request validators move back from module level to ExperimentSearchHandler statics, since compare no longer shares them -- web_handlers.py is byte-identical to L2-4 apart from the new handler. Client compare_experiments(env_ids) loses its optional selection knobs; __init__.pyi follows, and _EnvIds drops its Optional now that env_ids is required. Tests: the query-mode cases go; added that search-then-compare composes, that a stale caller still sending query/limit is ignored rather than a 500, and that a traversal env id degrades to 404 rather than a file read (JSONStore._primary_path already guards this) -- the latter replacing the injection test, whose parser surface compare no longer has. 398 py/tests pass. Verified live against a real server and client: compare, search-then-compare, the 400/404 paths, and the traversal id.
shared/differing was all-or-nothing: a field counted as shared only if
every run agreed, so with run-a lr=0.1, run-b lr=0.001, run-c lr=0.1 the
answer was just "lr differs". That run-a and run-c actually match was in
the values map, but the reader had to spot it.
Each section now also carries groups: per field, the runs clustered by the
value they used.
"lr": [{"value": 0.1, "env_ids": ["run-a", "run-c"]},
{"value": 0.001, "env_ids": ["run-b"]}]
shared/differing stay as the at-a-glance "what changed?"; groups answers
the finer "which runs agree?". They cannot disagree: a field is in shared
exactly when its groups are a single cluster holding every compared run,
and shared is now derived from the groups rather than computed twice.
Grouping deliberately does not use a dict keyed by value. Values need not
be hashable (a param may hold a list), hash(True) == hash(1) with True == 1
so a dict would silently merge them and undo the bool rule _same_value
exists to enforce, and NaN never equals itself so it would never group.
_group_values scans with _same_value instead, keeping one definition of
sameness for the module; the run count per comparison is small.
Groups are ordered by first appearance and env_ids within a group keep the
compared order, so the output is deterministic. A run that never logged the
field is in no group for it.
Tests: 7 new in TestBuildComparison (clustering, shared<->groups agreement,
missing field, bool-vs-1, NaN, unhashable lists, ordering) plus a JSON
round-trip through the endpoint. 406 py/tests pass. Verified live: three
runs on two learning rates cluster as intended, including list-valued
params, with shared and groups agreeing on every field.
Reserve /experiments/suggest for the next-run hyper-parameter suggestion
strategy (Optuna-backed), which belongs to a later layer. The endpoint is a
stub: it parses the request like its siblings and replies 501 Not Implemented
with a JSON body ({"status": "not_implemented", "suggestion": null, ...}) so a
caller gets a stable, decodable answer it can tell apart from a real result.
- ExperimentSuggestHandler (web_handlers.py) + route (app.py), mirroring the
log/search/compare handler shape (static wrap_func, @check_auth post).
- Visdom.suggest_experiment(params=None, env=None) client method + .pyi stub;
type-checks params and posts the search space through for the eventual
strategy.
- openapi.yaml: /experiments/suggest documented (operationId suggestExperiment,
501 stub response schema).
- test_experiment_suggest.py: endpoint 501/JSON-body contract and client
message shape.
The Layer-2 experiments client API had no README coverage (L2-1..L2-5 were local-only with docs deferred here). Add an Experiments section — both the API list entry and the per-method Details — covering the full workflow: experiment / log_metrics / finish_experiment / search_experiments / compare_experiments, plus suggest_experiment documented honestly as a reserved 501 stub. Argument names and defaults match the client signatures.
Add py/visdom/data_model/README.md describing the storage abstraction: the DataStore interface (env/layout/undo operations), the JSONStore backend (on-disk layout, in-memory mode, id sanitisation/path-traversal guard, long-id hash fallback, byte-stable JSON, atomic undo writes), how it is wired through Application.storage, and how to add a new backend. Point the env-persistence skill at the layer, which previously referenced only the old serialization path.
Add the client-side entry point for the Layer 3 hyper-parameter view. `vis.hparams()` gathers experiments through the existing `experiments/search` endpoint (same query syntax, optional `env_ids` filter), flattens them into a compact records payload via the new module-level `_flatten_experiments` helper, and creates a dedicated `hparams` window that renders from its content like the properties/embeddings panes. `_flatten_experiments` collapses each run's params/metrics lists into per-run maps, keeping only each metric's latest value, and returns the sorted param/metric key unions so the frontend renders a table/parallel-coordinates view without re-deriving columns. Includes the `.pyi` stub for the new method and pytest covering the flatten helper (unions, latest-metric, heterogeneous/empty/malformed input) and the client message shape (window type, env/win pass-through, env_ids validation and ordering).
Add a `mode` argument to `vis.hparams` ("query" | "env_ids" | "both",
default "both") so the caller explicitly chooses how the shown runs are
selected: by query alone, by an explicit env_ids list alone, or the
intersection of the two. mode="env_ids" requires env_ids and skips the
query; mode="query" ignores env_ids.
Include tags in `_flatten_experiments`: each run now carries a tags map
alongside params/metrics, and the payload exposes a sorted tag_keys union,
matching the server-side build_record which also flattens tags.
Switch the flatten test fixture to TemporaryDirectory with a tearDown so
the seeded JSONStore is removed after each test instead of leaking temp
directories, and extend coverage for the mode paths and tag flattening.
Previously the env_ids selection always went through search_experiments, which reads every environment on disk and returns every experiment over the wire, only for the client to keep the few it named. When no query is in play, route the selection through compare_experiments instead, which reads only the named environments and returns just those runs; a query, when present, still uses search. Also type-check query up front and treat a blank/whitespace query as no query so it takes the by-id path too. Tests stub both read endpoints and assert which one each mode reaches.
Make the three selection modes strict and mutually exclusive in their
arguments instead of leniently combining them:
query -> non-empty query, no env_ids; fetched via search
env_ids -> non-empty env_ids, no query; fetched via compare (reads only
the named environments)
both -> both required and non-empty; search then narrow by env_ids
mode defaults to None and is inferred from which of query/env_ids are
given; an explicit mode enforces its rule and rejects the wrong argument.
A blank or whitespace-only query counts as no query. There is no
"show everything" call: with neither query nor env_ids a ValueError is
raised.
Move the hyper-parameter selection, flattening and window creation off the Visdom.hparams client and into a POST /experiments/hparams handler. The handler resolves the strict query/env_ids/both modes the client used to resolve itself (invalid combinations are 400s), flattens the selected runs via visdom.experiments.flatten_experiments, and writes an hparams window with that content into the env state. For now the handler only writes to state; it does not broadcast the window to connected clients, since the frontend has no dedicated hparams pane to render it yet. The pane is served on the next env load, and broadcasting lands with that pane. vis.hparams is now a thin call: it validates opts and posts query/env_ids/mode/win/opts to the endpoint, returning the created window id, instead of gathering runs and posting to the events endpoint itself. window() learns the hparams type so the content is stored as-is rather than as a plot.
Register an `hparams` pane so `vis.hparams()` windows are drawn in the browser instead of falling through to the plot fallback: - js/panes/HParamsPane.js: functional pane (PlotPane-style React.memo) reading the flattened records payload from window content; renders empty / error / summary states and leaves an .hparams-views seam for the table / parallel-coordinates / SPLOM / filter views of later PRs. - js/settings.js: register the pane in PANES and PANE_SIZE. - py/visdom/static/css/hparams.css + index.html link: pane styling. Switch the /experiments/hparams handler from the persist-only _store_window to the standard register_window, so the pane is broadcast to connected clients and appears live (broadcast was deferred until this pane existed).
Replace the placeholder run list in the hparams pane with HParamsTable, mounted in the .hparams-views seam. Adds: - hparamsUtils: pure sort/format/filter/color helpers; the comparator mirrors the backend order rule (numbers before strings, missing/NaN last in both directions) so the table and server-side search agree. - Column-header sorting plus a tree 'sort by' dropdown with a 3-state direction toggle (asc/desc/off); active sort column is highlighted. - Client-side text filter and per-run selection checkboxes. - 'color by' shades a numeric param or metric column on a ramp off the Visdom blue (#3b5998). - Both dropdowns reuse rc-tree-select to match the environment selector.
- Add a centered, larger pane title: the experiments handler defaults the hparams window title, and Pane tags its bar with a per-type class so the hparams title can be styled without touching other panes. - Draw solid vertical dividers at the param/metric/tag group boundaries, aligned across the header band, header row, and body. - Fix the double horizontal scrollbar by scrolling only the inner table and adding min-width:0 to the flex chain.
…and color-by Adds a Table | Scatter matrix switcher to the hyper-parameter pane and a new HParamsSplom view that renders a Plotly splom trace from the window records. Users pick up to six numeric param/metric axes and an optional color-by metric (same light-to-dark ramp as the table color spine). NaN/missing values are dropped so empty axes never appear. Reuses hparamsUtils for column building and numeric detection; global Plotly and the PlotPane resize pattern; no new deps and no backend/API changes.
…ree views Centralize the numeric-column selection, grouped TreeSelect data, run labelling, and numeric-or-null mapping in hparamsUtils, and move the Plotly-facing snapshot download, notifier, snapshot mode-bar button, and resize observer into a new hparamsPlot module. The table, scatter matrix, and parallel coordinates views now consume these instead of each carrying their own copy.
The top margin was 30px, so Plotly drew the axis titles above the plotting area and the container clipped them, leaving the names half-cut. Widen the margins so titles, the top/bottom range values, and the colorbar all have room, and pin the label, tick, and range font colors for consistent contrast.
The hparams views grew their own copies of the same things as each view landed. Pull them into one place: - ColumnSelect / ColumnMultiSelect replace four hand-rolled rc-tree-select blocks that differed only in value and tree data. - HParamsMessage replaces eleven wrap-plus-message div pairs. - useHParamsColumns replaces three identical buildColumns memos. - plotRevision builds the datarevision key the three plots each concatenated by hand; the splom now derives its axis style from plotAxisStyle instead of restating it; the pane downloads through hparamsExport's downloadText, which also stops it leaking the object URL. - One sortGlyph for the caret and the direction button. Also give a single-observation run a visible marker: a run with one logged value drew nothing under mode 'lines', so the metrics plot read as empty.
The centred title rule matched every .pull-right in the pane bar, so the comment button was stretched across the bar and its icon landed on top of the title. Scope the rule to the title div and leave the buttons floating.
ExperimentStore read the env it was about to persist from disk, so every metric log rewrote the env file from a snapshot that was missing windows created since the last save and still carried windows the user had closed. The store now takes an env_provider (the server passes state.get) and prefers the live env for both reads and writes; the env may be a LazyEnvData, so it is mutated through the mapping interface (gaining __delitem__ for experiment deletion) and never copied. Logging now persists the environment the server is actually serving, panes included.
The generic /update route only understands plot-shaped content, so an hparams pane could not be changed without re-registering the whole window. POST /experiments/hparams/update rebuilds an existing hparams window in place: a query/env_ids/mode body replaces its selection under the same validation as create, and a bare win re-runs the selection now stored on the window (a manual refresh). The rebuilt window keeps its id and position but mints a fresh contentID, is broadcast to subscribers, and the env is saved so disk reflects the update immediately. Creating a pane also saves the env right away instead of waiting for the next explicit save.
Thin wrapper over the experiments/hparams/update endpoint, next to vis.hparams: pass a selection to replace the pane's one, or only the window id to re-run the stored selection and pick up runs logged since the pane was built. win is required client-side; opts are validated like the other plotting methods and override the pane's title/size when given.
/experiments/hparams was never added to openapi.yaml when the endpoint landed; document it together with the new /experiments/hparams/update, in the style of the other experiment paths. README gains the matching vis.hparams and vis.update_hparams entries next to the other experiment methods, and the type stub gains update_hparams.
A readonly server was accepting every experiment write: /experiments/log created and overwrote metadata, and both hparams endpoints registered windows and saved envs to disk. Only /upload_env was guarded, and it was guarded by an inline check that each new write handler had to remember to copy -- which is why the check was lost twice while these endpoints were being built. Make the guard something a handler declares rather than restates. reject_readonly(message) joins check_auth in server_utils as a decorator on the write entry point, "readonly" moves into the app attributes every web handler is given (socket handlers already had it), and the four write endpoints are switched onto it, upload_env included, so a single spelling of the guard remains in the tree. The refusal is checked before the request is validated, so a rejected hparams update cannot be used to probe which windows exist. Covered by py/tests/test_readonly_guard.py: every write endpoint refuses and leaves disk untouched, the read endpoints (search, compare) still answer, and the same requests succeed with readonly off. openapi.yaml documents the 403 on all three experiment paths.
tipHtml() builds markup that is assigned to innerHTML, and escaped every interpolated value except the two coordinates, which went in as raw formatValue() output. Those coordinates are numeric today, so nothing escapes through in practice -- but that is a property of the caller, not of the function. formatValue() ends in String(value) and returns whatever it is handed, so the safety of the tooltip rested on an upstream fact rather than on anything at the sink. A string-valued column, or a different caller, turns a logged param into markup in the dashboard. Escape both, so every value in the string is escaped by the same rule.
Searching visits every environment, and every visit went through the full env: ExperimentStore._read asked the server's state for the env and took "experiment" out of it. For a LazyEnvData that single lookup parses the entire env file -- every window, every encoded image -- to reach a few hundred bytes of metadata, and caches it on the object the server holds. The cost is therefore not transient. One search permanently converts every lazily-loaded env into a resident one, which is exactly what LazyEnvData exists to avoid. Measured on 200 envs of 100 KB each, searching for 29 matches: peak 23.1 MB, 20.8 MB still held afterwards, 200/200 envs materialised. After this change: peak 2.5 MB, 0.1 MB held, 0 materialised. Add DataStore.load_experiment(eid), a projection returning just the metadata blob, and implement it in JSONStore by taking the blob and dropping the rest. It is abstract like the rest of the interface rather than defaulting to load_env: a default would be correct everywhere and right nowhere, since a backend that reads whole environments would inherit it silently and do the one thing the projection exists to avoid, looking fine until someone measured it. Reads that want metadata alone go through a new ExperimentStore ._read_metadata, which prefers a live env only when it is already resident (LazyEnvData answers via a new is_loaded()); an env nobody has materialised cannot hold unsaved changes, which is the same invariant serialize_env already relies on when it skips those envs on write. Writes are untouched and still go through the live env. list_experiments() keeps its contract but is now a list() of a new iter_experiments() generator, so a filtering caller can collect what it rejects instead of holding the whole store.
search() held every match, plus a flattened record per match, for the whole scan: it built the record list first and filtered second, so a query matching one run out of thousands still materialised thousands of records before discarding them. Records are several times the size of the experiment they describe -- build_record duplicates every param, metric and tag under both a bare and a namespaced key -- and only the sort value outlives the comparison. Restructure the scan around that. _scan() builds one record at a time, takes the sort value, and drops it; a non-match is not retained at all. Add search_page(), which returns a page and the unpaged total, and give _scan a keep bound: the working set is trimmed back to the best offset+limit entries whenever it doubles, since an entry already outside the top k cannot re-enter it, and the value-less tail stops growing at the same bound. A request's memory is then set by the page it asked for rather than by how much the server stores. total stays exact -- it is a counter, and every match must be visited anyway to know it matched. Ranking moves from a stable list.sort to heap selection, which is not stable, so the arrival counter becomes part of the key and is negated for descending sorts. Without that, reversing the comparison would also reverse tied runs, and a page of equally-scoring runs would reorder itself depending on the sort direction. search() keeps its signature and its meaning (every match, sorted) for the pane path and existing callers; it is now _scan without a bound. py/tests/test_experiment_search_paging.py holds the equivalence that makes this safe to trust: a page equals the same slice of the unbounded result, at every offset, in both directions, across the boundary where sorted runs give way to value-less ones, and with enough ties to force repeated trims.
The endpoint documented "null returns all matches", and meant it: an explicit null limit sliced the entire result set, so one request could ask the server to rank and serialize every experiment it stores. The default of 100 was the only thing standing between a caller and that, and a caller chooses the limit. Add MAX_LIMIT (1000, matching what comparable tracking servers default to) and route the endpoint through the store's new search_page, so the scan retains a page rather than everything. null now means "as many as allowed" and a larger limit is coerced down to the cap rather than refused -- the AIP-158 rule, and the one that cannot turn a working integration into a failing one. The reply's limit field reports what was actually applied, so a capped caller can see it was capped; total still counts every match. _require_index grows an optional maximum instead of a second validator, since offset is deliberately left uncapped -- it costs nothing to skip. openapi.yaml documents the cap, the coercion and that the reply's limit is no longer nullable; the client docstring stops promising that limit=None returns everything and says to page with offset instead.
The comments added alongside search_page said what the code beside them already says: that a bounded scan trims its working set, that the unsorted branch takes the first entries it sees, that a page slice ends at the window. Each one restated its own line rather than explaining why the line is what it is, which is the only thing a comment can add that the code cannot. The reasoning worth keeping was already in the docstrings, where it describes the contract rather than the statement underneath it.
Capping limit bounded the page but not the request. A page at an offset is found by ranking every match above it and discarding all but the last limit of them -- search_page asks the store to keep offset + limit entries -- so the cost of a request is its depth, not its page. An offset of ten million with a limit of one asked the server to rank ten million entries to return one, a larger working set than the uncapped limit the cap was added to prevent, from a smaller body. MAX_WINDOW bounds offset + limit, checked on the pair because the pair is what the store keeps. Capping offset alone would leave the same hole open at a large enough limit, and capping neither leaves it open at any. The window is refused rather than coerced, unlike limit. A coerced limit returns fewer experiments and reports what it applied, so a caller can see what happened; a coerced offset would return a page other than the one asked for, and nothing in the reply would say so -- the caller would read someone else's page as its own. Reaching past the window is a sign the query is too broad, so the message says to narrow it. The window is checked after limit has been resolved, so a null limit cannot buy a deeper page than the cap it was coerced to.
/experiments/compare named its runs explicitly, so the request grew with the ask and looked self-limiting. It is not: an env_id is a few bytes and the run behind it is a whole environment, read from the store and echoed back in full, so a short list of ids is a long reply and a large read. MAX_ENV_IDS bounds the list, checked before the ids are read since loading them is the cost being bounded -- a check after the load would also answer 404 for the unknown ids and hide the real reason. It is set to search's MAX_LIMIT so that any single page of search results can be handed straight to compare, which is the flow the endpoint documents; a test asserts that relationship rather than the number, so the two cannot drift into a documented flow the server refuses. The list is refused rather than truncated, for the reason a missing run is already a 404: a diff of some of the runs asked for is a different answer, not a smaller one, and no field in the reply would reveal the substitution. The HParams pane sends one id per row, so a pane over a large sweep could have exceeded the cap. It now batches, merging only the experiments out of each reply, which is all it ever read. The batching is in the pane rather than in fetchExperimentComparison because a comparison is a diff: the params/metrics/tags sections of a batch describe that batch, and merging them would quietly answer a question nobody asked.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @Manik-Khajuria-5, your pull request is larger than the review limit of 150000 diff characters
Manik-Khajuria-5
requested review from
Jayantparashar10,
Saksham-Sirohi,
marcoag,
mariobehling,
norbusan,
rajnisht7,
tonypzy and
vedansh-5
July 29, 2026 15:15
Contributor
Reviewer's GuideAdds a full experiments API (log/search/compare/suggest/hparams) across backend, Python client, server handlers, JS UI and OpenAPI docs, and refactors experiment storage to be metadata-only for reads with bounded search paging; also introduces a readonly write guard and a new hparams visualization pane. Sequence diagram for creating a hyper-parameter pane (hparams)sequenceDiagram
actor User
participant Visdom as Visdom.hparams
participant H as ExperimentHparamsHandler.post
participant ES as ExperimentStore
participant DS as DataStore_JSONStore
participant WS as Env_state\n(register_window)
participant UI as HParamsPane
User->>Visdom: hparams(query, env_ids, mode, win, env, opts)
Visdom->>H: POST /experiments/hparams
H->>H: _resolve_spec(query, env_ids, mode)
H->>ES: ExperimentStore.search(query)
ES->>DS: iter_experiments() via\nlist_envs / load_experiment
DS-->>ES: Experiment objects
ES-->>H: matching experiments
H->>H: flatten_experiments(experiments)
H->>WS: window({type: hparams,\ncontent, hparams spec})
WS->>DS: save_env(eid, env)
DS-->>WS: persisted
H-->>Visdom: win id (text/plain)
Visdom-->>User: win id
User->>UI: open pane for win
UI->>UI: render HParamsPane\nfrom window.contentID and\nrecords/param_keys/metric_keys/tag_keys
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Manik-Khajuria-5
changed the title
4 tasks
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
1 participant
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.
Description
/experiments/search:MAX_LIMIT(1000) caps the page,MAX_WINDOW(10000) capsoffset + limit./experiments/compare:MAX_ENV_IDS(1000) caps the id list; the HParams pane batches, so large panes still work.limitis coerced since the reply reports it;offset/env_ids400, since a changed page or partial diff is a different answer, not a smaller one.Fixes : #1639
Integration note
This PR should be merged after #1656 , as it depends on the lower-layer changes introduced there.
Motivation and Context
Every experiments read was unbounded somewhere
limit: nullreturned everything, compare's id list was unlimited, paging depth sized the heap so a small request body could drive server memory.How Has This Been Tested?
py/tests/green (~530 tests, Python 3.12.13) with new coverage for the caps and the window boundary;yarn lintclean,yarn buildcompiles.Types of changes
Breaking:
limit: nullnow returns up to 1000, not every match, and is no longer nullable in the reply.offset + limitover 10000, and compare over 1000 ids, now 400 page withoffsetor narrow the query.Checklist:
py/visdom/VERSIONaccording to Semantic Versioning — left to the maintainers' release commit, butlimit: nullis not backwards compatible.openapi.yamland the client docstrings).Summary by Sourcery
Introduce server-side experiment metadata APIs with bounded search and comparison, add a hyper-parameter pane UI, and ensure readonly mode blocks all write endpoints while preserving env persistence.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: