Repository navigation
feat: add requirement reference-coverage pie charts and gap tables to the platform verification report - #896
Conversation
|
Documentation preview for this pull request is available at: |
…y' column Add per-requirement reference-coverage pie charts to the platform verification report, showing how many requirements are referenced by at least one lower-hierarchy requirement (stakeholder <- feature requirement, feature requirement <- component requirement). Only references within the report version scope are counted. Component requirements have no lower level, so no chart is generated. The stakeholder and feature requirement tables gain a 'Referenced by' column (derived_from_back) listing referencing requirements.
Place the third pie chart (reference coverage), which wraps onto a row of its own in the two-column pie grid, in the center via a new opt-in `score-centered-grid-item` grid-item class. The requirement tables gained a "Referenced by" column and became cramped; give them an opt-in `score-wide-needs-table` class with a larger minimum width and horizontal overflow so every column stays readable.
Remove the opt-in score-wide-needs-table class (min-width / horizontal overflow) from the requirement needtables and its CSS, restoring the table sizing to the state it had before the widening change. The reference-coverage pie centering is kept.
d082b00 to
b16a3dc
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The backlink column can contradict scoped chart results, and repeated full-graph scans introduce quadratic rendering cost.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds requirement-reference coverage reporting to platform verification reports.
Changes:
- Adds stakeholder and feature reference-coverage pie charts and backlink columns.
- Adds CSS to center wrapped pie charts.
- Updates generated HTML expectations for the CSS hash.
| File | Description |
|---|---|
src/needs_templates/platform_verification_report.need |
Adds reference coverage calculations, charts, and table columns. |
src/extensions/score_layout/assets/css/score_design.css |
Centers standalone grid items. |
src/tests/docs_bzl/scenarios/reference_integration/_expected/docs/modules/modern_module/components/unlinked_component/generated_metamodel/index.html |
Updates CSS asset hash. |
src/tests/docs_bzl/scenarios/nested_bundles/_expected/docs/index.html |
Updates CSS asset hash. |
src/tests/docs_bzl/scenarios/nested_bundles/_expected/docs/concepts/index.html |
Updates CSS asset hash. |
src/tests/docs_bzl/scenarios/nested_bundles/_expected/docs/concepts/example_bundle/child/landing.html |
Updates CSS asset hash. |
src/tests/docs_bzl/scenarios/external_bundle/_expected/docs/index.html |
Updates CSS asset hash. |
src/tests/docs_bzl/scenarios/data_files_runfiles/_expected/docs/legacy_data_test/index.html |
Updates CSS asset hash. |
src/tests/docs_bzl/scenarios/data_files_runfiles/_expected/docs/isolated_test/index.html |
Updates CSS asset hash. |
src/tests/docs_bzl/scenarios/data_files_runfiles/_expected/docs/index.html |
Updates CSS asset hash. |
src/tests/docs_bzl/scenarios/data_files_runfiles/_expected/docs/data_test/index.html |
Updates CSS asset hash. |
src/tests/docs_bzl/scenarios/basic_docs/_expected/docs/index.html |
Updates CSS asset hash. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The Stakeholder and Feature "Reference Coverage" pies previously scanned the whole Need graph per requirement via linked_needs(id, "derived_from_back"), which is O(all needs) each. Iterate the lower requirements once instead and follow their cheap outgoing derived_from link; for Features the index is built a single time before the per-feature loop, which then only tests membership. Addresses Copilot review Finding 1 (performance).
013c128 to
14aeb29
Compare
14aeb29 to
1c70135
Compare
The "Referenced by" column (backed by the unscoped `derived_from_back` backlinks) was inconsistent with the report-version-scoped Reference Coverage pie next to it. Remove it for now; a properly scoped approach will follow in a separate PR.
|
@AlexanderLanin please review/approve |
The reference coverage pies show *how many* requirements are not referenced by any lower-level requirement, but not *which* ones. Add a collapsed table next to each pie that lists exactly those requirements: - Stakeholder section: stakeholder requirements not referenced by any in-scope feature requirement. - Per feature: feature requirements not referenced by any in-scope component requirement. Both tables are filtered by the same pre-scoped ID lists that feed the pies (`ns_stkh_ref.missing` / `ns_feat_ref.missing`), so chart and table cannot disagree. The report version scoping therefore applies to the tables as well: a requirement referenced only from outside the report's `report_version` still counts as a gap. The tables are omitted entirely when there is no gap, so an empty table never shows up. Adds an end-to-end test rendering the report, covering the case where the only reference comes from an out-of-scope requirement.
| /* Center a grid item that wraps onto a row of its own (e.g. the third pie | ||
| chart in the verification report's two-column pie grids). */ | ||
| .score-centered-grid-item { | ||
| margin-left: auto; | ||
| margin-right: auto; | ||
| } |
There was a problem hiding this comment.
as this is platform verification report specific we should move it there?!
Example here:
docs-as-code/src/needs_templates/module_verification_report.need
Lines 73 to 97 in ae6c330
There was a problem hiding this comment.
Good call, done in 7f130f7 - moved into the template's own raw:: html style block, exactly like the module_verification_report.need example you linked.
It turned out to be worth more than just tidiness: .score-centered-grid-item is used only by platform_verification_report.need, and having those 7 lines in the globally loaded score_design.css changed its asset hash, which dragged 10 expected-output HTML fixtures into this PR. With the rule in the template, score_design.css is untouched, the hash stays at v=8048e6ee, and the PR is down to a single changed file.
Verified with pytest src/tests/docs_bzl/test_docs_bzl_scenarios.py across all scenarios that have expected HTML output: 20 passed.
|
Just a comment to the css. Since platform report is new and not established I dont see any need to discuss those changes at length. |
| {% endfor %} | ||
| {% set ns_stkh_ref = namespace(referenced=[], missing=[]) %} | ||
| {% for req_id in ns_stkh_reqs.list %} | ||
| {% if req_id in ns_ref.up_ids %} |
There was a problem hiding this comment.
Agreed on the analysis, but it is not actionable inside the template: these templates are rendered by minijinja, which has no set or dict literal and no mutable containers - namespace + list concatenation is the only accumulator available. A real fix means moving the index construction into a Python helper in the render context (like the existing linked_needs / needs_of_type globals).
On the cost: the finding that mattered was the previous linked_needs(..., "derived_from_back") call per row, which was O(requirements x all needs) over the whole platform graph - that one is fixed. What remains is O(references^2) over the in-scope requirements of a single report, i.e. a few thousand comparisons at realistic sizes, against a Sphinx build that takes minutes. Not worth a new render-context API in this PR.
Tracking it as a follow-up rather than resolving it silently - happy to add a reference_index(...) helper if we see report sizes grow.
There was a problem hiding this comment.
Minininja has set in it's compatibility markdown file and claims it's feature parity.
So could you try and implement it?
It won't be the end of the world if it doesn't work, but gathering performance bonuses where we can is always a plus.
https://github.com/mitsuhiko/minijinja/blob/main/COMPATIBILITY.md#-set-
There was a problem hiding this comment.
The {% set %} section in COMPATIBILITY.md is under ## Blocks — it documents the assignment tag, not a set type. The template already uses it throughout.
Checked against minijinja 2.22:
| construct | result |
|---|---|
{1, 2, 3} |
syntax error: unexpected ,, expected : |
set(...) |
unknown function: set is unknown |
{"a": 1} dict literal |
works |
Passing a Python frozenset in through the render context does work (in resolves), but it does not buy anything. 1000 membership tests, all misses:
| haystack | time |
|---|---|
| list | 1402 ms |
| frozenset | 1286 ms |
| same logic in Python | 0.12 ms |
The cost is the Rust/Python boundary crossing per in, not the lookup itself. A set inside the template would not have fixed it; not looping in the template does.
Implemented in 164fbf0: two render-context helpers, referenced_ids (one forward pass over the lower requirements) and split_by_reference (partition). The template just calls them, 44 lines of loop logic removed.
| stkh x feat | before | after | |
|---|---|---|---|
| 20 x 25 | 12.53 ms | 0.09 ms | 141x |
| 300 x 600 | 72.16 ms | 0.27 ms | 265x |
| 600 x 1200 | 362.2 ms | 0.55 ms | 663x |
Quadratic -> linear. The benchmark asserts the rendered output is identical in every row; bazel test //src/extensions/score_sphinx_needs_templates:unit_tests passes.
At today's size (~20 stkh_req, ~25 feat_req) this is 12 ms against a multi-minute Sphinx build, so there is no measurable gain right now. The scaling is the point.
This also resolves the earlier Copilot finding in this thread, which I had deferred as "not actionable in the template" — it was actionable, one layer down.
| .. grid-item:: | ||
| :class: score-centered-grid-item | ||
|
|
||
| .. needpie:: Stakeholder Requirements Reference Coverage |
There was a problem hiding this comment.
Fixed - the title and description claimed the Referenced by column that was removed in 0ec4ad8.
Updated the title, rewrote the What section, and added an explicit What this PR does not add section explaining why the column was dropped (the raw backlink field is not report-version scoped) and that the drill-down ships as rows instead in the stacked follow-up #921. The Files list now matches the actual single changed file.
The `.score-centered-grid-item` rule is used only by platform_verification_report.need, so it belongs next to the markup it styles rather than in the globally loaded score_design.css. Moves it into the template's existing `raw:: html` style block, following the pattern already used by module_verification_report.need. Side effect: score_design.css is unchanged again, so its asset hash stays at v=8048e6ee and the ten expected-output HTML fixtures no longer need to be touched by this PR. Addresses review feedback from @AlexanderLanin.
…ent-reference-gap-tables
antonkri
changed the title
antonkri
changed the title
|
One question here. Is this just for information, or is there actually a reason to check this? |
@MaximilianSoerenPollak Good question! It is not a en error in terms of metamodel. During the lifecycle of the project, it is ok, that e.g. not all stakeholder requirements are referenced by feature requirements at the beginning, as the implementation or break down is not there. But it is important to track this relationship to be able to judge about the completeness of the project. e.g. If not all feature requirements are referenced at least by one component requirement, then it is a metric, that shows, that feature implementation is not complete. |
The reference-coverage sections built their index and partitioned the
requirements with MiniJinja loops. Every `in` test and every
`list + [item]` append crossed the Rust/Python boundary, which dominated
the cost and scaled quadratically.
Add two render-context helpers, `referenced_ids` and `split_by_reference`,
and let the template call them instead. Output is unchanged.
Measured on the stakeholder block (identical rendered output):
stkh x feat | old | new | speedup
20 x 25 | 12.53 ms | 0.09 ms | 141x
300 x 600 | 72.16 ms | 0.27 ms | 265x
600 x 1200 | 362.2 ms | 0.55 ms | 663x
Note that a `set` is not available inside the template: MiniJinja has no
set literal and no `set()` function, and handing a Python `set` into the
template does not help either, because the boundary crossing rather than
the lookup is what costs (frozenset was only ~8% faster than a list).
|
@MaximilianSoerenPollak Informational, deliberately. It cannot be a metamodel check, because "not referenced" is not a property of the need — it is relative to The model is also permissive here by design: Enforcement already exists, one level up: |
MaximilianSoerenPollak
left a comment
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
I do like this much more, moving hte logic to python.
💯
|
@AlexanderLanin are all of your questions answered & okay to merge? |
What
Adds requirement reference coverage reporting to the platform verification report, in two parts.
1. Reference-coverage pie charts
For every requirement level, a pie chart showing how many requirements are referenced by at least one lower-hierarchy requirement:
feat_req(viaderived_from).comp_req(viaderived_from).Only references from requirements within the report-version scope are counted (
req_in_report_version).2. Gap tables
Under each pie, a collapsible table listing exactly which requirements make up the red "not referenced" slice (
id,title,safety,status):Collapsed by default (
sphinx-designdropdown), so the report stays readable. The pie and the table are rendered from the same pre-scoped ID list, so they cannot disagree.Why a table and not a column
An earlier revision added a
Referenced bycolumn (derived_from_back) to the requirement tables. That column was removed again in 0ec4ad8, because the raw backlink field is not report-version scoped: it could list a referencing requirement that the pie counts as "not referenced" (see this thread).The distinction is structural:
:filter:):columns:)A scoped column would need the scoping to happen before sphinx-needs post-processing, which means metamodel changes. A scoped table only needs row selection, which the existing
id_filter_listmachinery already does. Hence the drill-down ships as rows.Files
src/needs_templates/platform_verification_report.needsrc/extensions/score_sphinx_needs_templates/tests/test_needs_templates.pyThe report-specific centering CSS lives in the template's own
raw:: htmlstyle block (as inmodule_verification_report.need) rather than in the globally loadedscore_design.css, so no CSS asset hash changes and no expected-output fixtures are touched.Validation
bazel test //src/extensions/score_sphinx_needs_templates:unit_tests: PASSED, including a new test asserting that a stakeholder requirement referenced only from outside the report version still shows up as a gap.pytest src/tests/docs_bzl/test_docs_bzl_scenarios.pyfor all scenarios with expected HTML output: 20 passed.