Skip to content

Commit af5f48e

Browse files
committed
fix: score the vPlan whatever the regression concluded
Review fixes for the vPlan back-annotation series. The plan could not be scored in the regression it most needs to describe. `needs_all_dependencies_passing` had two states and this job needs a third, so it becomes `DependencyPolicy`. `ALL_PASSING` is the default and `ANY_PASSING` is what CovMerge always did, so neither changes behaviour. CovVPlan takes `ALWAYS` and runs once its dependencies are terminal, whatever they concluded. Under either existing policy a regression where nothing passed was killed rather than scored, and with --cov the job has a single dependency, so anything that stopped the coverage report also stopped the plan. dvsim now defines the evidence format rather than deferring to dvplan. doc/dv_evidence.md specifies it and the pydantic models are normative. dvsim produces the file and is the public repo, so a consumer can be written against a spec rather than against whichever tool was built first. Whether dvplan is installed is decided by the job's own script, so it reads the PATH of the machine the job lands on rather than that of the host dvsim was launched from, which on a compute farm need not be the same. A `dvplan_inspect` pattern matching nothing is now a config error. The command is built while the jobs are, so it stops the run in seconds rather than failing inside dvplan once the regression has already gone. The vPlan report page is linked only once it exists, since a killed job or a machine without dvplan otherwise left a dead link in the HTML report. AI-assisted (Claude Code) — reviewed and approved by author
1 parent 7f2bd4d commit af5f48e

12 files changed

Lines changed: 241 additions & 84 deletions

File tree

