feat(presenter): add technical content blocks and artifact source - #4518
Open
taimurrabuske wants to merge 8 commits into
Open
taimurrabuske wants to merge 8 commits into
taimurrabuske wants to merge 8 commits into
Conversation
…pty-plot and column-index validation
…conventions in CONTRIBUTING
…able serial lane The portable CI lanes execute only tests/*.test.sh, so the pytest suites behind presenter.blocks and presenter.artifacts were invisible to CI: a green run would never have executed this slice's own tests anywhere. Add tests/fm-presenter-blocks.test.sh, which builds an ephemeral venv with pinned pytest, NumPy, matplotlib, and SymPy versions and runs both suites through the public pytest interface, asserting the passed count so an empty collection cannot pass. Missing prerequisites hard-fail rather than gate-skip, mirroring the accepted sibling-lane wrapper tests/fm-presenter-render-docx.test.sh. Verified: the wrapper passes via direct run and bin/fm-test-run.sh (52 tests, family unclassified/serial); bin/fm-test-run.sh --check-coverage still proves the serial-shard partition (18/166 unhinted, under the 15% cap); tests/fm-test-run.test.sh passes; bin/fm-lint.sh is clean.
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.
Intent
Start 4 parallel lanes building the presenter presentation and datasheet generation library in this repo, under a new top-level Python package presenter/: template-driven PowerPoint (.pptx) and Word (.docx) authoring, corporate template ingestion, and technical content blocks (schematics, waveform plots, symbolic equations, PVT/spec tables).
This lane is the technical-content-blocks slice: builders for waveform plots, symbolic equations, and PVT/spec tables, plus the artifact-source interface that resolves asset references (with an adapter seam for polymath's SessionArtifactRepository). The other three lanes cover the package scaffold and core data model (presenter/core/ and pyproject.toml), template ingestion and master binding (presenter/templates/), and the DOCX render engine (presenter/render_docx/); a queued follow-up lane wires all four together. Deliver this slice complete with passing tests.
What Changed
presenter/blocks/builders that return JSON-serializable block dicts: waveform/data plots rendered headlessly to PNG via matplotlib Agg from traces, arrays, or CSV (build_plot,build_plot_from_arrays,build_plot_from_csvwith log axes, grid/legend/axis-unit styling); LaTeX equation blocks rendered to PNG via mathtext with optional SymPy conversion (build_equation,build_equation_from_sympy); and tables with sig-fig, fixed-decimal, and SI engineering-notation formatting plus generic, datasheet spec, and PVT corner builders (build_table,build_spec_table,build_pvt_table).presenter/artifacts.pywith theArtifactSourceprotocol — an adapter seam for polymath's externalSessionArtifactRepository— and aFilesystemArtifactSourceimplementation that resolves refs against a root directory with path-traversal guards.tests/presenter/covering artifacts, equations, plots, and tables, and update CONTRIBUTING.md to note this suite sits outside the bashtests/*.test.shconventions used bybin/fm-test-run.sh.Risk Assessment
✅ Low: The fix rounds correctly and durably implement every accepted review decision (each verified by concrete failing-input traces against the current code), regressions were checked and none found, and the only residual items are info-level consistency and test-strength matters safe to address as follow-ups.
Testing
Exercised the presenter blocks slice end-to-end: ran the 52 targeted presenter tests (all pass, via the python3.14 ephemeral venv since the default pytest interpreter can't load the repo's numpy/matplotlib), reproduced each accepted review fix as a before/after behavior change against commit 9ceb244 through the public API, and rendered reviewer-visible product evidence — a CSV-driven waveform PNG, a symbolic equation PNG, a matplotlib-rendered table sheet proving the F1 header-mapping and F4 inf/nan fixes, a well-formed HTML datasheet consuming the block contract, and a before/after visual pair for the negative x_col fix; no failures, no unfixable setup issues, worktree left clean.
Evidence: HTML consumer datasheet rendering all block dicts end-to-end (waveform + equation + 4 tables)
Source: HTML consumer datasheet rendering all block dicts end-to-end (waveform + equation + 4 tables)
Evidence: Before/after behavioral transcript for fixes V1, V2, V3, V4, R1, R2, F2 (public API outcomes only)
Source: Before/after behavioral transcript for fixes V1, V2, V3, V4, R1, R2, F2 (public API outcomes only)
[V1] build_plot_from_csv('time,voltage\n') before : FileNotFoundError: [Errno 2] No such CSV file: 'time,voltage\n' after : ValueError: CSV source contains no numeric data rows [V2] build_plot_from_csv('t,V\n0,foo\n1,bar\n') before : NO ERROR - silently rendered PNG (39,994 bytes) after : ValueError: CSV source contains no finite y values to plot [V3] build_waveform_plot() before : NO ERROR - silently rendered PNG (41,625 bytes) after : ValueError: No plot data provided: pass traces or x and y data [V4] x_col=-1 on a,b file: before renders 2 traces (b leaked) / after renders 1 trace (see v4 pair) [V4] x_col=5: before 'no numeric data rows' / after 'ValueError: x_col index 5 out of range for 2 CSV columns'Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
presenter/blocks/tables.py:384- build_spec_table builds every row in a fixed canonical order ([param, symbol?, min, typ, max, unit, conditions]) while its publicheadersparameter accepts arbitrary user-ordered/renamed header lists, and build_table formats cells positionally. Verified trace: build_spec_table([{'parameter':'IQ','min':1.0,'typ':1.2,'max':2.5,'unit':'mA'}], headers=['Min','Typ','Max','Units','Parameter']) returns rows [['IQ','1.00','1.20','2.5','mA']] — the parameter name lands under 'Min' and every value is shifted/mislabeled, no error. Renamed headers (e.g. 'Min (V)') silently drop the min value and shift typ/max/unit under wrong labels, leaving rows shorter than headers (verified: ['VDO','-','1.2','2.50','mV'] under 6 headers). Earliest durable boundary: derive cells by canonical column name (name→value map indexed by table_headers order) or validate custom headers are a subset of the canonical set and raise otherwise.presenter/blocks/plot.py:312- build_plot_from_csv treats a str argument that does not resolve to an existing file as raw CSV content. A typo'd file path (or a path to a directory, which fails the is_file() check) is parsed as a one-row CSV with zero data rows and silently returns a block pointing at a rendered, completely empty PNG. Verified: build_plot_from_csv('/tmp/typo.csv') returns a block whose PNG exists on disk with no exception. Since the docstring's primary documented usage is a file path, consider raising FileNotFoundError when the string looks like a path (e.g. single line with a path suffix) but doesn't exist, reserving raw-content interpretation for strings containing delimiters/newlines.presenter/blocks/plot.py:19- Module-level matplotlib.use('Agg') in both plot.py (line 19) and equation.py (line 17) forcibly switches the process-wide matplotlib backend on import, overriding any backend a host application (e.g. an interactive session or a future render lane wanting interactive previews) has chosen. Headless is the right default for this library; noting the global side-effect tradeoff since importing a presenter block module mutates global matplotlib state.presenter/blocks/tables.py:106- format_engineering returns Python's lowercase str(float) for inf/nan ('inf', 'nan') while sibling formatters format_sigfigs and format_decimals return 'Inf'/'NaN'. Same input renders with different labels depending on which column formatter handles it (verified via REPL). Align format_engineering to 'Inf'/'NaN' for consistency.presenter/blocks/plot.py:22- The slice's source and tests depend on matplotlib, numpy, sympy, and Pillow, but nothing in the repository declares these dependencies (no pyproject.toml exists yet). Per the stated intent, pyproject.toml belongs to the scaffold lane, so this is expected for now — flagging so the integration lane wires the dependency declarations before this slice's tests can run in a clean environment.🔧 Fix: Fix spec-table header mapping, CSV path errors, inf/nan labels
2 warnings still open:
presenter/blocks/plot.py:283- The fix-round content heuristic (_looks_like_csv_content, used at line 321) still lets a missing path containing a delimiter character silently render an empty PNG, violating the accepted decision that 'a typo'd path must never silently render an empty PNG'. Verified: build_plot_from_csv('data,old.csv') (nonexistent, but contains a comma) is classified as raw content, parsed as one header row with zero data rows, and returns a block pointing at a rendered ~49KB PNG with no exception. A real CSV payload always contains a line break, while a typo'd path essentially never does; restrict raw-content interpretation to strings containing '\n' or '\r' (optionally also rejecting strings with a path separator or file-like suffix) so any single-line missing path raises FileNotFoundError naming the path.presenter/blocks/plot.py:383- Data rows that fail float() parsing are silently skipped (except ValueError: continue), and no check ensures any numeric point was actually parsed, so a CSV that yields zero usable samples still renders an empty plot without error. Verified three concrete inputs, all returning a rendered PNG with no exception: (a) a header-only file ('time,voltage\n'), (b) a file whose data rows all fail float() with the default delimiter (decimal-comma '0,0;1,1' style data - a common real-world CSV variant), and (c) raw content string 't,V\nfoo,bar\n'. The function already raises ValueError('CSV source contains no data rows') for zero parsed rows; apply the same loud-failure invariant after parsing: raise ValueError when x_vals is empty (or every series is empty) instead of handing build_waveform_plot an empty trace.🔧 Fix: Restrict CSV content detection and reject empty parsed data
4 issues (2 warnings, 2 infos) still open:
presenter/blocks/plot.py:288- _looks_like_csv_content strips the input before checking for line breaks (s = text.strip()at line 285, thenreturn "\n" in s or "\r" in s), so a single-line raw CSV payload whose only break is a trailing newline is misclassified as a file path. Verified: build_plot_from_csv("time,voltage\n") raises "FileNotFoundError: [Errno 2] No such CSV file: 'time,voltage\n'", and build_plot_from_csv("0.0,1.0\n", has_header=False) — a valid one-point CSV payload, and exactly the str form Path.read_text yields for a one-line file — fails the same way. This contradicts both the accepted R1 decision (raw-content interpretation for strings containing a line break) and the function's own docstring in build_plot_from_csv. After the misroute, such input can never reach the R2 ValueError('CSV source contains no numeric data rows') path it should hit. Fix: check newlines on the raw text (keep .strip() only for the emptiness guard), so "time,voltage\n" flows into parsing and raises the correct ValueError; extend the tests so 'time,voltage\n' asserts ValueError, not FileNotFoundError.presenter/blocks/plot.py:397- The accepted R2 invariant ('a CSV that yields zero usable samples must fail loudly instead of rendering an empty plot') is only partially enforced: the new guards catch x_vals empty and y_indices empty, but when x parses while every y value is NaN the function still hands build_waveform_plot an all-NaN trace and silently renders an empty plot. Verified two concrete inputs, both returning a rendered PNG (43-45KB) with no exception: (a) raw content 't,V\n0,foo\n1,bar\n' (x numeric, y unparseable), and (b) build_plot_from_csv(file_with_text_column, y_cols=['note']) selecting a non-numeric column. Note the parse loop (lines 386-393) appends exactly one float-or-NaN per series per parsed x row, so the implemented 'y_vals empty' check can never fire when x parsed; the intended 'no usable samples' case escapes it. Earliest shared boundary: after the parse loop alongside the existing checks at lines 397-400, raise ValueError when no parsed y value is finite (all NaN across every series), preserving legitimate partial-NaN gap data.presenter/blocks/plot.py:172- build_waveform_plot renders a completely blank PNG without error when no traces and no data are provided: trace resolution (lines 143-170) only appends when traces/x+y are given, so build_waveform_plot(x=[1.0,2.0,3.0]) (y omitted) or build_waveform_plot() proceeds to plt.subplots and saves a 39KB blank figure, returning a block pointing at an empty plot — the same silent-empty-render failure mode the accepted review decisions eliminated on the CSV path. Apply the loud-failure invariant here too: raise ValueError when plot_traces is empty after argument resolution, before creating the figure.presenter/blocks/plot.py:351- x_col as a negative int is used directly as a row index (row[x_idx], indexing from the end) while the default y-column scan excludes columns viai != x_idxover range(len(rows[0])) — a negative x_idx never matches, so the x column is also emitted as a y trace (verified: build_plot_from_csv(f, x_col=-1) on a 2-column file renders a plot where the last column is both axis and trace, silently wrong semantics). Validate after resolution that 0 <= x_idx < len(header_names) (or normalize negative indices) and raise ValueError otherwise; the out-of-range positive case already fails loudly via the new 'no numeric data rows' check.🔧 Fix: Harden CSV classification, all-NaN rejection, empty-plot and column-index validation
3 infos still open:
presenter/blocks/plot.py:172- The accepted loud-failure invariant ('no silent empty renders') is enforced at the CSV boundary but not at the direct-array boundary of build_waveform_plot: when every provided trace has no plottable data, a blank ~41.7KB PNG is silently rendered. Verified three concrete inputs, all returning a block with a rendered PNG and no exception: (a) traces=[PlotTrace(x=[], y=[])] (equal-length empty data draws nothing), (b) x=[1.0,2.0], y=[nan,nan] (all-NaN y), and (c) x=[nan,nan], y=[1.0,2.0] (all-NaN x). Partial-NaN gaps and one-empty-plus-one-valid-trace combinations render correctly, so the check must be per-plot, not per-trace. Recommended earliest shared boundary: extend the new guard at line 172 - after trace resolution, raise ValueError unless at least one finite (x, y) pair exists across all traces and no trace data is empty, mirroring the finite-value check already accepted for the CSV path.presenter/blocks/tables.py:126- Engineering/sigfig formatters display one extra significant digit at prefix/decade boundaries without re-normalizing to the next SI prefix: format_engineering(999.9, unit='V') returns '1000 V' rather than '1.00 kV', format_engineering(9.999e-4, unit='A') returns '1000 µA' rather than '1.00 mA', and format_sigfigs(999.9, 3) returns '1000' (4 sig figs for a 3-sig-fig request). Values are correctly rounded and nothing errors; this is a display tradeoff for inputs within one rounding step of a boundary. For a datasheet-focused library, normalizing (increment the prefix when the mantissa rounds past 1000) would match convention, but it changes user-visible output, so flagged for the author to decide.tests/presenter/blocks/test_plot.py:340- The x_col=-1 regression assertion in test_build_plot_from_csv_column_index_handling only checks that a PNG renders, but the pre-fix buggy behavior (x column simultaneously used as axis and emitted as a y trace) also rendered a PNG, so this test would still pass if the V4 normalization regressed. Pin an observable that distinguishes the two: assert the rendered bytes differ from the x_col=0 rendering (a different x column must produce a different plot), or add a single-column case where correct wrapping (x_idx 0) yields 'no y columns' while unwrapped (-1) would yield 'out of range'.✅ **Test** - passed
✅ No issues found.
/tmp/presenter-venv/bin/python -m pytest tests/presenter/ -q (52 passed; system-default pytest is unusable here because its python3.12 interpreter cannot import the python3.14-built numpy/matplotlib)before/after regression transcript: loaded plot.py from commit 9ceb244 vs HEAD and exercised build_plot_from_csv/build_waveform_plot public APIs — build_plot_from_csv('time,voltage\n') now ValueError 'no numeric data rows' (was FileNotFoundError misroute), build_plot_from_csv('t,V\n0,foo\n1,bar\n') now ValueError 'no finite y values' (was silent 39,994-byte PNG), build_waveform_plot() and build_waveform_plot(x=[1,2,3]) now ValueError 'No plot data provided' (was silent blank PNGs), x_col=5 now ValueError 'x_col index 5 out of range' (was misleading 'no numeric data rows'), 'data,old.csv' and '/tmp/typo.csv' and directory input all raise FileNotFoundError naming the path, partial-NaN gaps and single-point '0.0,1.0\n' still rendervisual V4 regression pair: rendered the same 2-column CSV with x_col=-1 before vs after — before-fix legend shows the leaked 'b' trace, after-fix shows only 'a' (v4_xcol_before.png / v4_xcol_after.png)product render via build_plot_from_csv from a real CSV file (rc_step_response.csv, x_col='time_us', y_cols=['v_in','v_out'], units, legend) and visually verified rc_step_response.pngproduct render via build_equation_from_sympy('1/(2*pi*sqrt(L*C))') → LaTeX '\frac{1}{2 \pi \sqrt{C L}}' → resonance_equation.png, visually verifiedF1 behavior check: build_spec_table with headers ['Min (V)','Typ (V)','Max (V)','Units','Parameter','Conditions'] returns row ['1.18','1.20','1.22','V','Output Voltage','I_OUT = 10 mA'] — every cell under its requested columnF4 behavior check: format_engineering(inf/-inf/nan) returns 'Inf'/'-Inf'/'NaN', identical to format_sigfigsblock dicts consumed by two renderers: matplotlib table renderer → spec_pvt_tables.png (4 tables, visually verified) and HTML consumer → blocks_datasheet.html, validated well-formed with 4 tables and 2 embedded PNGs whose files existCONTRIBUTING.md:48- The presenter library's pytest suite under tests/presenter/ is currently only noted as outside bin/fm-test-run.sh's selection; there is no documented way to run it (no pytest invocation or dependency declaration) because pyproject.toml belongs to the scaffold lane per the accepted lane split. When that lane lands packaging, CONTRIBUTING.md's test mechanics should gain the owner-backed run command for the Python suite. Deferred rather than invented here to avoid documenting unwired mechanics.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.