Convert instrumentation metric reporting to Pydantic models#192
Merged
AlexJones0 merged 3 commits intoMay 14, 2026
Conversation
machshev
approved these changes
May 14, 2026
Collaborator
machshev
left a comment
machshev
left a comment
There was a problem hiding this comment.
Thanks @AlexJones0! This looks like a nice tidy up...
AlexJones0
force-pushed
the
instrumentation_refactor_2
branch
from
9692f19 to
0f1d1a5
Compare
Contributor
Author
|
The latest force push is a rebase on |
Migrate from Python dataclasses to Pydantic models for instrumentation metric reports, with a central `instrumentation/records.py` file containing each of the definition. In a future commit, this will allow us to easily make a statically typed Pydantic model for a full instrumentation report of the combined results. To promote better typing, this also changes the `SchedulerInstrumentation` interface such that you can now separately query the collected scheduler and job data, to provide a slightly more intuitive interface. This change is then propagated across the various built-in instrumentation types. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Rather than just relying on the class `__name__`, we will want a well- defined field that matches the various fields we are using within our Pydantic model. This will allow plugins to declare a 'name' that can then be queried to get their specific metrics. Note that there is now some overlap with the name that is used in the instrumentation registry/factory, but that will be addressed in a future commit. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
Get rid of the existing logic that collapsed everything to arbitrary dictionaries and ensure that the merged instrumentation report is a well-formed Pydantic model that can then be confidently queried, with extra attributes (instrumentation records) still allowed to support custom instrumentations registered via e.g. plugins. Note that there is slightly less flattening on the produced JSONs (this is a design change) - each independent job (and the scheduler itself) now contains many dictionaries, with one per instrumentation, to effectively scope each instrumentation's contribution to the final report. This doesn't impact readability too much and will importantly allow us to load the reports back in the future. The intention here is that built-in instrumentation types have a well-defined field in the report model that can be queried, whereas any additional plugin data is still made available through the Pydantic `extra="allow"` configuration option. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
AlexJones0
force-pushed
the
instrumentation_refactor_2
branch
from
0f1d1a5 to
c9bbf0a
Compare
Contributor
Author
|
And the latest force-push just fixed a bad copy-paste in a docstring 😅 |
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.
This PR is the third of a series of PRs to introduce instrumentation reporting to DVSim, which in this case also involves refactoring the existing instrumentation implementation to allow better modeling & static typing around this use case.
This PR features the main overhaul to the instrumentation data model logic. Unfortunately this PR is on the slightly larger side by necessity, as it would otherwise require a lot of extraneous code & commits to make the migration cleanly. But the code should hopefully be relatively easy to review. The main changes involved are:
instrumentation/records.pyfile. Stop re-exporting so many of these through theinstrumentation/__init__.pypackage.For the 1st commit, I recommend reviewing
base.py, thenrecords.pyalongside (metadata.py,timing.py,resources.py), and finally__init__.pyfor the easiest time.This will allow us to introduce reporting logic later on which can consume the instrumentation data whilst being confident in the static typing provided. Plugins can then still be supported via Pydantic's
extra=allowconfig on the combined scheduler and job instrumentation data models.See the commit messages for more information.