Skip to content

Render-ready codec: fix save() call, store data-only Zarr, add FigpackRef.serve_under#2

Open
MilagrosMarin wants to merge 10 commits into
mainfrom
feat/render-ready-codec
Open

Render-ready codec: fix save() call, store data-only Zarr, add FigpackRef.serve_under#2
MilagrosMarin wants to merge 10 commits into
mainfrom
feat/render-ready-codec

Conversation

@MilagrosMarin

Copy link
Copy Markdown

Makes the codec usable end-to-end against figpack ≥ 0.3 and DataJoint ≥ 2.3, and adds the
serving seam that dash-datajoint-components' PlotGrid consumes (design of record:
docs/design-specs/figpack-integration.md in dj-sciops/dash-datajoint-components, merged as #106).

Fixes

  • encode() raised TypeError on every insert: figpack ≥ 0.3 made title a required
    keyword-only argument of FigpackView.save(). The suite's own test_encode_produces_metadata
    reproduced this once the fixtures were updated to figpack 0.3.20 (views no longer take
    title/description constructor kwargs either — the codec reads them from optional view
    attributes via getattr, which now the fixtures exercise).
  • Store holds figure data only: encode() now uploads just the data.zarr/ subtree of
    figpack's save output (consolidated .zmetadata carries title/description), not the full
    viewer bundle — the viewer is laid over the data at render time, so the store never
    duplicates ~2 MB of viewer code per figure. This follows the direction from the #106 design
    review; please confirm this is the storage shape you want.
  • validate() rejects extension-based views (ExtensionView): their JavaScript lives
    outside data.zarr, so data-only storage would silently drop it and serve broken figures.
    Note this includes figpack.views.PlotlyFigure, which subclasses ExtensionView — a test
    canary documents that. Open question: acceptable v1 policy, or should extension files be
    stored alongside the Zarr?

New: FigpackRef.serve_under(base_dir) -> str

Materializes a self-contained, servable viewer bundle under base_dir/<sha1(path)[:16]>/
(viewer dist from the installed figpack package + downloaded data.zarr + extension manifest)
and returns /<id>/index.html. Idempotent; concurrency-tolerant (a worker losing the publish
race to a byte-equivalent bundle treats it as success); staged + os.replace-published so a
half-written bundle is never served; refreshes the directory mtime for TTL-based cache
cleaners; sweeps staging dirs orphaned by killed processes. Downloads use
StorageBackend.fs.get(..., recursive=True) for non-file protocols (StorageBackend has no
get_folder) — the S3 path is covered by a mocked test but not yet validated against a real
remote store.

Flagged, unchanged (proposing a follow-up issue)

FigpackRef.load() has two pre-existing defects: figpack.view_figure() serves the figure
over HTTP and returns None (it never reconstructs a view, so load() blocks and caches
None), and its remote branch calls backend.get_folder, which DataJoint 2.3's
StorageBackend does not provide. The dashboard path never calls load(); left untouched
here to keep this PR focused.

Notes

  • Carries over the applicable items from the dj-canvasxpress-codecs#1 review: the LICENSE
    body is restored byte-verbatim from the official Apache-2.0 text (this repo carried the
    same two deviations and was named there as the likely copy source), storage-path docs now
    describe a framework-owned schema-addressed layout, and validate() raises TypeError
    for unsupported types per the codec convention (the missing-figpack branch stays a
    DataJointError — an environment problem, not a value problem).
  • Dashboards installing this package get figpack transitively (it is a hard dependency), so
    the viewer dist is always importable serve-side. If a figpack-free dashboard is ever wanted,
    figpack could become an optional extra.
  • Tests: 23 passed, 2 skipped (live-DB round-trip incl. to_arrays returns the ref — needs
    DJ_HOST/DJ_USER/DJ_PASS; plotly canary needs plotly). Includes a round-trip through the
    real file-protocol StorageBackend with no DB, guarding against backend API drift.
  • Version 0.1.0 → 0.2.0.

- figpack >= 0.3 removed title/description from view constructors (save()-time
  metadata now); the fixture sets them as plain attributes, which encode() reads
  via getattr().
- datajoint >= 2.3 _build_path validates the store (get_store_spec), so encode
  tests configure a 'default' file store even with the backend mocked.
- Publish race: two workers cold-rendering the same figure no longer error —
  the loser detects the winner's completed bundle (os.replace ENOTEMPTY) and
  returns success; only stale PARTIAL dests are removed before publish.
