Be more careful with variants in OneShotCfg.gen_results#159
Be more careful with variants in OneShotCfg.gen_results#159rswarbrick wants to merge 1 commit intolowRISC:masterfrom
Conversation
We don't necessarily know that all sub-configurations are of type SimCfg. Indeed, this failed for me at runtime when trying an FPV test. We also don't know (from the types) that the sub-configuration defines _gen_results. Practically speaking, it does, so let's just move this to an explicit runtime check, which quiets the type checker. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
rswarbrick
force-pushed
the
variants-in-one-shot-cfg
branch
from
6459cc2 to
489ef55
Compare
| # 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) |
There was a problem hiding this comment.
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?
| 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) |
There was a problem hiding this comment.
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.
2 participants
We don't necessarily know that all sub-configurations are of type SimCfg. Indeed, this failed for me at runtime when trying an FPV test.
We also don't know (from the types) that the sub-configuration defines _gen_results. Practically speaking, it does, so let's just move this to an explicit runtime check, which quiets the type checker.