Skip to content

Tighten up type of Deploy._process_exports #155

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

Tighten up type of Deploy._process_exports #155

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
16 changes: 12 additions & 4 deletions src/dvsim/job/deploy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,16 @@ def __init__(self, sim_cfg: "SimCfg") -> None:
# Do variable substitutions.
self._subst_vars()

# List of vars required to be exported to sub-shell, as a dict.
self.exports = self._process_exports()
# List of vars required to be exported to sub-shell, as a dict. This
# has been loaded from the hjson as a list of dicts and we want to
# flatten it to a single dictionary.
#
# The slightly odd dance with the variables is to convince Pyright
# about the types we end up with.
exports_list: object = self.exports
if not isinstance(exports_list, list):
raise TypeError("Loaded exports value should be a list of dicts.")
self.exports: dict[str, str] = self._process_exports(exports_list)
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 Apr 2, 2026

Choose a reason for hiding this comment

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

This looks reasonable - below I've suggested an alternative pattern which may feel like a bit less of a dance around the type checker, but I'll leave it up to you if you prefer this or your existing solution:

def __init__(self, ...):
    self.exports: list[dict[str, str]] = []
    self.merged_exports: dict[str, str] = {}
    # ... self._define_attrs() then set_attrs, check_attrs, subst_vars, etc. ...
    self.merged_export = self._process_exports(self.exports)
    self.exports.clear()  # if we're being really paranoid about memory usage (likely unnecessary)
    # ...

def get_job_spec(self) -> "JobSpec":
    # ...
        exports=self.merged_exports,
    # ...


# Construct the job's command.
self.cmd = self._construct_cmd()
Expand Down Expand Up @@ -276,7 +284,7 @@ def _subst_vars(self, ignored_subst_vars=None) -> None:
ignore_error=False,
)

def _process_exports(self) -> Mapping:
def _process_exports(self, exports_list: list[dict[str, str]]) -> dict[str, str]:
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 Apr 2, 2026

Choose a reason for hiding this comment

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

Minor nit:

Suggested change
def _process_exports(self, exports_list: list[dict[str, str]]) -> dict[str, str]:
def _process_exports(self, exports_list: Iterable[Mapping[str, str]]) -> dict[str, str]:

"""Convert 'exports' as a list of dicts in the HJson to a dict.

Exports is a list of key-value pairs that are to be exported to the
Expand All @@ -286,7 +294,7 @@ def _process_exports(self) -> Mapping:
into a dict variable, which makes it easy to merge the list of exports
with the subprocess' env where the ASIC tool is invoked.
"""
return {k: str(v) for item in self.exports for k, v in item.items()}
return {k: str(v) for item in exports_list for k, v in item.items()}

def _construct_cmd(self) -> str:
"""Construct the command that will eventually be launched."""
Expand Down
Loading