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
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 😅

Copy link
Copy Markdown
Contributor Author

@rswarbrick rswarbrick Apr 3, 2026

Choose a reason for hiding this comment

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

I'm not sure about the first bit: I think that the code in deploy.py only modifies the FlowCfg in one place, which is the constructor of CovAnalyze (a horrible hack! We can probably eventually get rid of that too)

The idea of extracting the "stuff Deploy cares about" into a dictionary is a good one, though. Indeed, we can do even better than that because we can define some "DeployCfg" class and unconditionally fill everything out, so that the typing is a bit less magical.

Does that sound sensible to you? (as a follow-up)

Copy link
Copy Markdown
Contributor Author

@rswarbrick rswarbrick Apr 3, 2026

Choose a reason for hiding this comment

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

(I've made issue #161 to track the possible improvement)

Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 Apr 3, 2026

Choose a reason for hiding this comment

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

Ah, I grepped for "self.sim_cfg" instead of "sim_cfg", so I missed that instance.

I think the case that you mention (sim_cfg.gui = True to enforce GUI mode for coverage analysis) could also be very easily fixed by just moving this attribute set onto the sim flow itself, before it creates the CovAnalyze (since it knows it needs it at this point). Even better, this should probably either be an implicit conversion with a clear warning log, or just a ValueError/RuntimeError if self.gui == False. Definitely no need for this hacky code in the Deploy! 😄

And with your reference to DeployCfg, yes that seems reasonable - this is similar to the "more rigid type schemas" I mentioned in my second point, where we would use Pydantic models or dataclasses instead of an untyped dictionary.

Thanks for making the issue!


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.

Copy link
Copy Markdown
Contributor Author

@rswarbrick rswarbrick Apr 3, 2026

Choose a reason for hiding this comment

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

Good idea. To avoid scope creep in the PR, I've made an issue to track the fact we can do this as #162.


# 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).

Copy link
Copy Markdown
Contributor Author

@rswarbrick rswarbrick Apr 3, 2026

Choose a reason for hiding this comment

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

I think that probably makes sense (and I confess that my change here was "just making stuff work"). I've just opened up #163 to track doing it properly.

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