Skip to content

Rewrite how OneShotCfg.build_modes gets constructed #158

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
Open

Rewrite how OneShotCfg.build_modes gets constructed #158

Show file tree
Hide file tree
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
37 changes: 27 additions & 10 deletions src/dvsim/flow/one_shot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None:
self.dut = ""
self.fusesoc_core = ""
self.ral_spec = ""
self.build_modes = []
self.build_modes: Sequence[BuildMode] = []
self.run_modes = []
self.regressions = []
self.max_msg_count = -1
Expand Down Expand Up @@ -110,17 +110,34 @@ def _expand(self) -> None:
def _purge(self) -> None:
assert self.scratch_path
log.info("Purging scratch path %s", self.scratch_path)
rm_path(self.scratch_path)
rm_path(Path(self.scratch_path))

def _create_objects(self) -> None:
# Create build and run modes objects
build_modes = Mode.create_modes(BuildMode, self.build_modes)
self.build_modes = build_modes

# All defined build modes are being built, h
# ence extend all with the global opts.
for build_mode in build_modes:
build_mode.build_opts.extend(self.build_opts)
# Create build modes objects based on the names that were stored in
# self.build_modes by the superclass constructor's call to
# _merge_hjson.
#
# We've lied to the type checker about the type of self.build_modes,
# giving it the type that it will contain after this function is
# complete.
mode_dicts: list[dict[object, object]] = []
for mode_dict in self.build_modes:
if not isinstance(mode_dict, dict):
msg = f"Found a build mode item of {mode_dict} when we expected a dict."
raise TypeError(msg)

mode_dicts.append(mode_dict)

base_modes: Sequence[Mode] = Mode.create_modes(BuildMode, mode_dicts)
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.

Not having looked much into the original code for Mode, this seems like the ideal use case for bounded generics. Also, looking at the the method, it should just be a @classmethod, rather than a @staticmethod where we pass in the class. So, something like:

from typing import Type, TypeVar

T = TypeVar("T", bound="Mode")

class Mode:

    @classmethod
    def create_modes(cls: Type[T], mdicts: Sequence[Mapping[str, Any]]) -> list[T]:

This code would then just call e.g.

    base_modes: Sequence[BuildMode] = BuildMode.create_modes(mode_dicts)

with no need for a runtime check on its type.


# All defined build modes are being built, so should be extended with
# the global opts. Tighten up the inferred type at the same time.
self.build_modes = []
for mode in base_modes:
if not isinstance(mode, BuildMode):
raise TypeError("create_modes returned the wrong type")
mode.build_opts.extend(self.build_opts)
self.build_modes.append(mode)

def _print_list(self) -> None:
for list_item in self.list_items:
Expand Down
2 changes: 1 addition & 1 deletion src/dvsim/modes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ def merge_mode(self, mode: "Mode") -> None:
return True

@staticmethod
def create_modes(mode_type: "type[Mode]", mdicts: Mapping) -> Sequence["Mode"]:
def create_modes(mode_type: "type[Mode]", mdicts: Sequence[Mapping]) -> Sequence["Mode"]:
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:

Suggested change
def create_modes(mode_type: "type[Mode]", mdicts: Sequence[Mapping]) -> Sequence["Mode"]:
def create_modes(mode_type: "type[Mode]", mdicts: Iterable[Mapping]) -> Sequence["Mode"]:

Aside: I know these are problems in the original code, but there are a couple of parts that I dislike and so I wanted to mention. Feel free to address them or leave them.

  • Using the generic collection type Mapping is implicitly just Mapping[Any, Any] which disregards all type checks of the keys and values. Since it's loaded JSON the value is indeed Any, but the keys should at least be strings so this is better to explicitly be Iterable[Mapping[str, Any]].
  • I would prefer returning a list["Mode"] here. Since we know we are returning a list, this makes mutability a part of the contract, and stops users needing to copy into another list to mutate it. I see the argument for keeping a more generic interface, but practically this is not very useful and a list is more ergonomic.

"""Create modes of type mode_type.

Use the given list of raw dicts Process dependencies.
Expand Down
Loading