- Guard the TTL utime against a concurrent swap (FileNotFoundError).
- Sweep staging dirs orphaned by killed processes (dot-prefixed, >1h old).
- Tests: race loser succeeds; orphan sweep; failure leaves no litter/partials;
  real StorageBackend round-trip WITHOUT DB (guards backend API drift that
  MagicMock tests can't see); title-less view encodes (title=''); description
  propagates into zarr attrs; PlotlyFigure-is-ExtensionView canary + plain-view
  acceptance.
The license body deviated from the official apache.org text in two places
(a rewritten §8 Limitation of Liability and an inserted word in the
Contribution definition), which makes the file not the Apache-2.0 license.
Body is now byte-identical to https://www.apache.org/licenses/LICENSE-2.0.txt
through END OF TERMS AND CONDITIONS; the instructional APPENDIX stays dropped
and the Copyright 2026 DataJoint Inc. notice is kept. Same defect was flagged
on dj-canvasxpress-codecs#1 review, which named this repo as carrying it too.
The actual path comes from DataJoint's build_object_path: attr=value primary-key
segments and a tokenized {attribute}_{token}.zarr filename, subject to store
prefix/partitioning — the old {schema}/{table}/{pk}/{attribute}.zarr notation
implied a stable, hand-buildable layout. (Same alignment note as on
dj-canvasxpress-codecs#1.)
…ion)

Adopts the documented codec convention (TypeError for wrong types, matching
AttachCodec/FilepathCodec) flagged on the dj-canvasxpress-codecs#1 review.
The missing-figpack ImportError branch stays a DataJointError — an
environment problem, not a value problem.

@dimitri-yatsenko dimitri-yatsenko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Strong PR — approving in spirit, with a few small fixes to land first. It fixes a real total breakage (encode() raised TypeError on every insert under figpack ≥ 0.3), the data-only storage shape is well-reasoned, and serve_under is carefully built: the staged-build + atomic os.replace publish, race-loser detection, orphan sweep, and TTL touch are all correct and genuinely well-covered (the monkeypatched publish-race test is a nice touch).

I verified the backend-API claims against datajoint 2.3: put_folder, _full_path, fs, and protocol all exist, and get_folder really does not exist anywhere — so the diagnosis of the broken load() path is accurate.

Should-fix before merge

  1. __version__ not bumped. src/dj_figpack_codecs/__init__.py still declares __version__ = "0.1.0" while pyproject.toml is now 0.2.0. It's exported in __all__, so dj_figpack_codecs.__version__ disagrees with the installed metadata.

  2. Docstring examples break on the version this PR targets. __init__.py (vv.TimeseriesGraph(title="My Plot")) and the codec.py class docstring (TimeseriesGraph(title="Spike Raster")) still pass title= to the view constructor — but the premise of this PR is that figpack ≥ 0.3 removed title/description from view constructors (the fixtures now set fig.title after construction). These snippets would raise TypeError on 0.3. Please update them to the getattr-based attribute path.

  3. load() / show() are non-functional — make the failure legible. The class's headline API (featured in the __init__ example as ref.show()) is broken: the remote branch calls self._backend.get_folder(...)AttributeError (no such method in dj 2.3), and the file branch caches whatever view_figure() returns. Deferring the real fix is fine to keep scope tight (see the follow-up issue), but at minimum have load() raise a clear NotImplementedError/DataJointError that points at serve_under, rather than an AttributeError or a silently cached None.

Top thing to verify (already flagged in the description)

  1. fs.get(full_path, dst, recursive=True) remote semantics are unvalidated. fsspec's recursive get is trailing-slash-sensitive about whether contents land as dst or nested inside it. The remote test mocks fs.get and asserts the call, not the resulting on-disk layout — so a real S3/GCS store could produce data.zarr/<name>/... and serve a broken figure. Worth a real-remote smoke test before relying on remote serve_under.

Nits

  • mock_backend still defines get_folder (conftest) — a method the real backend lacks. Harmless today (nothing exercises load()), but it perpetuates the illusion the API exists and would mask #3. Drop it or mirror the real fs.get.
  • Unused DataJointError imports in the test_codec.py cases that assert pytest.raises(TypeError, ...).
  • validate() docstring still says "Raises DataJointError if value is not a FigpackView", but the body now raises TypeError.
  • LICENSE now matches canonical Apache-2.0 (the "to Licensor" and §8 corrections are right), but the diff adds a leading blank line at the very top; the canonical text has none — trivial to drop, and worth it given the byte-verbatim intent.

The two decisions you asked about

  • data.zarr-only storage shape → confirmed, yes. Matches the #106 design, avoids duplicating the viewer per figure, and the viewer-over-data-at-render-time seam is clean.
  • Rejecting ExtensionView (incl. PlotlyFigure) in v1 → agree with fail-closed: silently dropping extension JS would serve broken figures, so rejecting at insert time is correct. Filing a follow-up to track extension-file storage, since PlotlyFigure is a plausible ask — not a blocker.

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.

2 participants