Tweak how we set up Deploy._variant_suffix#146
Tweak how we set up Deploy._variant_suffix#146rswarbrick wants to merge 1 commit intolowRISC:masterfrom
Conversation
rswarbrick
force-pushed
the
variant-suffix-computation
branch
2 times, most recently
from
03d757b to
d97b094
Compare
Note: we can't do this by checking if self.sim_cfg is of type SimCfg, because that would give a circular dependency between job.deploy and sim.flow. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
rswarbrick
force-pushed
the
variant-suffix-computation
branch
from
d97b094 to
12b4438
Compare
| # 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") |
There was a problem hiding this comment.
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:
- No circular dependencies - we can just add a runtime type check!
- 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.
- Also, before we have rigid dataclasses/models, we can still use
dict.get(key, default)in the meantime instead ofgetattr(self.sim_cfg, attr, default), which I personally think looks a lot nicer to read when used everywhere 😅
| raise TypeError("Unexpected type for variant") | ||
|
|
||
| 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 "" |
There was a problem hiding this comment.
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.
| renew_odir=self.renew_odir, | ||
| log_path=Path(f"{self.odir}/{self.target}.log"), | ||
| links=self.sim_cfg.links, | ||
| links=links, |
There was a problem hiding this comment.
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).
2 participants
Note: we can't do this by checking if self.sim_cfg is of type SimCfg, because that would give a circular dependency between job.deploy and sim.flow.