feat: score the vPlan from test results as well as coverage - #243
Open
martin-velay wants to merge 4 commits into
Open
feat: score the vPlan from test results as well as coverage#243martin-velay wants to merge 4 commits into
martin-velay wants to merge 4 commits into
Conversation
martin-velay
changed the title
martin-velay
force-pushed
the
testcase_dvplan
branch
from
August 18, 2026 14:29
af5f48e to
258e394
Compare
machshev
reviewed
Sep 1, 2026
machshev
left a comment
machshev
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @martin-velay, great work in general. Some parts of the architecture I'm not so sure about, given the ongoing refactoring work. But hopefully we can resolve these fairly quickly.
Comment on lines
+700
to
+709
| def on_job_completed( | ||
| self, spec: JobSpec, status: JobStatus, reason: JobStatusInfo | None | ||
| ) -> None: | ||
| """Record each run's outcome as the scheduler concludes it. | ||
|
|
||
| Fed from the scheduler rather than from a job callback, because only the scheduler sees a job | ||
| it cancelled before dispatching it. | ||
| """ | ||
| self.run_evidence.record(spec, status, reason) | ||
|
|
Collaborator
There was a problem hiding this comment.
This does mean the post processing is being interleaved with the scheduled jobs. Other steps in the flows use either:
JobSpecsubmission to the scheduler to perform dependency based schedulinggen_resultsreport post processing based on theCompletedJobStatusof each job
This adds a new mechanism that is interleaved with (1) but not visible to the scheduler or the metrics we record in the scheduler. I'm tempted to suggest that this is refactored into one of the two existing mechanisms.
So if the work in this callback is reporting, then maybe add it to the gen_results and filter out the VPlan jobs from the CompletedJobStatus collection and do the post processing there. If there is significant work involved i.e. calling external processes to do parsing and such like then I'd suggest it should be moved into it's own Job and pass through the scheduler as a substantial piece of work.
Message passing between jobs should ideally be done via the filesystem in the scratch directory. That means we can later implement restarting of partial runs, or rerunning of individual steps. When the original runtime memory for the previous steps have been lost, the scratch directory working files are still there.
martin-velay
force-pushed
the
testcase_dvplan
branch
3 times, most recently
from
September 3, 2026 08:25
fdb0f1c to
3ef132f
Compare
The status-change callback carries no reason, so an observer cannot tell a failed job from one the scheduler cancelled before dispatching it. The new callback gets the reason and fires once the status is settled. The existing callback is untouched, since its only consumer is the status printer and that has no use for the reason. Flows opt in by overriding FlowCfg.on_job_completed, which does nothing by default. AI-assisted (Claude Code) - reviewed and approved by author Signed-off-by: martin-velay <mvelay@lowrisc.org>
A cov_vplan job runs dvplan over the coverage report and a dv_evidence.json this flow writes, so a nightly says how much of the verification plan is met and not just how much of the design was covered. Nothing runs unless the sim cfg names a vplan. The evidence comes from the scheduler's completion hook as each run finishes, so it covers jobs cancelled before dispatch too. A test the plan asked for and the regression never ran then shows up as a hole instead of going missing. Both sources go to one dvplan invocation, because dvplan writes an item off as unmeasurable when nothing it was given can measure it, and that sticks in the annotated file. Runs without --cov score from the evidence alone. Deploy gains a log_path property for the path it already built inline, which is what the LSF launcher reaches for. AI-assisted (Claude Code) - reviewed and approved by author Signed-off-by: martin-velay <mvelay@lowrisc.org>
martin-velay
force-pushed
the
testcase_dvplan
branch
from
September 3, 2026 11:09
3ef132f to
bfeeed1
Compare
machshev
reviewed
Sep 3, 2026
| EVIDENCE_JSON = "dv_evidence.json" | ||
|
|
||
|
|
||
| @dataclass(frozen=True) |
Collaborator
There was a problem hiding this comment.
It would be nice to use pydantic model instead of dataclass here. Should be a drop-in replacement almost.
Contributor
Author
There was a problem hiding this comment.
Should be done now
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. Each run is logged to the scratch directory as it finishes rather than accumulated in memory. The log, not a live dvsim process, is what the vPlan job reads, so the step can be retried and a part-finished run picked up without the earlier outcomes having been lost with the process that saw them. It costs one short append per test against a simulation that took minutes, and it is off the critical path of everything except the job that just ended. That also fixes a primary cfg run. One scheduler serves every cfg, so the completion hook belonged to the top-level cfg while each vPlan job read its own child cfg, and every child wrote an evidence file holding no tests at all. A run is now filed under the scratch area it ran in, and the vPlan job reads the log beside its own output rather than being told which of the regression's runs were its. 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. `VPlanInputs` is a pydantic model rather than a frozen dataclass, which is what every other data model in the tree is and what this series' own evidence models already were. The fields a cfg fills are validated where the job is built, so a `dvplan_inspect` written as a list is rejected there rather than reaching `shlex.split` inside the job. 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 Signed-off-by: martin-velay <mvelay@lowrisc.org>
The page claimed the pydantic models were the normative definition of the whole format. They are not: they forbid the inspection key, so they reject a file DVPlan accepts. That is correct for a writer, but it makes the claim wrong. AI-assisted (Claude Code) — reviewed and approved by author Signed-off-by: martin-velay <mvelay@lowrisc.org>
martin-velay
force-pushed
the
testcase_dvplan
branch
from
September 4, 2026 14:56
bfeeed1 to
e9f048f
Compare
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
2 participants
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.
Linked to https://github.com/lowRISC/dvplan/pull/117
What this does
The
cov_vplanjob already annotated a vPlan from the coverage database. It now alsoannotates from the regression's test results, so a nightly says how much of the
verification plan is met and not only how much of the design was covered. A test the
plan asked for and the regression never ran shows as a hole rather than going missing.
dvsim writes those results to
dv_evidence.jsonand passes it to dvplan alongside thecoverage report. It also passes through a path to hand-written inspection records if the
cfg names one.
Nothing runs unless the sim cfg sets
vplan.Three decisions worth reviewing
The evidence comes from a new scheduler callback, not from the end-of-run results.
Everything it records is already in
run_results, but that only exists once the schedulerhas finished, and
cov_vplanis a scheduled job that needs the file before then. Thealternative is to stop scheduling it and run it at
gen_resultstime, which would dropthe callback, the collector and the
FlowCfghook. I kept it scheduled so the step keepsits row in the status table and can fail like any other job.
needs_all_dependencies_passingbecameDependencyPolicy. The bool had two statesand this job needs a third.
ALL_PASSINGis the default andANY_PASSINGis whatCovMergealways did, so neither changes.CovVPlantakesALWAYS, because a regressionwhere nothing passed is exactly the case the plan has to describe, and under either
existing policy it was killed instead of scored: with
--covit has one dependency, soanything that stopped the coverage report also stopped the plan.
dvsim defines the evidence format, not dvplan.
doc/dv_evidence.mdspecifies it andthe pydantic models are normative. dvsim is what 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. dvplan is one consumer.
Dependencies and degradation
Needs the linked dvplan PR first: it defines the
dv_evidencesource name and themulti-source behaviour. Whether dvplan is installed is decided by the job's own script on
the machine it runs on, so a checkout without it warns and passes.
The credential fix rides along
git_https_url_with_commitreturns theoriginremote verbatim today, so a CI checkoutwith
https://oauth2:<token>@host/org/repoalready publishes the token in the JSON andHTML reports. This PR adds another archived artefact carrying the same URL, so the fix is
first in the series. It reverts cleanly on its own.
Behaviour change
A cfg that sets
vplanand runs without--covpreviously got no vPlan job. It now getsone, scored from test results alone.
Verified
ruff format --checkandruff check --config=ruff-ci.tomlclean,pytest --strict396passed,
license_check.pyclean,pyright182 errors unchanged, every commit testedstandalone. Run end to end against a real hmac regression in an OpenTitan tree.