Skip to content

Be more careful with variants in OneShotCfg.gen_results #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rswarbrick wants to merge 1 commit into
base: master
Choose a base branch
from
+21 −2
Open

Be more careful with variants in OneShotCfg.gen_results #159

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions src/dvsim/flow/one_shot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,32 @@ def gen_results(self, results: Sequence[CompletedJobStatus]) -> None:
for item in self.cfgs:
project = item.name

# We want to report all the results with the correct block
# (checking the name) and also with the correct variant.
#
# For the latter check, we don't necessarily know that item (the
# configuration that is being run) is a type that defines variant
# at all. If not, we just interpret the variant as None.
item_variant = getattr(item, "variant", None)
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 Apr 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: do you think variants are a ubiquitous enough concept in DVSim that we should just define self.variant: str | None = None in FlowCfg.__init__? At least for the sim code I've seen them used a lot, but maybe they're just very sim-specific?

From the examples I've seen, such as 32kB and 64kB OpenTitan rom_ctrl variants, it seems like it should be part of any flow?


# The typing of the configuration also doesn't guarantee it has a
# _gen_results function, which we will need here. Check that it
# really exists: if not, we'll have to fail at runtime.
gen_results_fun = getattr(item, "_gen_results", None)
if not gen_results_fun:
msg = (
f"Cannot generate results for FlowCfg {item.name}: "
"not an instance of a class that defines _gen_results"
)
raise RuntimeError(msg)
Comment on lines +185 to +191
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 Apr 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: a better check would probably be:

            if not isinstance(item, OneShotCfg):
                msg = f"sub-configs of a OneShotCfg should also be OneShotCfgs, not '{type(item)}'"
                raise TypeError(msg)

Because we know that _gen_results should exist on any subclass as a result of the abstract method being defined on the OneShotCfg. I think this is also a fairly reasonable use case for typing.cast, e.g.:

        for item in typing.cast(Sequence[OneShotCfg], self.cfgs):

though some would argue that this might be a bit of a code smell just to appease the type checker.


item_results = [
res
for res in results
if res.block.name == item.name and res.block.variant == item.variant
if res.block.name == item.name and res.block.variant == item_variant
]

result = item._gen_results(item_results)
result = gen_results_fun(item_results)

log.info("[results]: [%s]:\n%s\n", project, result)
log.info("[scratch_path]: [%s] [%s]", project, item.scratch_path)
Expand Down
Loading