README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ You can access it [online at opentitan.org/book/](https://opentitan.org/book/).
8686

8787
* [Testplanner tool](./doc/testplanner.md)
8888
* [Design document](./doc/design_doc.md)
89+
* [The `lowrisc-dv-evidence` format](./doc/dv_evidence.md)
8990
* [Glossary](./doc/glossary.md)
9091

9192
## How to contribute

doc/dv_evidence.md

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
<!--
2+
# Copyright lowRISC contributors (OpenTitan project).
3+
# Licensed under the Apache License, Version 2.0, see LICENSE for details.
4+
# SPDX-License-Identifier: Apache-2.0
5+
-->
6+
# The `lowrisc-dv-evidence` format
7+
8+
A regression tells you which tests passed.
9+
A verification plan asks a different question: of everything we said we would verify, how much is now backed by something that ran?
10+
Answering it needs the regression's own outcomes in a form a planning tool can read, rather than a log directory and a human.
11+
12+
This is that form.
13+
DVSim writes one of these files per simulation flow, and it is the definition of the format rather than a description of one tool's output.
14+
Anything that can produce it can be scored against a verification plan, whether or not it is DVSim.
15+
16+
## Where DVSim writes it
17+
18+
`<scratch_path>/cov_vplan/dv_evidence.json`, produced by the `cov_vplan` job, and only when the sim config names a `vplan`.
19+
It is written before the annotation step runs and is archived alongside the reports, so it outlives the scratch area it describes.
20+
21+
## Shape
22+
23+
```json
24+
{
25+
"schema": "lowrisc-dv-evidence",
26+
"dut": "hmac",
27+
"tool": "xcelium",
28+
"produced_by": "dvsim 1.50.1",
29+
"revision": "https://github.com/lowRISC/opentitan/tree/a1b2c3d (dirty)",
30+
"timestamp": "2026-08-18T09:00:00+00:00",
31+
"testcase": {
32+
"hmac_smoke": [
33+
{ "status": "passed", "seed": 1234, "log": "/scratch/hmac/1234.hmac_smoke/run.log" },
34+
{ "status": "failed", "seed": 5678, "log": "...", "message": "UVM_ERROR", "line": 812 }
35+
],
36+
"hmac_stress_all": [
37+
{ "status": "not_run" }
38+
]
39+
}
40+
}
41+
```
42+
43+
Fields are omitted when they have no value rather than written as `null`.
44+
45+
### Top level
46+
47+
| Key | Meaning |
48+
| --- | --- |
49+
| `schema` | Always `lowrisc-dv-evidence`. Identifies the format to whatever reads the file. |
50+
| `testcase` | Test name to the list of runs of that test. The only required key. |
51+
| `dut` | The design the results are about, named as a verification plan addresses it. |
52+
| `tool` | The simulator that produced them. |
53+
| `produced_by` | What wrote the file, with its version. |
54+
| `revision` | The tree the results were produced against, suffixed ` (dirty)` when it was not clean. |
55+
| `timestamp` | When the run started, as an ISO 8601 datetime with an offset. |
56+
57+
### A run
58+
59+
Every entry under `testcase` is keyed by the test name, because that is the name a plan refers to.
60+
Reseeds of one test share the key and are told apart by `seed`.
61+
62+
| Key | Meaning |
63+
| --- | --- |
64+
| `status` | One of `passed`, `failed`, `killed`, `not_run`. Required. |
65+
| `seed` | The seed the run used, where the flow randomises. |
66+
| `log` | Path to the run's log. |
67+
| `message` | Why it ended that way. Present only on a run that did not pass. |
68+
| `line` | The log line the failure was first reported at. |
69+
70+
`killed` and `not_run` are separate on purpose.
71+
A killed test started and was terminated, so the design was exercised and something went wrong.
72+
A `not_run` test never started, because the scheduler cancelled it once a dependency failed or the run was shut down.
73+
The two are different answers to "did we verify this", and collapsing them would let a build failure read as a passing plan item.
74+
75+
There is no `waived` status.
76+
A waiver needs an owner and a date, and a regression can supply neither, so a known failure is recorded as an inspection instead.
77+
78+
## Inspections
79+
80+
The format also carries an `inspection` key, for claims no simulation can measure, such as a parameterisation or a structural fact.
81+
Those records are written by hand and live in the tree next to the plan they support.
82+
DVSim never produces them; it only passes their path through to whatever consumes this format, so they are out of scope for this document.
83+
84+
## Consumers
85+
86+
[DVPlan](https://github.com/lowRISC/dvplan) reads it to back-annotate a verification plan.
87+
It is not the only thing that could: the format carries no DVPlan concepts, and a dashboard or a CI job wanting machine-readable regression results can read the same file.
88+
89+
## Changing it
90+
91+
The pydantic models in `src/dvsim/report/dv_evidence.py` are the normative definition, and this document describes them.
92+
A change to either is a change to the format, so change both, and bear in mind that a consumer may be reading files this repo wrote months ago.

src/dvsim/job/data.py

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
"""
1111

1212
from collections.abc import Callable, Mapping, Sequence
13+
from enum import Enum
1314
from pathlib import Path
1415
from typing import TypeAlias
1516

@@ -20,12 +21,30 @@
2021

2122
__all__ = (
2223
"CompletedJobStatus",
24+
"DependencyPolicy",
2325
"JobSpec",
2426
"JobStatusInfo",
2527
"WorkspaceConfig",
2628
)
2729

2830

31+
class DependencyPolicy(Enum):
32+
"""When a job may start, given how the jobs it depends on ended."""
33+
34+
ALL_PASSING = "all_passing"
35+
"""Start only if every dependency passed, which is right for a job consuming their output."""
36+
37+
ANY_PASSING = "any_passing"
38+
"""Start if at least one dependency passed, for a job gathering whatever results exist."""
39+
40+
ALWAYS = "always"
41+
"""Start once every dependency is terminal, whatever they concluded.
42+
43+
For a job whose input is the outcome itself rather than an artefact a dependency produced, so
44+
a regression where nothing passed is still the thing it has to report on.
45+
"""
46+
47+
2948
class WorkspaceConfig(BaseModel):
3049
"""Workspace configuration."""
3150

@@ -92,8 +111,8 @@ class JobSpec(BaseModel):
92111

93112
dependencies: list[str]
94113
"""Full names of the other Jobs that this one depends on."""
95-
needs_all_dependencies_passing: bool
96-
"""Wait for dependent jobs to pass before scheduling."""
114+
dependency_policy: DependencyPolicy
115+
"""What the jobs this one depends on must have concluded before it may be scheduled."""
97116
weight: int
98117
"""Weight to apply to the scheduling priority."""
99118
timeout_mins: float | None

src/dvsim/job/deploy.py

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
from typing import TYPE_CHECKING, ClassVar
1313

1414
from dvsim.flow.base import FlowCfg
15-
from dvsim.job.data import JobSpec
15+
from dvsim.job.data import DependencyPolicy, JobSpec
1616
from dvsim.job.status import JobStatus
1717
from dvsim.job.time import JobTime
1818
from dvsim.logging import log
@@ -94,10 +94,9 @@ def __init__(self, sim_cfg: "FlowCfg") -> None:
9494
# A list of jobs on which this job depends.
9595
self.dependencies = []
9696

97-
# Indicates whether running this job requires all dependencies to pass.
98-
# If this flag is set to False, any passing dependency will trigger
99-
# this current job to run
100-
self.needs_all_dependencies_passing = True
97+
# What the jobs this one depends on must have concluded before it may run. The default
98+
# suits anything consuming a dependency's output, which is most jobs
99+
self.dependency_policy = DependencyPolicy.ALL_PASSING
101100

102101
# These variables will be extracted from the hjson file by _set_attrs,
103102
# and then _check_attrs checks that they were indeed extracted. Define
@@ -175,7 +174,7 @@ def get_job_spec(self) -> "JobSpec":
175174
),
176175
workspace_cfg=self.sim_cfg.workspace_cfg,
177176
dependencies=[d.full_name for d in self.dependencies],
178-
needs_all_dependencies_passing=self.needs_all_dependencies_passing,
177+
dependency_policy=self.dependency_policy,
179178
weight=self.weight,
180179
timeout_mins=(None if self.gui else self.get_timeout_mins()),
181180
cmd=self.cmd,
@@ -911,8 +910,8 @@ def __init__(self, run_items: Iterable[RunTest], sim_cfg: FlowCfg) -> None:
911910

912911
super().__init__(sim_cfg)
913912
self.dependencies.extend(run_items)
914-
# Run coverage merge even if one test passes.
915-
self.needs_all_dependencies_passing = False
913+
# Merge whatever coverage exists, so one passing test is enough to be worth merging.
914+
self.dependency_policy = DependencyPolicy.ANY_PASSING
916915

917916
# Append cov_db_dirs to the list of exports.
918917
self.merged_exports["cov_db_dirs"] = shlex.quote(" ".join(self.cov_db_dirs))
@@ -1074,8 +1073,9 @@ def __init__(self, dependencies: "Iterable[Deploy]", sim_cfg: "SimCfg") -> None:
10741073
super().__init__(sim_cfg)
10751074
# Every run it scores has to be terminal first, so the collector's evidence is complete
10761075
self.dependencies.extend(dependencies)
1077-
# A failed or killed run is still evidence, so score what happened rather than skipping
1078-
self.needs_all_dependencies_passing = False
1076+
# A failed or killed run is still evidence, and a regression where nothing passed is the
1077+
# case the plan most needs to describe, so this is scored whatever the dependencies did
1078+
self.dependency_policy = DependencyPolicy.ALWAYS
10791079

10801080
def _define_attrs(self) -> None:
10811081
super()._define_attrs()

src/dvsim/report/dv_evidence.py

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,12 @@
44

55
"""Regression results in the tool-neutral `lowrisc-dv-evidence` format.
66
7-
dvplan defines the format, so a vPlan can be back-annotated from any regression flow and a person
8-
can write one by hand. What dvsim writes here is a plain serialisation of what it already knows.
7+
These models are the format's definition, and `doc/dv_evidence.md` describes them. It lives here
8+
because dvsim is what produces the file, so anything reading one can be written against a public
9+
spec rather than against whichever consumer happened to be built first.
10+
11+
The format carries no planning-tool concepts, so a verification plan can be scored from any
12+
regression flow that emits it, and a person can write one by hand.
913
1014
Built from what the scheduler concludes about each job, through its completion hook. That is the
1115
same state the JSON report is derived from, so the two cannot disagree about a run, and it is
@@ -50,8 +54,8 @@
5054
class Outcome(Enum):
5155
"""How one run of a test ended, in the neutral format's vocabulary.
5256
53-
There is no waived outcome: dvplan requires an owner and a date on a waiver, and a regression
54-
can supply neither. A known failure is accepted there by recording an inspection instead.
57+
There is no waived outcome. A waiver needs an owner and a date, and a regression can supply
58+
neither, so the format only allows one on an inspection, which is written by hand.
5559
"""
5660

5761
PASSED = "passed"
@@ -96,8 +100,8 @@ class TestRun(BaseModel):
96100
class EvidenceFile(BaseModel):
97101
"""A regression's results, in the tool-neutral evidence format.
98102
99-
dvsim only ever fills the `testcase` half. The format also carries manual inspections, which a
100-
person writes by hand.
103+
dvsim only ever fills the `testcase` half. The format also has an `inspection` key, for claims
104+
no simulation can measure, and those records are written by hand.
101105
"""
102106

103107
model_config = ConfigDict(frozen=True, extra="forbid", populate_by_name=True)

src/dvsim/report/vplan.py

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212

1313
import glob
1414
import shlex
15-
import shutil
1615
from collections.abc import Sequence
1716
from dataclasses import dataclass, field
1817
from pathlib import Path
@@ -21,7 +20,7 @@
2120

2221
from dvsim.logging import log
2322

24-
__all__ = ("VPlanInputs", "overall_coverage", "shell_command")
23+
__all__ = ("SKIP_WITHOUT_DVPLAN", "VPlanInputs", "overall_coverage", "shell_command")
2524

2625
# Scratch subdirectory the annotated plan and its report are written to. Unchanged, so an existing
2726
# link to the report still resolves
@@ -31,6 +30,14 @@
3130
ANNOTATED_HTML = "vplan_annotated.html"
3231
EVIDENCE_JSON = "dv_evidence.json"
3332

33+
# Whether dvplan is installed is decided by the script, on the machine the job lands on, rather
34+
# than by dvsim on whichever host the run was launched from. Exits 0 so that a checkout without
35+
# dvplan does not fail every regression that names a vPlan
36+
SKIP_WITHOUT_DVPLAN = (
37+
"if ! command -v dvplan >/dev/null 2>&1; then "
38+
"echo 'WARNING: dvplan is not installed on PATH. Skipping vPlan annotation.'; exit 0; fi;"
39+
)
40+
3441

3542
@dataclass(frozen=True)
3643
class VPlanInputs:
@@ -74,12 +81,10 @@ def shell_command(inputs: VPlanInputs) -> str:
7481
7582
Returned as one `bash -c` string because a scheduled job runs a shell command. `set -e` and the
7683
`&&` mean a broken annotation shows as a failed job rather than a silently missing score.
77-
"""
78-
if shutil.which("dvplan") is None:
79-
# Warn and pass, so a checkout without dvplan does not fail every regression naming a vPlan
80-
warning = "WARNING: dvplan is not installed on PATH. Skipping vPlan annotation."
81-
return f"/usr/bin/env bash -c {shlex.quote(f'echo {shlex.quote(warning)}')}"
8284
85+
The command is the same whether or not dvplan is installed here, because here is not where it
86+
runs. See `SKIP_WITHOUT_DVPLAN`.
87+
"""
8388
# The vPlan sits at <ip_root>/<something>/<vplan>, so its grandparent is the IP root that
8489
# `prepare_vplan` traces specifications against.
8590
ip_root = inputs.vplan.parent.parent
@@ -94,7 +99,8 @@ def shell_command(inputs: VPlanInputs) -> str:
9499
process = _process_command(inputs)
95100

96101
script = (
97-
f"set -e; mkdir -p {shlex.quote(str(inputs.out_dir))}; "
102+
f"set -e; {SKIP_WITHOUT_DVPLAN} "
103+
f"mkdir -p {shlex.quote(str(inputs.out_dir))}; "
98104
f"{shlex.join(prepare)} && {shlex.join(process)}"
99105
)
100106
return f"/usr/bin/env bash -c {shlex.quote(script)}"
@@ -146,11 +152,17 @@ def _expand(pattern: str) -> list[str]:
146152
147153
A cfg naming inspections through `{proj_root}` always produces an absolute pattern, which
148154
`Path.glob` refuses, so this is one of the places the pathlib rule does not apply.
155+
156+
A pattern matching nothing raises, because both other answers are worse: passing it through
157+
fails the job with dvplan's own message once the regression has already run, and dropping it
158+
scores the plan as though the cfg had never named inspections at all. The command is built
159+
while the jobs are, so this lands before a single test starts.
149160
"""
150161
matches = sorted(glob.glob(pattern)) # noqa: PTH207 (Path.glob rejects an absolute pattern)
151162
if not matches:
152-
log.warning("No inspection records matched '%s', so none were annotated from.", pattern)
153-
return matches or [pattern]
163+
msg = f"No inspection records matched 'dvplan_inspect' pattern '{pattern}'."
164+
raise ValueError(msg)
165+
return matches
154166

155167

156168
def overall_coverage(annotated: Path) -> float | None:

src/dvsim/scheduler/core.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
from types import FrameType
1414
from typing import Any, TypeAlias
1515

16-
from dvsim.job.data import CompletedJobStatus, JobSpec, JobStatusInfo
16+
from dvsim.job.data import CompletedJobStatus, DependencyPolicy, JobSpec, JobStatusInfo
1717
from dvsim.job.status import JobStatus
1818
from dvsim.logging import log
1919
from dvsim.runtime.backend import RuntimeBackend
@@ -365,7 +365,10 @@ def _update_completed_job_deps(self, job: JobRecord) -> None:
365365

366366
# Handle dependency management and marking dependents as ready
367367
if dep.remaining_deps == 0 and dep.status == JobStatus.SCHEDULED:
368-
if dep.spec.needs_all_dependencies_passing:
368+
policy = dep.spec.dependency_policy
369+
if policy is DependencyPolicy.ALWAYS:
370+
self._mark_job_ready(dep)
371+
elif policy is DependencyPolicy.ALL_PASSING:
369372
if dep.passing_deps == len(dep.spec.dependencies):
370373
self._mark_job_ready(dep)
371374
else:

src/dvsim/sim/flow.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -884,10 +884,13 @@ def make_test_result(tr) -> TestResult | None:
884884
cov_report_dir = self.cov_report_dir or "cov_report"
885885
cov_report_page = Path(cov_report_dir, self.cov_report_page)
886886

887+
# Linked only once the page is actually there. The job can be killed, and it exits without
888+
# annotating anything where dvplan is not installed, so its output directory is not proof
887889
vplan_report_page = None
888890
vplan_coverage = None
889891
if self.cov_vplan_deploy is not None:
890-
vplan_report_page = self.cov_vplan_deploy.report_page
892+
page = self.cov_vplan_deploy.report_page
893+
vplan_report_page = page if page.is_file() else None
891894
vplan_coverage = self.cov_vplan_deploy.vplan_coverage
892895

893896
failures = BucketedFailures.from_job_status(results=run_results)

0 commit comments

Comments
 (0)