Skip to content

Tweak how we set up Deploy._variant_suffix #146

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
+12 −5
Open

Tweak how we set up Deploy._variant_suffix #146

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
17 changes: 12 additions & 5 deletions src/dvsim/job/deploy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,14 @@ def __init__(self, sim_cfg: "SimCfg") -> None:
self.sim_cfg = sim_cfg
self.flow = sim_cfg.name

if not hasattr(self.sim_cfg, "variant"):
self.sim_cfg.variant = ""
# The sim_cfg argument might be a SimCfg, in which case it might define
# a variant. We don't depend on this, though: if sim_cfg is an instance
# of some other subclass of FlowCfg, just take an empty "variant".
self._variant: str | None = getattr(self.sim_cfg, "variant", None)
if not (isinstance(self._variant, str) or self._variant is None):
raise TypeError("Unexpected type for variant")
Comment on lines +76 to +81
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.

A question: from a cursory glance, it looks like this is the only case where a Deploy is modifying the FlowCfg that is passed into it, correct? It looks like all other accesses are just to read attributes from the expanded config (so as to not pass around a bunch of args).

In that case, to avoid the whole circular dependency, we could just extract the sim_cfg attributes to a dictionary before giving just passing the dict to the Deploy objects instead. If we did this, we get a few benefits:

  1. No circular dependencies - we can just add a runtime type check!
  2. When we have more rigid definitions of which attributes are needed in the HJSON files for which flows, we can eventually migrate these dictionaries to more rigid typed schemas. Then deploys can define which flows/schemas they support.
  3. Also, before we have rigid dataclasses/models, we can still use dict.get(key, default) in the meantime instead of getattr(self.sim_cfg, attr, default), which I personally think looks a lot nicer to read when used everywhere 😅


self._variant_suffix = f"_{self.sim_cfg.variant}" if self.sim_cfg.variant else ""
self._variant_suffix = f"_{self._variant}" if self._variant is not None else ""
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.

Not necessarily for this PR:

This might be a larger refactor, but there is an even better way to get rid of this self._variant_suffix. A while back when refactoring I introduced this IPMeta.variant_name method which combines the name and variant with a separator, which - if you grep for the uses of self._variant_name, is basically all this is being used for throughout this file.

So if we can figure out the right order (maybe it is difficult to do it with the chain of _set_attrs and instead we need to extract these name/variant fields from IPMeta into its own small model/dataclass), we could just reuse that for all of these occurrences.


# A list of jobs on which this job depends.
self.dependencies = []
Expand Down Expand Up @@ -106,6 +110,9 @@ def __init__(self, sim_cfg: "SimCfg") -> None:

def get_job_spec(self) -> "JobSpec":
"""Get the job spec for this deployment."""
# If the FlowCfg object in self.sim_cfg doesn't have a links dictionary, default to {}.
links = getattr(self.sim_cfg, "links", {})

return JobSpec(
name=self.name,
job_type=self.__class__.__name__,
Expand All @@ -115,7 +122,7 @@ def get_job_spec(self) -> "JobSpec":
qual_name=self.qual_name,
block=IPMeta(
name=self.sim_cfg.name,
variant=self.sim_cfg.variant,
variant=self._variant,
commit=self.sim_cfg.commit,
commit_short=self.sim_cfg.commit_short,
branch=self.sim_cfg.branch,
Expand Down Expand Up @@ -143,7 +150,7 @@ def get_job_spec(self) -> "JobSpec":
odir=self.odir,
renew_odir=self.renew_odir,
log_path=Path(f"{self.odir}/{self.target}.log"),
links=self.sim_cfg.links,
links=links,
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: rather than default to an empty dict, should we instead make the links in the base FlowCfg instead? This attribute is being used to specify which directories job logs should be symlinked in by the Launcher objects as they change states. For example, for the sim flows, this is e.g. /path/to/scratch/branch/block/passed/.

Having not run any non-simulation flows - and so perhaps being completely misinformed - I would imagine that we would still want to see the logs and that the Launchers would still try to link the logs based on the status. Might it make more sense to define these links on the base FlowCfg? I would anticipate these status symlink subdirs should look the same regardless of the flow (that is, the fact that a job was running, passing, failed or killed is completely independent of whether it is a sim job or not).

pre_launch=self.pre_launch(),
post_finish=self.post_finish(),
pass_patterns=self.pass_patterns,
Expand Down
Loading