From 61776ff0bff28b907e26369a8ac525cb23cc7ca3 Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Fri, 3 Oct 2025 10:52:38 +0100 Subject: [PATCH 01/13] feat: add deployment object dump debug feature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Debug feature to dump a JSON representation of the `Deploy` objects, just before they are sent to the scheduler. This feature is hidden behind an environment variable to avoid cluttering the CLI interface. The full `Deploy` object is not JSON serialisable in it's current form, so we only serialise the attributes that the `LocalLauncher` uses and some identifying attributes like the full name of the job and the deployment class name. To enable set `DVSIM_DEPLOY_DUMP=true`, run DVSim and there will be a file called `deploy--.json` generated in the scratch directory `scratch/deploy--`. This file can they be compared between runs to check for regressions in the flow leading up to job deployment. With `--fixed-seed=1 --branch=baseline`, the resulting json files should be identical. Setting the "branch" doesn't actually change the git branch, it just overrides what DVSim thinks is the branch as far as paths in the scratch directory are concerned. We need to either set this to something fixed so that the deployment objects contain the same paths, otherwise a diff will fail. Generating a `diff` or hash of the two files and comparing shows if the job deployments would be identical or not. Using this functionality I have confirmed, by backporting this commit to the first release tag, that none of the refactorings made so far in this repository have changed the deployment objects in such a way that the launched jobs are different in any way. ``` ✦ ❯ sha512sum baseline.json deploy_25.10.03_11.35.33.json af732c3011753cfc7dc40068e1ce9b6cf3624973ffbbd25f828e4d7727f27dd65b5ada19407500315ea6f279946222d185dc939fe4978b0b32b3e7e8b4f7f60c baseline.json af732c3011753cfc7dc40068e1ce9b6cf3624973ffbbd25f828e4d7727f27dd65b5ada19407500315ea6f279946222d185dc939fe4978b0b32b3e7e8b4f7f60c deploy_25.10.03_11.35.33.json ``` The first JSON file was generated from backporting the this feature to the tagged commit. The second file was generated with this branch and includes all the tidyup work made so far to DVSim. The DVSim command used is: ``` DVSIM_DEPLOY_DUMP=true dvsim \ ~/base/opentitan/hw/top_earlgrey/dv/top_earlgrey_sim_cfgs.hjson \ --proj-root ~/base/opentitan -i nightly --scratch ~/scratch \ --branch baseline --fixed-seed 1 \ --verbose=debug --max-parallel=50 \ --cov ``` **Note:** *your hashes will be different from mine as the directory paths will be different. However if you run this against the same versions your hashes should be the same as each other.* **Note:** *Depending on the flags you pass to DVSim there may be some minor indeterminism in the configuration. For example with CovMerge the coverage directories are not always given in the same order. It can take several runs to get a different hash, or it could be different on every run. In such cases it's worth using `diff` across the files to see what the actual differences are, they may not be consequential.* Signed-off-by: James McCorrie --- src/dvsim/flow/base.py | 14 ++++++++++++++ src/dvsim/job/deploy.py | 21 +++++++++++++++++++++ src/dvsim/launcher/local.py | 6 +++++- 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/dvsim/flow/base.py b/src/dvsim/flow/base.py index 619526d5..a54bed3d 100644 --- a/src/dvsim/flow/base.py +++ b/src/dvsim/flow/base.py @@ -4,6 +4,7 @@ """Flow config base class.""" +import json import os import pprint import sys @@ -411,6 +412,19 @@ def deploy_objects(self) -> Mapping[Deploy, str]: log.error("Nothing to run!") sys.exit(1) + if os.environ.get("DVSIM_DEPLOY_DUMP", "true"): + filename = f"deploy_{self.branch}_{self.timestamp}.json" + (Path(self.scratch_root) / filename).write_text( + json.dumps( + # Sort on full name to ensure consistent ordering + sorted( + [d.model_dump() for d in deploy], + key=lambda d: d["full_name"], + ), + indent=2, + ), + ) + return Scheduler( items=deploy, launcher_cls=get_launcher_cls(), diff --git a/src/dvsim/job/deploy.py b/src/dvsim/job/deploy.py index cebd66be..1a744494 100644 --- a/src/dvsim/job/deploy.py +++ b/src/dvsim/job/deploy.py @@ -334,6 +334,27 @@ def create_launcher(self) -> None: # Retain the handle to self for lookup & callbacks. self.launcher = get_launcher(self) + def model_dump(self) -> Mapping: + """Dump the deployment object to mapping object. + + This method matches the interface provided by pydantic models to dump a + subset of the class attributes + + Returns: + Representation of a deployment object as a dict. + + """ + return { + "full_name": self.full_name, + "type": self.__class__.__name__, + "exports": self.exports, + "interactive": self.sim_cfg.interactive, + "log_path": self.get_log_path(), + "timeout_mins": self.get_timeout_mins(), + "cmd": self.cmd, + "gui": self.gui, + } + class CompileSim(Deploy): """Abstraction for building the simulation executable.""" diff --git a/src/dvsim/launcher/local.py b/src/dvsim/launcher/local.py index 8f9bec9f..eecc3ce6 100644 --- a/src/dvsim/launcher/local.py +++ b/src/dvsim/launcher/local.py @@ -9,9 +9,13 @@ import shlex import subprocess from pathlib import Path +from typing import TYPE_CHECKING from dvsim.launcher.base import ErrorMessage, Launcher, LauncherBusyError, LauncherError +if TYPE_CHECKING: + from dvsim.job.deploy import Deploy + class LocalLauncher(Launcher): """Implementation of Launcher to launch jobs in the user's local workstation.""" @@ -19,7 +23,7 @@ class LocalLauncher(Launcher): # Poll job's completion status every this many seconds poll_freq = 0.025 - def __init__(self, deploy) -> None: + def __init__(self, deploy: "Deploy") -> None: """Initialize common class members.""" super().__init__(deploy) From ca9405a60e03538d8c9e3d285a5f143cef2d4235 Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Thu, 2 Oct 2025 16:41:11 +0100 Subject: [PATCH 02/13] fix: results refactor name clash Signed-off-by: James McCorrie --- src/dvsim/flow/sim.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/dvsim/flow/sim.py b/src/dvsim/flow/sim.py index dce93af7..21b5796d 100644 --- a/src/dvsim/flow/sim.py +++ b/src/dvsim/flow/sim.py @@ -807,7 +807,7 @@ def create_bucket_report(buckets): return fail_msgs deployed_items = self.deploy - results = SimResults(deployed_items, results) + sim_results = SimResults(deployed_items, results) # Generate results table for runs. results_str = "## " + self.results_title + "\n" @@ -857,13 +857,13 @@ def create_bucket_report(buckets): if self.build_seed and not self.run_only: results_str += f"### Build randomization enabled with --build-seed {self.build_seed}\n" - if not results.table: + if not sim_results.table: results_str += "No results to display.\n" else: # Map regr results to the testplan entries. if not self.testplan.test_results_mapped: - self.testplan.map_test_results(test_results=results.table) + self.testplan.map_test_results(test_results=sim_results.table) results_str += self.testplan.get_test_results_table( map_full_testplan=self.map_full_testplan, @@ -889,9 +889,9 @@ def create_bucket_report(buckets): else: self.results_summary["Coverage"] = "--" - if results.buckets: + if sim_results.buckets: self.errors_seen = True - results_str += "\n".join(create_bucket_report(results.buckets)) + results_str += "\n".join(create_bucket_report(sim_results.buckets)) self.results_md = results_str return results_str From 5b954566d2bcbc3b63ce892fc46e446aee1afddd Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Wed, 26 Mar 2025 10:39:40 +0000 Subject: [PATCH 03/13] feat: [git] add repo root helper and migrate to GitPython library With the nightly regression running functionality more git interaction is required. Rather than implementing everything from scratch using subprocess calls and maintaining it, add the GitPython library as a dependency and use that for the git interactions. The repo root helper is implemented using Pathlib as the GitPython library doesn't seem to support this functionality. It requires a path to the repo root exactly and fails if it doesn't get a valid repo path. Signed-off-by: James McCorrie --- pyproject.toml | 1 + src/dvsim/utils/git.py | 35 ++++++++++++++++++++--- tests/utils/test_git.py | 52 +++++++++++++++++++++++++++++++++++ tests/utils/test_wildcards.py | 2 ++ uv.lock | 35 +++++++++++++++++++++++ 5 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 tests/utils/test_git.py diff --git a/pyproject.toml b/pyproject.toml index 5467585b..b439741f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -13,6 +13,7 @@ dependencies = [ # Keep sorted "click>=8.1.7", "enlighten>=1.12.4", + "gitpython>=3.1.44", "hjson>=3.1.0", "logzero>=1.7.0", "mistletoe>=1.4.0", diff --git a/src/dvsim/utils/git.py b/src/dvsim/utils/git.py index 4dfc9a5f..c75612ab 100644 --- a/src/dvsim/utils/git.py +++ b/src/dvsim/utils/git.py @@ -4,11 +4,38 @@ """Git utinity functions.""" -from dvsim.utils.subprocess import run_cmd +from pathlib import Path -__all__ = ("git_commit_hash",) +from git import Repo +from dvsim.logging import log -def git_commit_hash() -> str: +__all__ = ("repo_root",) + + +def repo_root(path: Path) -> Path | None: + """Given a sub dir in a git repo provide the root path. + + Where the provided path is not part of a repo then None is returned. + """ + if (path / ".git").exists(): + return path + + for p in path.parents: + if (p / ".git").exists(): + return p + + return None + + +def git_commit_hash(path: Path | None = None) -> str: """Hash of the current git commit.""" - return run_cmd("git rev-parse HEAD") + root = repo_root(path=path if path else Path.cwd()) + + if root is None: + log.error("no git repo found at %s", root) + raise ValueError + + r = Repo(root) + + return r.head.commit.hexsha diff --git a/tests/utils/test_git.py b/tests/utils/test_git.py new file mode 100644 index 00000000..826eacb2 --- /dev/null +++ b/tests/utils/test_git.py @@ -0,0 +1,52 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Test git helpers.""" + +from pathlib import Path + +from git import Repo +from hamcrest import assert_that, equal_to + +from dvsim.utils.git import git_commit_hash, repo_root + +__all__ = () + + +class TestGit: + """Test git helpers.""" + + @staticmethod + def test_repo_root(tmp_path: Path) -> None: + """Test git repo root can be found.""" + repo_root_path = tmp_path / "repo" + repo_root_path.mkdir() + + Repo.init(path=repo_root_path) + + # from the actual repo root + assert_that(repo_root(path=repo_root_path), equal_to(repo_root_path)) + + # from the repo sub dir + sub_dir_path = repo_root_path / "a" + sub_dir_path.mkdir() + assert_that(repo_root(path=sub_dir_path), equal_to(repo_root_path)) + + # from outside the repo + assert_that(repo_root(path=tmp_path), equal_to(None)) + + @staticmethod + def test_git_commit_hash(tmp_path: Path) -> None: + """Test that the expected git commit sha is returned.""" + r = Repo.init(path=tmp_path) + + file = tmp_path / "a" + file.write_text("file to commit") + r.index.add([file]) + r.index.commit("initial commit") + + assert_that( + git_commit_hash(tmp_path), + equal_to(r.head.commit.hexsha), + ) diff --git a/tests/utils/test_wildcards.py b/tests/utils/test_wildcards.py index 02d81490..7b535549 100644 --- a/tests/utils/test_wildcards.py +++ b/tests/utils/test_wildcards.py @@ -16,6 +16,8 @@ subst_wildcards, ) +__all__ = ("TestSubstWildcards",) + class TestSubstWildcards: """Test subst_wildcards.""" diff --git a/uv.lock b/uv.lock index 1005908c..bb3013c4 100644 --- a/uv.lock +++ b/uv.lock @@ -276,6 +276,7 @@ source = { editable = "." } dependencies = [ { name = "click" }, { name = "enlighten" }, + { name = "gitpython" }, { name = "hjson" }, { name = "logzero" }, { name = "mistletoe" }, @@ -332,6 +333,7 @@ requires-dist = [ { name = "dvsim", extras = ["linting", "typing", "test"], marker = "extra == 'dev'" }, { name = "dvsim", extras = ["typing", "test"], marker = "extra == 'nix'" }, { name = "enlighten", specifier = ">=1.12.4" }, + { name = "gitpython", specifier = ">=3.1.44" }, { name = "hjson", specifier = ">=3.1.0" }, { name = "logzero", specifier = ">=1.7.0" }, { name = "mistletoe", specifier = ">=1.4.0" }, @@ -385,6 +387,30 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/c1/ea/53f2148663b321f21b5a606bd5f191517cf40b7072c0497d3c92c4a13b1e/executing-2.2.1-py2.py3-none-any.whl", hash = "sha256:760643d3452b4d777d295bb167ccc74c64a81df23fb5e08eff250c425a4b2017", size = 28317, upload-time = "2025-09-01T09:48:08.5Z" }, ] +[[package]] +name = "gitdb" +version = "4.0.12" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "smmap" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/72/94/63b0fc47eb32792c7ba1fe1b694daec9a63620db1e313033d18140c2320a/gitdb-4.0.12.tar.gz", hash = "sha256:5ef71f855d191a3326fcfbc0d5da835f26b13fbcba60c32c21091c349ffdb571", size = 394684 } +wheels = [ + { url = "https://files.pythonhosted.org/packages/a0/61/5c78b91c3143ed5c14207f463aecfc8f9dbb5092fb2869baf37c273b2705/gitdb-4.0.12-py3-none-any.whl", hash = "sha256:67073e15955400952c6565cc3e707c554a4eea2e428946f7a4c162fab9bd9bcf", size = 62794 }, +] + +[[package]] +name = "gitpython" +version = "3.1.44" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "gitdb" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/c0/89/37df0b71473153574a5cdef8f242de422a0f5d26d7a9e231e6f169b4ad14/gitpython-3.1.44.tar.gz", hash = "sha256:c87e30b26253bf5418b01b0660f818967f3c503193838337fe5e573331249269", size = 214196 } +wheels = [ + { url = "https://files.pythonhosted.org/packages/1d/9a/4114a9057db2f1462d5c8f8390ab7383925fe1ac012eaa42402ad65c2963/GitPython-3.1.44-py3-none-any.whl", hash = "sha256:9e0e10cda9bed1ee64bc9a6de50e7e38a9c9943241cd7f585f6df3ed28011110", size = 207599 }, +] + [[package]] name = "hjson" version = "3.1.0" @@ -995,6 +1021,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/28/7e/61c42657f6e4614a4258f1c3b0c5b93adc4d1f8575f5229d1906b483099b/ruff-0.12.12-py3-none-win_arm64.whl", hash = "sha256:2a8199cab4ce4d72d158319b63370abf60991495fb733db96cd923a34c52d093", size = 12256762, upload-time = "2025-09-04T16:50:15.737Z" }, ] +[[package]] +name = "smmap" +version = "5.0.2" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/44/cd/a040c4b3119bbe532e5b0732286f805445375489fceaec1f48306068ee3b/smmap-5.0.2.tar.gz", hash = "sha256:26ea65a03958fa0c8a1c7e8c7a58fdc77221b8910f6be2131affade476898ad5", size = 22329 } +wheels = [ + { url = "https://files.pythonhosted.org/packages/04/be/d09147ad1ec7934636ad912901c5fd7667e1c858e19d355237db0d0cd5e4/smmap-5.0.2-py3-none-any.whl", hash = "sha256:b30115f0def7d7531d22a0fb6502488d879e75b260a9db4d0819cfb25403af5e", size = 24303 }, +] + [[package]] name = "stack-data" version = "0.6.3" From b4c99b06f5c02dab5bec63583515649864fd9dcf Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Wed, 26 Mar 2025 09:59:06 +0000 Subject: [PATCH 04/13] refactor: [project] move out the project level config functions. The cli module should probably be a thin wrapper around internal functions rather than containing a fairly large amount of business logic. This commit copies out and improves this logic. Signed-off-by: James McCorrie --- src/dvsim/project.py | 299 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 299 insertions(+) create mode 100644 src/dvsim/project.py diff --git a/src/dvsim/project.py b/src/dvsim/project.py new file mode 100644 index 00000000..899ffd0d --- /dev/null +++ b/src/dvsim/project.py @@ -0,0 +1,299 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""DVSim project.""" + +import os +import shlex +import subprocess +import sys +from dataclasses import dataclass +from pathlib import Path + +from dvsim.logging import log +from dvsim.utils import ( + rm_path, + run_cmd_with_timeout, +) + +__all__ = ("init_project",) + + +@dataclass(frozen=True, kw_only=True) +class ProjectMeta: + """Project meta data.""" + + top_cfg_path: Path + root_path: Path + src_path: Path + scratch_path: Path + branch: str + job_prefix: str + + +def init_project( + cfg_path: Path, + proj_root: Path | None, + scratch_root: Path | None, + branch: str, + *, + job_prefix: str = "", + purge: bool = False, + dry_run: bool = False, + remote: bool = False, +) -> ProjectMeta: + """Initialise a project workspace. + + If --remote switch is set, a location in the scratch area is chosen as the + new proj_root. The entire repo is copied over to this location. Else, the + proj_root is discovered using get_proj_root() method, unless the user + overrides it on the command line. + + This function returns the updated proj_root src and destination path. If + --remote switch is not set, the destination path is identical to the src + path. Likewise, if --dry-run is set. + """ + if not cfg_path.exists(): + log.fatal("Path to config file %s appears to be invalid.", cfg_path) + sys.exit(1) + + branch = resolve_branch(branch) + + src_path = Path(proj_root) if proj_root else get_proj_root() + + scratch_path = resolve_scratch_root( + scratch_root, + src_path, + ) + + # Check if jobs are dispatched to external compute machines. If yes, + # then the repo needs to be copied over to the scratch area + # accessible to those machines. + # If --purge arg is set, then purge the repo_top that was copied before. + if remote and not dry_run: + root_path = scratch_path / branch / "repo_top" + if purge: + rm_path(root_path) + copy_repo(src_path, root_path) + else: + root_path = src_path + + log.info("[proj_root]: %s", root_path) + + # Create an empty FUSESOC_IGNORE file in scratch_root. This ensures that + # any fusesoc invocation from a job won't search within scratch_root for + # core files. + (scratch_path / "FUSESOC_IGNORE").touch() + + cfg_path = cfg_path.resolve() + if remote: + cfg_path = root_path / cfg_path.relative_to(src_path) + + return ProjectMeta( + top_cfg_path=cfg_path, + root_path=root_path, + src_path=src_path, + scratch_path=scratch_path, + branch=branch, + job_prefix=job_prefix, + ) + + +def _network_dir_accessible_and_exists( + path: Path, + timeout: int = 1, +) -> bool: + """Check network path is accessible and exists with timeout. + + Path could be mounted in a filesystem (such as NFS) on + a network drive. If the network is down, it could cause the + access access check to hang. So run a simple ls command with a + timeout to prevent the hang. + + Args: + path: the directory path to check + timeout: number of seconds to wait before giving up + + Returns: + True if the directory was listable otherwise False + + """ + (out, status) = run_cmd_with_timeout( + cmd="ls -d " + str(path), + timeout=timeout, + exit_on_failure=False, + ) + + return status == 0 and out != "" + + +def _ensure_dir_exists_and_accessible(path: Path) -> None: + """Directory exists and is accessible.""" + try: + path.mkdir(exist_ok=True, parents=True) + except PermissionError as e: + log.fatal( + f"Failed to create dir {path}:\n{e}.", + ) + sys.exit(1) + + if not os.access(path, os.W_OK): + log.fatal(f"Path {path} is not writable!") + sys.exit(1) + + +def resolve_scratch_root( + arg_scratch_root: Path | None, + proj_root: Path, +) -> Path: + """Resolve the scratch root directory. + + Among the available options: + If set on the command line, then use that as a preference. + Else, check if $SCRATCH_ROOT env variable exists and is a directory. + Else use the default (/scratch) + + Try to create the directory if it does not already exist. + """ + scratch_root_env = os.environ.get("SCRATCH_ROOT") + + if arg_scratch_root: + scratch_root = Path(os.path.realpath(str(arg_scratch_root))) + + elif scratch_root_env: + resolved_path = Path(os.path.realpath(scratch_root_env)) + + if _network_dir_accessible_and_exists(resolved_path): + scratch_root = resolved_path + + else: + log.warning('Scratch root "%s" is not accessible', resolved_path) + + scratch_root = proj_root / "scratch" + else: + scratch_root = proj_root / "scratch" + + log.info('Using scratch root "%s"', scratch_root) + + _ensure_dir_exists_and_accessible(scratch_root) + + return scratch_root + + +def get_proj_root() -> Path: + """Get the project root directory path. + + this is used to construct the full paths. + """ + cmd = ["git", "rev-parse", "--show-toplevel"] + result = subprocess.run( + cmd, + capture_output=True, + check=False, + ) + + proj_root = result.stdout.decode("utf-8").strip() + + if not proj_root: + cmd_line = " ".join(cmd) + err_str = result.stderr.decode("utf-8") + + log.error( + "Attempted to find the root of this GitHub repository by running:" + "\n%s\nBut this command has failed:\n%s", + cmd_line, + err_str, + ) + sys.exit(1) + + return Path(proj_root) + + +def copy_repo(src: Path, dest: Path) -> None: + """Copy over the repo to a new location. + + The repo is copied over from src to dest area. It tentatively uses the + rsync utility which provides the ability to specify a file containing some + exclude patterns to skip certain things from being copied over. With GitHub + repos, an existing `.gitignore` serves this purpose pretty well. + """ + rsync_cmd = [ + "rsync", + "--recursive", + "--links", + "--checksum", + "--update", + "--inplace", + "--no-group", + ] + + # Supply `.gitignore` from the src area to skip temp files. + ignore_patterns_file = src / ".gitignore" + if ignore_patterns_file.exists(): + # TODO: hack - include hw/foundry since it is excluded in .gitignore. + rsync_cmd += [ + "--include=hw/foundry", + f"--exclude-from={ignore_patterns_file}", + "--exclude=.*", + ] + + rsync_cmd += [str(src / "."), str(dest)] + + cmd = [ + "flock", + "--timeout", + "600", + dest, + "--command", + " ".join([shlex.quote(w) for w in rsync_cmd]), + ] + + log.info("[copy_repo] [dest]: %s", dest) + log.verbose("[copy_repo] [cmd]: \n%s", " ".join(cmd)) + + # Make sure the dest exists first. + dest.mkdir(parents=True, exist_ok=True) + try: + subprocess.run( + cmd, + check=True, + capture_output=True, + ) + except subprocess.CalledProcessError as e: + log.exception( + "Failed to copy over %s to %s: %s", + src, + dest, + e.stderr.decode("utf-8").strip(), + ) + log.info("Done.") + + +def resolve_branch(branch: str | None) -> str: + """Choose a branch name for output files. + + If the --branch argument was passed on the command line, the branch + argument is the branch name to use. Otherwise it is None and we use git to + find the name of the current branch in the working directory. + + Note, as this name will be used to generate output files any forward + slashes are replaced with single dashes to avoid being interpreted as + directory hierarchy. + """ + if branch is not None: + return branch.replace("/", "-") + + result = subprocess.run( + ["git", "rev-parse", "--abbrev-ref", "HEAD"], + stdout=subprocess.PIPE, + check=False, + ) + branch = result.stdout.decode("utf-8").strip().replace("/", "-") + if not branch: + log.warning( + 'Failed to find current git branch. Setting it to "default"', + ) + branch = "default" + + return branch From 0e0862f66c3b177649840b861e3535a4a4feb533 Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Tue, 30 Sep 2025 15:28:46 +0100 Subject: [PATCH 05/13] refactor: [config] improved config loading with testing Signed-off-by: James McCorrie --- src/dvsim/config/__init__.py | 5 + src/dvsim/config/errors.py | 26 + src/dvsim/config/load.py | 511 ++++++++++++++++++ src/dvsim/examples/testplanner/foo_dv_doc.md | 1 + tests/config/__init__.py | 5 + tests/config/example_cfgs/a.hjson | 4 + tests/config/example_cfgs/a/circular.hjson | 22 + tests/config/example_cfgs/b.hjson | 4 + tests/config/example_cfgs/basic.hjson | 10 + tests/config/example_cfgs/c.hjson | 4 + .../example_cfgs/child_cfgs/child_1.hjson | 20 + .../example_cfgs/child_cfgs/child_2.hjson | 16 + tests/config/example_cfgs/common.hjson | 8 + tests/config/example_cfgs/config | 4 + tests/config/example_cfgs/config.invalid | 4 + tests/config/example_cfgs/empty.hjson | 4 + .../example_cfgs/import_cfg_not_list.hjson | 8 + tests/config/example_cfgs/list_on_top.hjson | 8 + tests/config/example_cfgs/parent.hjson | 16 + .../config/example_cfgs/sub_root/other.hjson | 8 + tests/config/example_cfgs/values.hjson | 8 + tests/config/example_cfgs/with_imports.hjson | 11 + tests/config/test_load.py | 456 ++++++++++++++++ 23 files changed, 1163 insertions(+) create mode 100644 src/dvsim/config/__init__.py create mode 100644 src/dvsim/config/errors.py create mode 100644 src/dvsim/config/load.py create mode 100644 tests/config/__init__.py create mode 100644 tests/config/example_cfgs/a.hjson create mode 100644 tests/config/example_cfgs/a/circular.hjson create mode 100644 tests/config/example_cfgs/b.hjson create mode 100644 tests/config/example_cfgs/basic.hjson create mode 100644 tests/config/example_cfgs/c.hjson create mode 100644 tests/config/example_cfgs/child_cfgs/child_1.hjson create mode 100644 tests/config/example_cfgs/child_cfgs/child_2.hjson create mode 100644 tests/config/example_cfgs/common.hjson create mode 100644 tests/config/example_cfgs/config create mode 100644 tests/config/example_cfgs/config.invalid create mode 100644 tests/config/example_cfgs/empty.hjson create mode 100644 tests/config/example_cfgs/import_cfg_not_list.hjson create mode 100644 tests/config/example_cfgs/list_on_top.hjson create mode 100644 tests/config/example_cfgs/parent.hjson create mode 100644 tests/config/example_cfgs/sub_root/other.hjson create mode 100644 tests/config/example_cfgs/values.hjson create mode 100644 tests/config/example_cfgs/with_imports.hjson create mode 100644 tests/config/test_load.py diff --git a/src/dvsim/config/__init__.py b/src/dvsim/config/__init__.py new file mode 100644 index 00000000..6093d2ab --- /dev/null +++ b/src/dvsim/config/__init__.py @@ -0,0 +1,5 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Configuration for DVSim.""" diff --git a/src/dvsim/config/errors.py b/src/dvsim/config/errors.py new file mode 100644 index 00000000..9362cffd --- /dev/null +++ b/src/dvsim/config/errors.py @@ -0,0 +1,26 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Custom Exception classes for config handling.""" + + +class ConflictingConfigValueError(Exception): + """Config values can not be merged as their values conflict.""" + + def __init__(self, *, key: str, value: object, new_value: object) -> None: + """Initialise the Error object. + + Args: + key: the mapping key + value: initial value + new_value: the value in the other config + + """ + self.key = key + self.value = value + self.new_value = new_value + + super().__init__( + f"Values {value!r} and {new_value!r} are in conflict as they cannot be merged", + ) diff --git a/src/dvsim/config/load.py b/src/dvsim/config/load.py new file mode 100644 index 00000000..6214f5bd --- /dev/null +++ b/src/dvsim/config/load.py @@ -0,0 +1,511 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Helper function for loading DVSim config files. + +## Formats + +Load config files in several formats including HJSON, JSON, and TOML. These +formats can be extended by registering a new decoder to a file extension. + +## Include other config files + +Configuration files can include other configuration files by including a key +named `import_cfgs` which contains a list of paths to other config files to +import. These are resolved and recursively evaluated such that the resulting +loaded configuration dictionary is the union of all the included configuration +files. The format of included configuration files need not be the same as the +parent config file. + +## Primary and Child config files + +The top level or primary config file may also contain child configuration +files. These are not merged into the primary configuration, but rather +registered as a child of the primary config. Child configurations are listed in +the `use_cfgs` attribute on the primary config. + +NOTE: at the moment only the top level config (primary config) can contain +child configs. Configs that contain `use_cfgs` can not be listed in another +config files `use_cfgs` attribute. + +NOTE: the list of merged configurations (`import_cfgs`) is removed from the +resulting config mapping data during the load process. This is to keep the data +clean at each stage passing on only the data the next stage needs. However for +debugging purposes it could be useful to preserve the provinance of each config +value. This information is not provided by preserving the list of imported +config paths (pre wildcard substitution), as the values could be a result of +different merged files... and wildcard substitutions. +""" + +from collections.abc import ( + Callable, + Iterable, + Mapping, + Sequence, +) +from pathlib import Path + +from dvsim.config.errors import ConflictingConfigValueError +from dvsim.logging import log +from dvsim.utils.hjson import decode_hjson + +__all__ = ("load_cfg",) + +_CFG_DECODERS = { + "hjson": decode_hjson, +} + + +def _get_decoder(path: Path) -> Callable[[str], Mapping]: + """Get a config decoder.""" + ext = path.suffix + if not ext: + log.error("Config file name '%s' requires an extension", path) + raise ValueError + + cfg_format = ext[1:] + + if cfg_format not in _CFG_DECODERS: + log.error( + "Config file '%s' is of unsupported format '%s', supported formats are: %s.", + path, + cfg_format, + str(list(_CFG_DECODERS.keys())), + ) + raise TypeError + + return _CFG_DECODERS[cfg_format] + + +def _load_cfg_file(path: Path) -> Mapping[str, object]: + """Load a single config file.""" + decoder = _get_decoder(path) + + data = decoder(path.read_text()) + + if not data: + msg = f"{path!r}: config file is empty" + raise ValueError( + msg, + ) + + if not isinstance(data, Mapping): + msg = f"{path!r}: Top-level config object is not a dictionary." + raise TypeError( + msg, + ) + + return data + + +def _simple_path_wildcard_resolve( + path: str, + wildcard_values: Mapping[str, object], +) -> Path: + """Resolve wildcards in a string path.""" + # Substitute any wildcards - only simple wildcards are supported for + # the config file paths. So the standard library format_map is enough. + while True: + if "{" not in path: + break + + try: + path = path.format_map(wildcard_values) + + except KeyError as e: + e.add_note( + f"Unresolved wildcards while resolving path: {path}", + ) + raise + + return Path(path) + + +def _resolve_cfg_path( + path: Path | str, + include_paths: Iterable[Path], + wildcard_values: Mapping[str, object], +) -> Path: + """Resolve a config file path. + + If the path is a string, then substitute any wildcards found in the string + and convert to a Path object before further processing. + + If the path is a Path that exists then return the resolved version of the + path object, converting relative paths to absolute paths. If the provided + path is already absolute and the file it points to does not exist then + raise a Value error. Otherwise treat the path as a relative path and look + for the first file that exists relative to the provided include_paths. + + Args: + path: the path to resolve + include_paths: the provided path may be a relative path to one of these + provided base paths. + wildcard_values: the path may be a string containing wildcards which + can be substituted with the values provided in this mapping. + + Returns: + Path object that is a fully resolved path + + """ + orig_path = path + if isinstance(path, str): + path = _simple_path_wildcard_resolve( + path=path, + wildcard_values=wildcard_values, + ) + + # Resolve path relative to the provided include paths + if not path.is_absolute(): + include_paths = list(include_paths) + log.debug( + "Trying to resolve as a relative path using include paths [%s]", + ",".join(str(path) for path in include_paths), + ) + + found_path = None + for base_path in include_paths: + potential_path = base_path / path + + if potential_path.exists(): + found_path = potential_path + break + + if found_path is None: + log.error( + "'%s' is not an absolute path, and can't find an existing file" + "relative to the include_paths: [%s]", + path, + ",".join(str(path) for path in include_paths), + ) + raise ValueError + + path = found_path + + path = path.resolve() + + log.debug("Resolved '%s' -> '%s'", orig_path, path) + + if not path.exists(): + log.error("Resolved path '%s' does not exist", path) + raise FileNotFoundError(str(path)) + + return path + + +def _extract_config_paths( + data: Mapping, + field: str, + include_paths: Iterable[Path], + wildcard_values: Mapping[str, object], +) -> Sequence[Path]: + """Extract config import paths. + + Args: + data: config data which contains both config and include paths for + other config files. + field: name of the field containing paths to extract and resolve + include_paths: the provided path may be a relative path to one of these + provided base paths. + wildcard_values: the path may be a string containing wildcards which + can be substituted with the values provided in this mapping. + + Returns: + tuple containing config data and an iterable of config paths as str. + The config paths may contain wildcards that need expanding before they + can become valid Path objects. + + """ + if field not in data: + return [] + + import_cfgs = data[field] + + if not isinstance(import_cfgs, list): + msg = (f"{field} is not a list of strings as expected: {import_cfgs!r}",) + log.error(msg) + raise TypeError(msg) + + return [ + _resolve_cfg_path( + path=path, + include_paths=include_paths, + wildcard_values=wildcard_values, + ) + for path in import_cfgs + ] + + +def _merge_cfg_map( + obj: Mapping, + other: Mapping, +) -> Mapping[str, object]: + """Merge configuration values from two config maps. + + This operation is recursive. Mapping types are merged where the values of + the other config take precedent. + + Args: + obj: initial config map + other: config map to merge into the initial config map + + Returns: + New map containing the merged config values. + + """ + new = dict(obj).copy() + + for k, v in other.items(): + # key doesn't exist yet, just copy the value over + if k not in new: + new[k] = v + continue + + initial = new[k] + + # recursively merge maps + if isinstance(initial, Mapping) and isinstance(v, Mapping): + new[k] = _merge_cfg_map(initial, v) + continue + + # merge sequence by extending the initial list with the values in the + # other sequence + if ( + # str is a Sequence but it should not be concatenated + not isinstance(initial, str) + and not isinstance(v, str) + and isinstance(initial, Sequence) + and isinstance(v, Sequence) + ): + new[k] += v + continue + + # New value is the same as the old one, or empty + if initial == v or not v: + continue + + # initial value is empty so replace it with new value + if not initial: + new[k] = v + continue + + raise ConflictingConfigValueError(key=k, value=initial, new_value=v) + + return new + + +def _merge_import_cfgs( + top_cfg: Mapping[str, object], + *, + include_paths: Iterable, + wildcard_values: Mapping, +) -> Mapping[str, object]: + """Recursively resolve and merge import configs. + + Returns: + mapping of config keys to values removing the 'import_cfgs' field. + + """ + import_cfgs = _extract_config_paths( + data=top_cfg, + field="import_cfgs", + include_paths=include_paths, + wildcard_values=wildcard_values, + ) + + if not import_cfgs: + return top_cfg + + # Make the config mapping mutable so the import configs can be merged into + # the top config. + cfg = dict(top_cfg) + + log.verbose( + "config directly imports:\n - %s", + "\n - ".join(str(path) for path in import_cfgs), + ) + + # Take a mutable copy of the import config paths + remaining_cfgs = list(import_cfgs) + parsed_cfgs = set() + while remaining_cfgs: + next_cfg_path = remaining_cfgs.pop() + + # If already merged a config file then skip. This allows duplicate + # imported configs, where for example a common set of definitions can + # be included in multiple config files. Config files already parsed and + # merged are skipped. + if next_cfg_path in parsed_cfgs: + continue + + parsed_cfgs.add(next_cfg_path) + + # load the imported config file + inc_cfg = _load_cfg_file(next_cfg_path) + cfg = _merge_cfg_map(obj=cfg, other=inc_cfg) + + inc_import_cfgs = _extract_config_paths( + data=inc_cfg, + field="import_cfgs", + include_paths=include_paths, + wildcard_values={ + **wildcard_values, + **cfg, # include new config values in path filename resolution + }, + ) + + # queue included configs to process + if inc_import_cfgs: + log.verbose( + "config indirectly imports:\n - %s", + "\n - ".join(str(path) for path in inc_import_cfgs), + ) + remaining_cfgs.extend(inc_import_cfgs) + + # Create a filtered copy without import_cfgs key to keep the config dict + # clean, the field has no further value. + return {k: v for k, v in cfg.items() if k != "import_cfgs"} + + +def _merge_use_cfgs( + top_cfg: Mapping[str, object], + *, + include_paths: Iterable, + wildcard_values: Mapping, + select_cfgs: Sequence[str] | None = None, +) -> Mapping[str, object]: + """Merge in the configuration files in use_cfgs field. + + Process the list of child configuration files in use_cfgs field and store + the resulting config data on a new `cfgs` mapping of resolved paths to + config data. + + Returns: + Mapping containing filtering out the 'use_cfgs' field and instead + containing the configuration mapping data from those files in a new + 'cfgs' field. + + """ + use_cfgs = _extract_config_paths( + data=top_cfg, + field="use_cfgs", + include_paths=include_paths, + wildcard_values=wildcard_values, + ) + + if not use_cfgs: + return top_cfg + + # Make the config mapping mutable so the import configs can be merged into + # the top config. + cfg = dict(top_cfg) + + cfg["cfgs"] = { + path: load_cfg( + path, + include_paths=include_paths, + path_resolution_wildcards=wildcard_values, + ) + for path in use_cfgs + } + + # Filter by selected configs if provided + if select_cfgs: + cfg["cfgs"] = { + path: child_cfg + for path, child_cfg in cfg["cfgs"].items() + if child_cfg["name"] in select_cfgs + } + + return {k: v for k, v in cfg.items() if k != "use_cfgs"} + + +def load_cfg( + path: Path, + *, + include_paths: Iterable | None = None, + path_resolution_wildcards: Mapping | None = None, + select_cfgs: Sequence[str] | None = None, +) -> Mapping[str, object]: + """Load a config file and return the data. + + The config file may contain references to other config files to import. + These are recursively loaded and merged with the initial config. The paths + to imported configs are provided in a list of strings as the value of the + optional `import_cfgs` key. + + Imported config paths provided may contain wildcards and may be either + relative or absolute paths. Relative paths are resolved from the provided + include paths in the order provided, the first path found to point to an + existing file is used as the path to the imported config. + + Wildcards may also be included in the path strings {token} as well as any + other string values in the configuration file. However only the wildcards + in the paths in import_cfgs are evaluated at this stage. The other fields + are left unchanged for downstream post processing in components that own + the information required to resolve the wildcards. + + Once the configs referenced in import_cfgs have been merged, the + import_cfgs key is removed. + + Args: + path: config file to parse + include_paths: iterable of paths to search for relative import config + files in. + path_resolution_wildcards: optional mapping of wildcard substitution + values. + select_cfgs: subset of configs to use. + + Returns: + combined mapping of key value pairs found in the config file and its + imported config files. + + """ + # Take an iterable but convert to list as an iterable can only be guaranteed + # to be consumable once. + include_paths = list(include_paths) if include_paths is not None else [] + + log.verbose("Loading config file '%s'", path) + + # Load in the top level config file as is + cfg_data = dict( + _load_cfg_file( + _resolve_cfg_path( + path, + include_paths=include_paths, + wildcard_values={}, + ), + ), + ) + + # Special wildcard self_dir points to the parent directory of the current + # config. This allows config relative paths. + cfg_data["self_dir"] = path.parent + + # config paths can be resolved with the provided wildcards, or values + # provided in the configuration itself. However the config values used in + # paths must be fully resolvable with constants in the config itself. + path_resolution_wildcards = ( + { + **path_resolution_wildcards, + **cfg_data, + } + if path_resolution_wildcards is not None + else {**cfg_data} + ) + + # Recurse the import_cfgs files and merge into the top cfg + cfg_data = _merge_import_cfgs( + top_cfg=cfg_data, + include_paths=include_paths, + wildcard_values=path_resolution_wildcards, + ) + + # Import any use_cfgs child config files + return _merge_use_cfgs( + top_cfg=cfg_data, + include_paths=include_paths, + wildcard_values=path_resolution_wildcards, + select_cfgs=select_cfgs, + ) diff --git a/src/dvsim/examples/testplanner/foo_dv_doc.md b/src/dvsim/examples/testplanner/foo_dv_doc.md index f2ea9e5e..61686ac4 100644 --- a/src/dvsim/examples/testplanner/foo_dv_doc.md +++ b/src/dvsim/examples/testplanner/foo_dv_doc.md @@ -2,6 +2,7 @@ # Copyright lowRISC contributors (OpenTitan project). # Licensed under the Apache License, Version 2.0, see LICENSE for details. # SPDX-License-Identifier: Apache-2.0 + --> # FOO DV document diff --git a/tests/config/__init__.py b/tests/config/__init__.py new file mode 100644 index 00000000..7f5722d0 --- /dev/null +++ b/tests/config/__init__.py @@ -0,0 +1,5 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Test configuration infrastructure.""" diff --git a/tests/config/example_cfgs/a.hjson b/tests/config/example_cfgs/a.hjson new file mode 100644 index 00000000..9a18e0aa --- /dev/null +++ b/tests/config/example_cfgs/a.hjson @@ -0,0 +1,4 @@ +// Copyright lowRISC contributors (OpenTitan project). +// Licensed under the Apache License, Version 2.0, see LICENSE for details. +// SPDX-License-Identifier: Apache-2.0 + diff --git a/tests/config/example_cfgs/a/circular.hjson b/tests/config/example_cfgs/a/circular.hjson new file mode 100644 index 00000000..ac426e88 --- /dev/null +++ b/tests/config/example_cfgs/a/circular.hjson @@ -0,0 +1,22 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +# Import config from other file +{ + import_cfgs: [ + "common.hjson", + "with_imports.hjson", + "a/circular.hjson", + ] + + # New config values + new: "value", + + tool: { + sim: { + name: "simulator", + cmd: "sim" + }, + }, +} diff --git a/tests/config/example_cfgs/b.hjson b/tests/config/example_cfgs/b.hjson new file mode 100644 index 00000000..9a18e0aa --- /dev/null +++ b/tests/config/example_cfgs/b.hjson @@ -0,0 +1,4 @@ +// Copyright lowRISC contributors (OpenTitan project). +// Licensed under the Apache License, Version 2.0, see LICENSE for details. +// SPDX-License-Identifier: Apache-2.0 + diff --git a/tests/config/example_cfgs/basic.hjson b/tests/config/example_cfgs/basic.hjson new file mode 100644 index 00000000..3fa694f5 --- /dev/null +++ b/tests/config/example_cfgs/basic.hjson @@ -0,0 +1,10 @@ +// Copyright lowRISC contributors (OpenTitan project). +// Licensed under the Apache License, Version 2.0, see LICENSE for details. +// SPDX-License-Identifier: Apache-2.0 + +{ + // comment + name: basic_config + a: b + b: 3 +} diff --git a/tests/config/example_cfgs/c.hjson b/tests/config/example_cfgs/c.hjson new file mode 100644 index 00000000..e88b24b0 --- /dev/null +++ b/tests/config/example_cfgs/c.hjson @@ -0,0 +1,4 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + diff --git a/tests/config/example_cfgs/child_cfgs/child_1.hjson b/tests/config/example_cfgs/child_cfgs/child_1.hjson new file mode 100644 index 00000000..17688ff3 --- /dev/null +++ b/tests/config/example_cfgs/child_cfgs/child_1.hjson @@ -0,0 +1,20 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +{ + name: "child_1", + + import_cfgs: ["common.hjson"], + + test_cases: { + fred: { + cmd: "test_runner", + args: ["a", "b", "c"], + }, + elaine: { + cmd: "test_runner", + args: ["g", "h", "i"], + } + } +} diff --git a/tests/config/example_cfgs/child_cfgs/child_2.hjson b/tests/config/example_cfgs/child_cfgs/child_2.hjson new file mode 100644 index 00000000..e26bdbb4 --- /dev/null +++ b/tests/config/example_cfgs/child_cfgs/child_2.hjson @@ -0,0 +1,16 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +{ + name: "child_2", + + import_cfgs: ["common.hjson"], + + test_cases: { + george: { + cmd: "test_runner", + args: ["d", "e", "f"], + }, + } +} diff --git a/tests/config/example_cfgs/common.hjson b/tests/config/example_cfgs/common.hjson new file mode 100644 index 00000000..4c064fd7 --- /dev/null +++ b/tests/config/example_cfgs/common.hjson @@ -0,0 +1,8 @@ +// Copyright lowRISC contributors (OpenTitan project). +// Licensed under the Apache License, Version 2.0, see LICENSE for details. +// SPDX-License-Identifier: Apache-2.0 + +{ + a: b + y: 3 +} diff --git a/tests/config/example_cfgs/config b/tests/config/example_cfgs/config new file mode 100644 index 00000000..e88b24b0 --- /dev/null +++ b/tests/config/example_cfgs/config @@ -0,0 +1,4 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + diff --git a/tests/config/example_cfgs/config.invalid b/tests/config/example_cfgs/config.invalid new file mode 100644 index 00000000..e88b24b0 --- /dev/null +++ b/tests/config/example_cfgs/config.invalid @@ -0,0 +1,4 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + diff --git a/tests/config/example_cfgs/empty.hjson b/tests/config/example_cfgs/empty.hjson new file mode 100644 index 00000000..9a18e0aa --- /dev/null +++ b/tests/config/example_cfgs/empty.hjson @@ -0,0 +1,4 @@ +// Copyright lowRISC contributors (OpenTitan project). +// Licensed under the Apache License, Version 2.0, see LICENSE for details. +// SPDX-License-Identifier: Apache-2.0 + diff --git a/tests/config/example_cfgs/import_cfg_not_list.hjson b/tests/config/example_cfgs/import_cfg_not_list.hjson new file mode 100644 index 00000000..01807abf --- /dev/null +++ b/tests/config/example_cfgs/import_cfg_not_list.hjson @@ -0,0 +1,8 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +# This field should be a list not a string +{ + "import_cfgs": "not a list", +} diff --git a/tests/config/example_cfgs/list_on_top.hjson b/tests/config/example_cfgs/list_on_top.hjson new file mode 100644 index 00000000..b93aae29 --- /dev/null +++ b/tests/config/example_cfgs/list_on_top.hjson @@ -0,0 +1,8 @@ +// Copyright lowRISC contributors (OpenTitan project). +// Licensed under the Apache License, Version 2.0, see LICENSE for details. +// SPDX-License-Identifier: Apache-2.0 + +[ + {"name": "first"}, + {"name": "second"}, +] diff --git a/tests/config/example_cfgs/parent.hjson b/tests/config/example_cfgs/parent.hjson new file mode 100644 index 00000000..c2f16614 --- /dev/null +++ b/tests/config/example_cfgs/parent.hjson @@ -0,0 +1,16 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +{ + name: "parent", + + import_cfgs: [ + "values.hjson", + ], + + use_cfgs: [ + "child_1.hjson", + "child_2.hjson", + ], +} diff --git a/tests/config/example_cfgs/sub_root/other.hjson b/tests/config/example_cfgs/sub_root/other.hjson new file mode 100644 index 00000000..df807d1a --- /dev/null +++ b/tests/config/example_cfgs/sub_root/other.hjson @@ -0,0 +1,8 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +{ +a: "10" +b: "test" +} diff --git a/tests/config/example_cfgs/values.hjson b/tests/config/example_cfgs/values.hjson new file mode 100644 index 00000000..65f9fbf1 --- /dev/null +++ b/tests/config/example_cfgs/values.hjson @@ -0,0 +1,8 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +{ + d: [1, 2, 3], + e: "test", +} diff --git a/tests/config/example_cfgs/with_imports.hjson b/tests/config/example_cfgs/with_imports.hjson new file mode 100644 index 00000000..9eb35e4f --- /dev/null +++ b/tests/config/example_cfgs/with_imports.hjson @@ -0,0 +1,11 @@ +// Copyright lowRISC contributors (OpenTitan project). +// Licensed under the Apache License, Version 2.0, see LICENSE for details. +// SPDX-License-Identifier: Apache-2.0 + +{ + name: with_imports + import_cfgs: [ + "common.hjson", + "values.hjson", + ] +} diff --git a/tests/config/test_load.py b/tests/config/test_load.py new file mode 100644 index 00000000..6dafa6f5 --- /dev/null +++ b/tests/config/test_load.py @@ -0,0 +1,456 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Test config loading.""" + +from collections.abc import Mapping, Sequence +from pathlib import Path + +import pytest +from hamcrest import assert_that, calling, equal_to, raises + +from dvsim.config.load import ( + ConflictingConfigValueError, + _extract_config_paths, + _merge_cfg_map, + _resolve_cfg_path, + load_cfg, +) + +CFG_BASE = Path(__file__).parent / "example_cfgs" + + +class TestConfigLoader: + """Test load_cfg.""" + + @staticmethod + @pytest.mark.parametrize( + ("cfg_path", "exception", "match"), + [ + # file doesn't exist + (CFG_BASE / "file_does_not_exist", FileNotFoundError, ""), + # existing file with no extension + (CFG_BASE / "config", ValueError, ""), + # existing file with invalid extension + (CFG_BASE / "config.invalid", TypeError, ""), + # empty config file + (CFG_BASE / "empty.hjson", ValueError, ""), + # top level config container is list not dict + (CFG_BASE / "list_on_top.hjson", TypeError, ""), + (CFG_BASE / "import_cfg_not_list.hjson", TypeError, ".*not a list"), + ], + ) + def test_invalid_cfg_raises( + cfg_path: Path, + exception: type[Exception], + match: str, + ) -> None: + """Test invalid config raises.""" + assert_that( + calling(load_cfg).with_args( + path=cfg_path, + include_paths=[CFG_BASE], + ), + raises(exception, match), + ) + + @staticmethod + @pytest.mark.parametrize( + ("cfg_path", "expected"), + [ + ( + Path("basic.hjson"), + { + "name": "basic_config", + "a": "b", + "b": 3, + }, + ), + # With import_cfgs to pull in values from other config files + ( + Path("with_imports.hjson"), + { + "name": "with_imports", + "a": "b", + "y": 3, + "d": [1, 2, 3], + "e": "test", + }, + ), + ( + Path("a") / "circular.hjson", + { + "new": "value", + "tool": {"sim": {"name": "simulator", "cmd": "sim"}}, + "name": "with_imports", + "d": [1, 2, 3], + "e": "test", + "a": "b", + "y": 3, + }, + ), + ], + ) + def test_load(cfg_path: Path, expected: Mapping) -> None: + """Test config can be loaded.""" + exp_cfg = dict(expected) + exp_cfg["self_dir"] = (CFG_BASE / cfg_path).parent + + assert_that( + load_cfg(path=CFG_BASE / cfg_path, include_paths=[CFG_BASE]), + equal_to(exp_cfg), + ) + + @staticmethod + @pytest.mark.parametrize( + ("cfg_path", "selected", "expected"), + [ + ( + # No selection means load all use configs + Path("parent.hjson"), + [], + { + "name": "parent", + "d": [1, 2, 3], + "e": "test", + "cfgs": { + CFG_BASE / "child_cfgs" / "child_1.hjson": { + "name": "child_1", + "test_cases": { + "fred": { + "cmd": "test_runner", + "args": ["a", "b", "c"], + }, + "elaine": { + "cmd": "test_runner", + "args": ["g", "h", "i"], + }, + }, + "a": "b", + "y": 3, + }, + CFG_BASE / "child_cfgs" / "child_2.hjson": { + "name": "child_2", + "test_cases": { + "george": { + "cmd": "test_runner", + "args": ["d", "e", "f"], + }, + }, + "a": "b", + "y": 3, + }, + }, + }, + ), + # TODO: move to an inline config + # Select all configs explicitly + ( + Path("parent.hjson"), + ["child_1", "child_2"], + { + "name": "parent", + "d": [1, 2, 3], + "e": "test", + "cfgs": { + CFG_BASE / "child_cfgs" / "child_1.hjson": { + "name": "child_1", + "test_cases": { + "fred": { + "cmd": "test_runner", + "args": ["a", "b", "c"], + }, + "elaine": { + "cmd": "test_runner", + "args": ["g", "h", "i"], + }, + }, + "a": "b", + "y": 3, + }, + CFG_BASE / "child_cfgs" / "child_2.hjson": { + "name": "child_2", + "test_cases": { + "george": { + "cmd": "test_runner", + "args": ["d", "e", "f"], + }, + }, + "a": "b", + "y": 3, + }, + }, + }, + ), + # Only select the first config + ( + Path("parent.hjson"), + ["child_1"], + { + "name": "parent", + "d": [1, 2, 3], + "e": "test", + "cfgs": { + CFG_BASE / "child_cfgs" / "child_1.hjson": { + "name": "child_1", + "test_cases": { + "fred": { + "cmd": "test_runner", + "args": ["a", "b", "c"], + }, + "elaine": { + "cmd": "test_runner", + "args": ["g", "h", "i"], + }, + }, + "a": "b", + "y": 3, + }, + }, + }, + ), + # Only select the second config + ( + Path("parent.hjson"), + ["child_2"], + { + "name": "parent", + "d": [1, 2, 3], + "e": "test", + "cfgs": { + CFG_BASE / "child_cfgs" / "child_2.hjson": { + "name": "child_2", + "test_cases": { + "george": { + "cmd": "test_runner", + "args": ["d", "e", "f"], + }, + }, + "a": "b", + "y": 3, + }, + }, + }, + ), + ], + ) + def test_load_child_use_cfgs( + cfg_path: Path, + selected: Sequence[str] | None, + expected: Mapping, + ) -> None: + """Test child configuration files are loaded.""" + cfg = load_cfg( + path=CFG_BASE / cfg_path, + include_paths=[CFG_BASE, CFG_BASE / "child_cfgs"], + select_cfgs=selected, + ) + + # Check the selected config names + assert_that( + {c["name"] for c in cfg["cfgs"].values()}, + equal_to({c["name"] for c in expected["cfgs"].values()}), + ) + + # Check the contents of the child configs match expected + for path in cfg["cfgs"]: + exp_cfg = expected["cfgs"][path] + exp_cfg["self_dir"] = path.parent + assert_that(cfg["cfgs"][path], equal_to(exp_cfg)) + + # Double check the entire config matches + exp_cfg = dict(expected) + exp_cfg["self_dir"] = (CFG_BASE / cfg_path).parent + + assert_that(cfg, equal_to(exp_cfg)) + + +class TestResolveCfgPath: + """Test _resolve_cfg_path.""" + + @staticmethod + @pytest.mark.parametrize( + ("path", "exception"), + [ + (CFG_BASE / "file_not_exist.hjson", FileNotFoundError), + (Path("file_not_exist.hjson"), ValueError), + ("file_not_exist.hjson", ValueError), + (Path("/absolute/path/that/is/missing.hjson"), FileNotFoundError), + ("/absolute/path/that/is/missing.hjson", FileNotFoundError), + ("/path/with/unresolvable_{wildcards}.hjson", KeyError), + ], + ) + def test_resolve_cfg_path_invalid_path( + path: str | Path, + exception: type[Exception], + ) -> None: + """Test that invalid paths raise.""" + assert_that( + calling(_resolve_cfg_path).with_args( + path, + include_paths=(CFG_BASE,), + wildcard_values={}, + ), + raises(exception), + ) + + @staticmethod + @pytest.mark.parametrize( + ("path", "resolved_path", "wildcard_values"), + [ + # Full path + (CFG_BASE / "basic.hjson", CFG_BASE / "basic.hjson", {}), + (str(CFG_BASE / "basic.hjson"), CFG_BASE / "basic.hjson", {}), + ( + "{cfg_base}/basic.hjson", + CFG_BASE / "basic.hjson", + {"cfg_base": CFG_BASE}, + ), + # Sting path with no wildcards + (Path("basic.hjson"), CFG_BASE / "basic.hjson", {}), + ("basic.hjson", CFG_BASE / "basic.hjson", {}), + ("bas{end}.hjson", CFG_BASE / "basic.hjson", {"end": "ic"}), + ( + "bas{end}.hj{inner}n", + CFG_BASE / "basic.hjson", + {"end": "ic", "inner": "so"}, + ), + # sub directory + ( + Path("sub_root") / "other.hjson", + CFG_BASE / "sub_root" / "other.hjson", + {}, + ), + ("sub_root/other.hjson", CFG_BASE / "sub_root" / "other.hjson", {}), + # relative to additional include path CFG_BASE/sub_root + (Path("other.hjson"), CFG_BASE / "sub_root" / "other.hjson", {}), + ("other.hjson", CFG_BASE / "sub_root" / "other.hjson", {}), + ], + ) + def test_resolve_cfg_path( + path: str | Path, + resolved_path: Path, + wildcard_values: dict[str, object], + ) -> None: + """Test that valid paths can be resolved.""" + assert_that( + _resolve_cfg_path( + path, + include_paths=(CFG_BASE, CFG_BASE / "sub_root"), + wildcard_values=wildcard_values, + ), + equal_to(resolved_path), + ) + + @staticmethod + def test_extract_config_import_paths() -> None: + """Test list of import cfgs is retreaved and then filtered from cfg.""" + assert_that( + _extract_config_paths( + data={ + "import_cfgs": ["a.hjson", "b.hjson", "c.hjson"], + "a": 1, + "b": 2, + }, + field="import_cfgs", + include_paths=[CFG_BASE], + wildcard_values={}, + ), + equal_to( + [ + CFG_BASE / "a.hjson", + CFG_BASE / "b.hjson", + CFG_BASE / "c.hjson", + ], + ), + ) + + +class TestMergeCfgValues: + """Test _merge_cfg_values.""" + + @staticmethod + @pytest.mark.parametrize( + ("obj", "other", "expected"), + [ + # Empty configuration + ({}, {}, {}), + # int + ({"a": 1}, {}, {"a": 1}), # Left single value + ({}, {"a": 1}, {"a": 1}), # Right single value + ({"a": 1}, {"a": 1}, {"a": 1}), # Same values + ({"b": 1}, {"a": 2}, {"a": 2, "b": 1}), + ({"top": {"b": 1}}, {"top": {"a": 2}}, {"top": {"a": 2, "b": 1}}), + # float + ({"a": 1.5}, {}, {"a": 1.5}), + ({}, {"a": 1.5}, {"a": 1.5}), + ({"a": 1.5}, {"a": 1.5}, {"a": 1.5}), + ( + {"top": {"b": 1.5}}, + {"top": {"a": 2.4}}, + {"top": {"a": 2.4, "b": 1.5}}, + ), + # str + ({"a": "v"}, {}, {"a": "v"}), + ({}, {"a": "v"}, {"a": "v"}), + ({"a": "v"}, {"a": "v"}, {"a": "v"}), + ({"a": "v"}, {"a": ""}, {"a": "v"}), # empty string ignored + ({"a": ""}, {"a": "v"}, {"a": "v"}), # empty string ignored + ( + {"top": {"b": "v1"}}, + {"top": {"a": "v2"}}, + {"top": {"a": "v2", "b": "v1"}}, + ), + # Lists + ({"a": []}, {}, {"a": []}), + ({"a": [1]}, {}, {"a": [1]}), + ({"a": [1]}, {"a": [2]}, {"a": [1, 2]}), + ({}, {"a": []}, {"a": []}), + ({}, {"a": [1]}, {"a": [1]}), + # three levels merge + ( + {"top": {"mid": {"b": 1}}}, + {"top": {"mid": {"a": 2}}}, + {"top": {"mid": {"a": 2, "b": 1}}}, + ), + ( + {"top": {"mid": {"b": [1, 2]}}}, + {"top": {"mid": {"b": [3]}}}, + {"top": {"mid": {"b": [1, 2, 3]}}}, + ), + ], + ) + def test_merge_cfg_maps_valid_with_no_conflicts( + obj: Mapping, + other: Mapping, + expected: Mapping, + ) -> None: + """Test two configs are merged when no conflicts.""" + assert_that( + _merge_cfg_map(obj=obj, other=other), + equal_to(expected), + ) + + @staticmethod + @pytest.mark.parametrize( + ("obj", "other"), + [ + # top level conflicts + ({"a": 1}, {"a": 2}), + ({"a": "abc"}, {"a": "efg"}), + # second level conflicts + ({"top": {"a": 1}}, {"top": {"a": 2}}), + # third level conflicts + ({"top": {"mid": {"a": 1}}}, {"top": {"mid": {"a": 2}}}), + ], + ) + def test_conflicting_values_raises( + obj: Mapping, + other: Mapping, + ) -> None: + """Test config maps with conflicting values raises.""" + assert_that( + calling(_merge_cfg_map).with_args(obj=obj, other=other), + raises(ConflictingConfigValueError), + ) From d81f21436081af25e7d87f32f004147617fd048e Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Wed, 26 Mar 2025 10:10:25 +0000 Subject: [PATCH 06/13] refactor: [config] update cli and flow config classes to use the new config loader. Signed-off-by: James McCorrie --- src/dvsim/cli.py | 206 +++-------------------------- src/dvsim/flow/base.py | 259 ++++++------------------------------- src/dvsim/flow/cdc.py | 22 +++- src/dvsim/flow/factory.py | 181 +++++++++++++------------- src/dvsim/flow/formal.py | 28 +++- src/dvsim/flow/hjson.py | 190 --------------------------- src/dvsim/flow/lint.py | 22 +++- src/dvsim/flow/one_shot.py | 28 +++- src/dvsim/flow/rdc.py | 22 +++- src/dvsim/flow/sim.py | 24 +++- src/dvsim/flow/syn.py | 21 ++- 11 files changed, 290 insertions(+), 713 deletions(-) delete mode 100644 src/dvsim/flow/hjson.py diff --git a/src/dvsim/cli.py b/src/dvsim/cli.py index 81ff4953..cb57f13d 100644 --- a/src/dvsim/cli.py +++ b/src/dvsim/cli.py @@ -22,10 +22,9 @@ import argparse import datetime +import logging as log import os import random -import shlex -import subprocess import sys import textwrap from pathlib import Path @@ -39,8 +38,9 @@ from dvsim.launcher.nc import NcLauncher from dvsim.launcher.sge import SgeLauncher from dvsim.launcher.slurm import SlurmLauncher -from dvsim.logging import configure_logging, log -from dvsim.utils import TS_FORMAT, TS_FORMAT_LONG, Timer, rm_path, run_cmd_with_timeout +from dvsim.logging import configure_logging +from dvsim.project import init_project +from dvsim.utils import TS_FORMAT, TS_FORMAT_LONG, Timer # TODO: add dvsim_cfg.hjson to retrieve this info version = 0.1 @@ -49,50 +49,6 @@ _LIST_CATEGORIES = ["build_modes", "run_modes", "tests", "regressions"] -# Function to resolve the scratch root directory among the available options: -# If set on the command line, then use that as a preference. -# Else, check if $SCRATCH_ROOT env variable exists and is a directory. -# Else use the default (/scratch) -# Try to create the directory if it does not already exist. -def resolve_scratch_root(arg_scratch_root, proj_root): - default_scratch_root = proj_root + "/scratch" - scratch_root = os.environ.get("SCRATCH_ROOT") - if not arg_scratch_root: - if scratch_root is None: - arg_scratch_root = default_scratch_root - else: - # Scratch space could be mounted in a filesystem (such as NFS) on a network drive. - # If the network is down, it could cause the access access check to hang. So run a - # simple ls command with a timeout to prevent the hang. - (out, status) = run_cmd_with_timeout( - cmd="ls -d " + scratch_root, - timeout=1, - exit_on_failure=0, - ) - if status == 0 and out != "": - arg_scratch_root = scratch_root - else: - arg_scratch_root = default_scratch_root - log.warning( - f'Env variable $SCRATCH_ROOT="{scratch_root}" is not accessible.\n' - f'Using "{arg_scratch_root}" instead.', - ) - else: - arg_scratch_root = os.path.realpath(arg_scratch_root) - - try: - os.makedirs(arg_scratch_root, exist_ok=True) - except PermissionError as e: - log.fatal(f"Failed to create scratch root {arg_scratch_root}:\n{e}.") - sys.exit(1) - - if not os.access(arg_scratch_root, os.W_OK): - log.fatal(f"Scratch root {arg_scratch_root} is not writable!") - sys.exit(1) - - return arg_scratch_root - - def read_max_parallel(arg): """Take value for --max-parallel as an integer.""" try: @@ -129,124 +85,6 @@ def resolve_max_parallel(arg): return 16 -def resolve_branch(branch): - """Choose a branch name for output files. - - If the --branch argument was passed on the command line, the branch - argument is the branch name to use. Otherwise it is None and we use git to - find the name of the current branch in the working directory. - - Note, as this name will be used to generate output files any forward slashes - are replaced with single dashes to avoid being interpreted as directory hierarchy. - """ - if branch is not None: - return branch.replace("/", "-") - - result = subprocess.run( - ["git", "rev-parse", "--abbrev-ref", "HEAD"], - stdout=subprocess.PIPE, - check=False, - ) - branch = result.stdout.decode("utf-8").strip().replace("/", "-") - if not branch: - log.warning('Failed to find current git branch. Setting it to "default"') - branch = "default" - - return branch - - -# Get the project root directory path - this is used to construct the full paths -def get_proj_root(): - cmd = ["git", "rev-parse", "--show-toplevel"] - result = subprocess.run(cmd, capture_output=True, check=False) - proj_root = result.stdout.decode("utf-8").strip() - if not proj_root: - log.error( - "Attempted to find the root of this GitHub repository by running:\n" - "{}\n" - "But this command has failed:\n" - "{}".format(" ".join(cmd), result.stderr.decode("utf-8")), - ) - sys.exit(1) - return proj_root - - -def resolve_proj_root(args): - """Update proj_root based on how DVSim is invoked. - - If --remote switch is set, a location in the scratch area is chosen as the - new proj_root. The entire repo is copied over to this location. Else, the - proj_root is discovered using get_proj_root() method, unless the user - overrides it on the command line. - - This function returns the updated proj_root src and destination path. If - --remote switch is not set, the destination path is identical to the src - path. Likewise, if --dry-run is set. - """ - proj_root_src = args.proj_root or get_proj_root() - - # Check if jobs are dispatched to external compute machines. If yes, - # then the repo needs to be copied over to the scratch area - # accessible to those machines. - # If --purge arg is set, then purge the repo_top that was copied before. - if args.remote and not args.dry_run: - proj_root_dest = os.path.join(args.scratch_root, args.branch, "repo_top") - if args.purge: - rm_path(proj_root_dest) - copy_repo(proj_root_src, proj_root_dest) - else: - proj_root_dest = proj_root_src - - return proj_root_src, proj_root_dest - - -def copy_repo(src, dest) -> None: - """Copy over the repo to a new location. - - The repo is copied over from src to dest area. It tentatively uses the - rsync utility which provides the ability to specify a file containing some - exclude patterns to skip certain things from being copied over. With GitHub - repos, an existing `.gitignore` serves this purpose pretty well. - """ - rsync_cmd = [ - "rsync", - "--recursive", - "--links", - "--checksum", - "--update", - "--inplace", - "--no-group", - ] - - # Supply `.gitignore` from the src area to skip temp files. - ignore_patterns_file = Path(src) / ".gitignore" - if ignore_patterns_file.exists(): - # TODO: hack - include hw/foundry since it is excluded in .gitignore. - rsync_cmd += [ - "--include=hw/foundry", - f"--exclude-from={ignore_patterns_file}", - "--exclude=.*", - ] - - rsync_cmd += [src + "/.", dest] - rsync_str = " ".join([shlex.quote(w) for w in rsync_cmd]) - - cmd = ["flock", "--timeout", "600", dest, "--command", rsync_str] - - log.info("[copy_repo] [dest]: %s", dest) - log.verbose("[copy_repo] [cmd]: \n%s", " ".join(cmd)) - - # Make sure the dest exists first. - os.makedirs(dest, exist_ok=True) - try: - subprocess.run(cmd, check=True, capture_output=True) - except subprocess.CalledProcessError as e: - log.exception( - "Failed to copy over %s to %s: %s", src, dest, e.stderr.decode("utf-8").strip() - ) - log.info("Done.") - - def wrapped_docstring(): """Return a text-wrapped version of the module docstring.""" paras = [] @@ -823,27 +661,18 @@ def main() -> None: debug=args.verbose == "debug", ) - if not Path(args.cfg).exists(): - log.fatal("Path to config file %s appears to be invalid.", args.cfg) - sys.exit(1) - - args.branch = resolve_branch(args.branch) - proj_root_src, proj_root = resolve_proj_root(args) - args.scratch_root = resolve_scratch_root(args.scratch_root, proj_root) - log.info("[proj_root]: %s", proj_root) - - # Create an empty FUSESOC_IGNORE file in scratch_root. This ensures that - # any fusesoc invocation from a job won't search within scratch_root for - # core files. - (Path(args.scratch_root) / "FUSESOC_IGNORE").touch() - - args.cfg = Path(args.cfg).resolve() - if args.remote: - cfg_path = args.cfg.replace(proj_root_src + "/", "") - args.cfg = os.path.join(proj_root, cfg_path) + project_cfg = init_project( + cfg_path=Path(args.cfg), + proj_root=Path(args.proj_root) if args.proj_root else None, + scratch_root=Path(args.scratch_root) if args.scratch_root else None, + branch=args.branch, + dry_run=args.dry_run, + remote=args.remote, + job_prefix=args.job_prefix, + ) # Add timestamp to args that all downstream objects can use. - curr_ts = datetime.datetime.utcnow() + curr_ts = datetime.datetime.now(datetime.UTC) args.timestamp_long = curr_ts.strftime(TS_FORMAT_LONG) args.timestamp = curr_ts.strftime(TS_FORMAT) @@ -867,8 +696,11 @@ def main() -> None: # Build infrastructure from hjson file and create the list of items to # be deployed. - global cfg - cfg = make_cfg(args.cfg, args, proj_root) + cfg = make_cfg( + project_cfg=project_cfg, + select_cfgs=args.select_cfgs, + args=args, + ) # List items available for run if --list switch is passed, and exit. if args.list is not None: diff --git a/src/dvsim/flow/base.py b/src/dvsim/flow/base.py index a54bed3d..2d21a336 100644 --- a/src/dvsim/flow/base.py +++ b/src/dvsim/flow/base.py @@ -8,35 +8,27 @@ import os import pprint import sys -from abc import ABC, abstractmethod +from abc import abstractmethod +from argparse import Namespace from collections.abc import Mapping, Sequence from pathlib import Path from typing import ClassVar -import hjson - -from dvsim.flow.hjson import set_target_attribute from dvsim.job.deploy import Deploy from dvsim.launcher.factory import get_launcher_cls from dvsim.logging import log +from dvsim.project import ProjectMeta from dvsim.scheduler import Scheduler from dvsim.utils import ( find_and_substitute_wildcards, md_results_to_html, mk_path, - rm_path, - subst_wildcards, ) # Interface class for extensions. -class FlowCfg(ABC): - """Base class for the different flows supported by dvsim.py. - - The constructor expects some parsed hjson data. Create these objects with - the factory function in CfgFactory.py, which loads the hjson data and picks - a subclass of FlowCfg based on its contents. - """ +class FlowCfg: + """Base class for the different flows supported by DVSim.""" # Set in subclasses. This is the key that must be used in an hjson file to # tell dvsim.py which subclass to use. @@ -50,7 +42,24 @@ def __str__(self) -> str: """Get string representation of the flow config.""" return pprint.pformat(self.__dict__) - def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: + def __init__( + self, + *, + flow_cfg_file: Path, + project_cfg: ProjectMeta, + config_data: Mapping, + args: Namespace, + child_configs: Sequence["FlowCfg"] | None = None, + ) -> None: + """Initialise the flow.""" + self._project_cfg = project_cfg + self._config_data = dict(config_data) + if "cfgs" in self._config_data: + del self._config_data["cfgs"] + + # Take the configs as provided + self.cfgs: Sequence[FlowCfg] = child_configs or [] + # Options set from command line # Uniquify input items, while preserving the order. self.items = list(dict.fromkeys(args.items)) @@ -58,9 +67,6 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: self.select_cfgs = args.select_cfgs self.flow_cfg_file = flow_cfg_file self.args = args - self.scratch_root = args.scratch_root - self.branch = args.branch - self.job_prefix = args.job_prefix self.gui = args.gui self.gui_debug = args.gui_debug if self.gui_debug: @@ -70,8 +76,6 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: # Options set from hjson cfg. self.project = "" - self.scratch_path = "" - self.scratch_base_path = "" # Add exports using 'exports' keyword - these are exported to the child # process' environment. @@ -81,12 +85,9 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: # are overridden with the override values. self.overrides = [] - # List of cfgs if the parsed cfg is a primary cfg list - self.cfgs = [] - # Add a notion of "primary" cfg - this is indicated using # a special key 'use_cfgs' within the hjson cfg. - self.is_primary_cfg = False + self.is_primary_cfg = child_configs is not None # For a primary cfg, it is the aggregated list of all deploy objects # under self.cfgs. For a non-primary cfg, it is the list of items @@ -117,54 +118,25 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: # Summary results, generated by over-arching cfg self.results_summary_md = "" - # Merge in the values from the loaded hjson file. If subclasses want to - # add other default parameters that depend on the parameters above, - # they can override _merge_hjson and add their parameters at the start - # of that. - self._merge_hjson(hjson_data) + # These are for temporary backward compatibility and wildcard expansion + self.proj_root = self._project_cfg.root_path + self.scratch_root = self._project_cfg.scratch_path + self.branch = self._project_cfg.branch + self.job_prefix = self._project_cfg.job_prefix + + # Merge in the values from the loaded config file. + self.__dict__.update(self._config_data) # Is this a primary config? If so, we need to load up all the child # configurations at this point. If not, we place ourselves into # self.cfgs and consider ourselves a sort of "degenerate primary # configuration". - self.is_primary_cfg = "use_cfgs" in hjson_data - - if not self.is_primary_cfg: - self.cfgs.append(self) - else: - for entry in self.use_cfgs: - self._load_child_cfg(entry, mk_config) if self.rel_path == "": - self.rel_path = str( - Path(self.flow_cfg_file).parent.relative_to(self.proj_root), + self.rel_path = self.flow_cfg_file.parent.relative_to( + self._project_cfg.root_path, ) - # Process overrides before substituting wildcards - self._process_overrides() - - # Expand wildcards. If subclasses need to mess around with parameters - # after merging the hjson but before expansion, they can override - # _expand and add the code at the start. - self._expand() - - # Construct the path variables after variable expansion. - self.results_dir = Path(self.scratch_base_path) / "reports" / self.rel_path - self.results_page = self.results_dir / self.results_html_name - - # Run any final checks - self._post_init() - - def _merge_hjson(self, hjson_data: Mapping) -> None: - """Take hjson data and merge it into self.__dict__. - - Subclasses that need to do something just before the merge should - override this method and call super()._merge_hjson(..) at the end. - - """ - for key, value in hjson_data.items(): - set_target_attribute(self.flow_cfg_file, self.__dict__, key, value) - def _expand(self) -> None: """Expand wildcards after merging hjson. @@ -180,166 +152,12 @@ def _expand(self) -> None: self.run_script = "{proj_root}/" + self.args.dump_script self.__dict__ = find_and_substitute_wildcards( - self.__dict__, - self.__dict__, - self.ignored_wildcards, + obj=self.__dict__, + wildcard_values=self.__dict__, + ignored_wildcards=self.ignored_wildcards, ignore_error=partial, ) - def _post_init(self) -> None: - # Run some post init checks - if not self.is_primary_cfg: # noqa: SIM102 - # Check if self.cfgs is a list of exactly 1 item (self) - if not (len(self.cfgs) == 1 and self.cfgs[0].name == self.name): - log.error("Parse error!\n%s", self.cfgs) - sys.exit(1) - - def create_instance(self, mk_config, flow_cfg_file): - """Create a new instance of this class for the given config file. - - mk_config is a factory method (passed explicitly to avoid a circular - dependency between this file and CfgFactory.py). - - """ - new_instance = mk_config(flow_cfg_file) - - # Sanity check to make sure the new object is the same class as us: we - # don't yet support heterogeneous primary configurations. - if type(self) is not type(new_instance): - log.error( - f"{self.flow_cfg_file}: Loading child configuration at {flow_cfg_file!r}, but the " - f"resulting flow types don't match: ({type(self).__name__} vs. {type(new_instance).__name__}).", - ) - sys.exit(1) - - return new_instance - - def _load_child_cfg(self, entry, mk_config) -> None: - """Load a child configuration for a primary cfg.""" - if type(entry) is str: - # Treat this as a file entry. Substitute wildcards in cfg_file - # files since we need to process them right away. - cfg_file = subst_wildcards(entry, self.__dict__, ignore_error=True) - self.cfgs.append(self.create_instance(mk_config, cfg_file)) - - elif type(entry) is dict: - # Treat this as a cfg expanded in-line - temp_cfg_file = self._conv_inline_cfg_to_hjson(entry) - if not temp_cfg_file: - return - self.cfgs.append(self.create_instance(mk_config, temp_cfg_file)) - - # Delete the temp_cfg_file once the instance is created - log.verbose("Deleting temp cfg file:\n%s", temp_cfg_file) - rm_path(temp_cfg_file, ignore_error=True) - - else: - log.error( - 'Type of entry "%s" in the "use_cfgs" key is invalid: %s', - entry, - str(type(entry)), - ) - sys.exit(1) - - def _conv_inline_cfg_to_hjson(self, idict: Mapping) -> str | None: - """Dump a temp hjson file in the scratch space from input dict. - - This method is to be called only by a primary cfg. - """ - if not self.is_primary_cfg: - log.fatal("This method can only be called by a primary cfg") - sys.exit(1) - - name = idict.get("name", None) - if not name: - log.error( - "In-line entry in use_cfgs list does not contain a " - '"name" key (will be skipped!):\n%s', - idict, - ) - return None - - # Check if temp cfg file already exists - temp_cfg_file = self.scratch_root + "/." + self.branch + "__" + name + "_cfg.hjson" - - # Create the file and dump the dict as hjson - log.verbose('Dumping inline cfg "%s" in hjson to:\n%s', name, temp_cfg_file) - - try: - Path(temp_cfg_file).write_text(hjson.dumps(idict, for_json=True)) - - except Exception as e: - log.exception( - 'Failed to hjson-dump temp cfg file"%s" for "%s"(will be skipped!) due to:\n%s', - temp_cfg_file, - name, - e, - ) - return None - - # Return the temp cfg file created - return temp_cfg_file - - def _process_overrides(self) -> None: - # Look through the dict and find available overrides. - # If override is available, check if the type of the value for existing - # and the overridden keys are the same. - overrides_dict = {} - if hasattr(self, "overrides"): - overrides = self.overrides - if type(overrides) is not list: - log.error( - 'The type of key "overrides" is %s - it should be a list', - type(overrides), - ) - sys.exit(1) - - # Process override one by one - for item in overrides: - if type(item) is dict and set(item.keys()) == {"name", "value"}: - ov_name = item["name"] - ov_value = item["value"] - if ov_name not in overrides_dict: - overrides_dict[ov_name] = ov_value - self._do_override(ov_name, ov_value) - else: - log.error( - 'Override for key "%s" already exists!\nOld: %s\nNew: %s', - ov_name, - overrides_dict[ov_name], - ov_value, - ) - sys.exit(1) - else: - log.error( - '"overrides" is a list of dicts with ' - '{"name": , "value": } pairs. ' - "Found this instead:\n%s", - str(item), - ) - sys.exit(1) - - def _do_override(self, ov_name: str, ov_value: object) -> None: - # Go through self attributes and replace with overrides - if hasattr(self, ov_name): - orig_value = getattr(self, ov_name) - if isinstance(ov_value, type(orig_value)): - log.debug('Overriding "%s" value "%s" with "%s"', ov_name, orig_value, ov_value) - setattr(self, ov_name, ov_value) - else: - log.error( - 'The type of override value "%s" for "%s" ' - 'doesn\'t match the type of original value "%s"', - ov_value, - ov_name, - orig_value, - ) - sys.exit(1) - else: - log.error('Override key "%s" not found in the cfg!', ov_name) - sys.exit(1) - - @abstractmethod def _purge(self) -> None: """Purge the existing scratch areas in preparation for the new run.""" @@ -359,9 +177,6 @@ def print_list(self) -> None: def prune_selected_cfgs(self) -> None: """Prune the list of configs for a primary config file.""" - # This should run after self.cfgs has been set - assert self.cfgs - # If the user didn't pass --select-cfgs, we don't do anything. if self.select_cfgs is None: return diff --git a/src/dvsim/flow/cdc.py b/src/dvsim/flow/cdc.py index 2a3bea7c..ba846172 100644 --- a/src/dvsim/flow/cdc.py +++ b/src/dvsim/flow/cdc.py @@ -4,7 +4,12 @@ """Class describing lint configuration object.""" +from argparse import Namespace +from collections.abc import Mapping, Sequence +from pathlib import Path + from dvsim.flow.lint import LintCfg +from dvsim.project import ProjectMeta class CdcCfg(LintCfg): @@ -12,7 +17,20 @@ class CdcCfg(LintCfg): flow = "cdc" - def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: - super().__init__(flow_cfg_file, hjson_data, args, mk_config) + def __init__( + self, + flow_cfg_file: Path, + project_cfg: ProjectMeta, + config_data: Mapping, + args: Namespace, + child_configs: Sequence["CdcCfg"] | None = None, + ) -> None: + super().__init__( + flow_cfg_file=flow_cfg_file, + project_cfg=project_cfg, + config_data=config_data, + args=args, + child_configs=child_configs, + ) self.results_title = f"{self.name.upper()} CDC Results" diff --git a/src/dvsim/flow/factory.py b/src/dvsim/flow/factory.py index 844462a4..d9d5a741 100644 --- a/src/dvsim/flow/factory.py +++ b/src/dvsim/flow/factory.py @@ -2,17 +2,21 @@ # Licensed under the Apache License, Version 2.0, see LICENSE for details. # SPDX-License-Identifier: Apache-2.0 -import pathlib -import sys +"""Factory to generate a flow config.""" +from argparse import Namespace +from collections.abc import Sequence + +from dvsim.config.load import load_cfg +from dvsim.flow.base import FlowCfg from dvsim.flow.cdc import CdcCfg from dvsim.flow.formal import FormalCfg -from dvsim.flow.hjson import load_hjson from dvsim.flow.lint import LintCfg from dvsim.flow.rdc import RdcCfg from dvsim.flow.sim import SimCfg from dvsim.flow.syn import SynCfg from dvsim.logging import log +from dvsim.project import ProjectMeta FLOW_HANDLERS = { "cdc": CdcCfg, @@ -24,110 +28,105 @@ } -def _load_cfg(path, initial_values): - """Worker function for make_cfg. +def _get_flow_handler_cls(flow: str) -> type[FlowCfg]: + """Get a flow handler class for the given flow name. - initial_values is passed to load_hjson (see documentation there). + Args: + flow: name of the flow - Returns a pair (cls, hjson_data) on success or raises a RuntimeError on - failure. + Returns: + Class object that can be used to instantiate a flow handler """ - # Set the `self_dir` template variable to the path of the currently - # processed Hjson file. - assert "self_dir" in initial_values - initial_values["self_dir"] = pathlib.Path(path).parent - - # Start by loading up the hjson file and any included files - hjson_data = load_hjson(path, initial_values) - - # Look up the value of flow in the loaded data. This is a required field, - # and tells us what sort of FlowCfg to make. - flow = hjson_data.get("flow") - if flow is None: + if flow not in FLOW_HANDLERS: + known_flows = ", ".join(FLOW_HANDLERS.keys()) msg = ( - f'{path!r}: No value for the "flow" key. Are you sure ' - "this is a dvsim configuration file?" + f'Configuration file sets "flow" to "{flow}", but ' + f"this is not a known flow (known: {known_flows})." ) raise RuntimeError( msg, ) - classes = [ - RdcCfg, - CdcCfg, - LintCfg, - SynCfg, - FormalCfg, - SimCfg, - ] - found_cls = None - known_types = [] - for cls in classes: - assert cls.flow is not None - known_types.append(cls.flow) - if cls.flow == flow: - found_cls = cls - break - if found_cls is None: - msg = ( - '{}: Configuration file sets "flow" to {!r}, but ' - "this is not a known flow (known: {}).".format(path, flow, ", ".join(known_types)) - ) - raise RuntimeError( - msg, - ) + return FLOW_HANDLERS[flow] - return (found_cls, hjson_data) - -def _make_child_cfg(path, args, initial_values): - try: - cls, hjson_data = _load_cfg(path, initial_values) - except RuntimeError as err: - log.exception(str(err)) - sys.exit(1) - - # Since this is a child configuration (from some primary configuration), - # make sure that we aren't ourselves a primary configuration. We don't need - # multi-level hierarchies and this avoids circular dependencies. - if "use_cfgs" in hjson_data: - msg = ( - f"{path}: Configuration file has use_cfgs, but is " - "itself included from another configuration." - ) - raise RuntimeError( - msg, - ) - - # Call cls as a constructor. Note that we pass None as the mk_config - # argument: this is not supposed to load anything else. - return cls(path, hjson_data, args, None) - - -def make_cfg(path, args, proj_root): +def make_cfg( + project_cfg: ProjectMeta, + select_cfgs: Sequence[str] | None, + args: Namespace, +) -> FlowCfg: """Make a flow config by loading the config file at path. - args is the arguments passed to the dvsim.py tool and proj_root is the top - of the project. + Args: + project_cfg: metadata about the project + select_cfgs: list of child config names to use from the primary config + args: are the arguments passed to the CLI + + Returns: + Instantiated FlowCfg object configured using the project's top level + config file. """ - initial_values = { - "proj_root": proj_root, - "self_dir": pathlib.Path(path).parent, - } + log.info("Loading primary config file: %s", project_cfg.top_cfg_path) + + # load the whole project config data + primary_cfg = dict( + load_cfg( + path=project_cfg.top_cfg_path, + path_resolution_wildcards={ + "proj_root": project_cfg.root_path, + }, + select_cfgs=select_cfgs, + ), + ) + + # Tool specified on CLI overrides the file based config if args.tool is not None: - initial_values["tool"] = args.tool - - try: - cls, hjson_data = _load_cfg(path, initial_values) - except RuntimeError as err: - log.exception(str(err)) - sys.exit(1) + primary_cfg["tool"] = args.tool - def factory(child_path): - child_ivs = initial_values.copy() - child_ivs["flow"] = hjson_data["flow"] - return _make_child_cfg(child_path, args, child_ivs) + if "flow" not in primary_cfg: + msg = 'No value for the "flow" key. Are you sure this is a dvsim configuration file?' + raise RuntimeError( + msg, + ) - return cls(path, hjson_data, args, factory) + cls = _get_flow_handler_cls(str(primary_cfg["flow"])) + + child_flow_handlers = [] + if "cfgs" in primary_cfg: + for child_cfg_path, child_cfg_data in primary_cfg["cfgs"].items(): + # Tool specified on CLI overrides the file based config + if args.tool is not None: + child_cfg_data["tool"] = args.tool + + log.info( + "Constructing child '%s' %s flow with config: '%s'", + child_cfg_data["name"], + child_cfg_data["flow"], + child_cfg_path, + ) + child_flow_handlers.append( + cls( + flow_cfg_file=child_cfg_path, + project_cfg=project_cfg, + config_data=child_cfg_data, + args=args, + ), + ) + + log.info( + "Constructing top level '%s' %s flow with config: '%s'", + primary_cfg["name"], + primary_cfg["flow"], + project_cfg.top_cfg_path, + ) + log.info("Constructing top level flow handler with %s", cls.__name__) + + return cls( + flow_cfg_file=project_cfg.top_cfg_path, + project_cfg=project_cfg, + config_data=primary_cfg, + args=args, + child_configs=child_flow_handlers, + ) diff --git a/src/dvsim/flow/formal.py b/src/dvsim/flow/formal.py index 716f1610..d8bb43d0 100644 --- a/src/dvsim/flow/formal.py +++ b/src/dvsim/flow/formal.py @@ -2,6 +2,10 @@ # Licensed under the Apache License, Version 2.0, see LICENSE for details. # SPDX-License-Identifier: Apache-2.0 +"""Formal flow.""" + +from argparse import Namespace +from collections.abc import Mapping, Sequence from pathlib import Path import hjson @@ -9,6 +13,7 @@ from dvsim.flow.one_shot import OneShotCfg from dvsim.logging import log +from dvsim.project import ProjectMeta from dvsim.utils import subst_wildcards @@ -17,11 +22,26 @@ class FormalCfg(OneShotCfg): flow = "formal" - def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: + def __init__( + self, + flow_cfg_file: Path, + project_cfg: ProjectMeta, + config_data: Mapping, + args: Namespace, + child_configs: Sequence["FormalCfg"] | None = None, + ) -> None: + """Formal flow config.""" # Options set from command line self.batch_mode_prefix = "" if args.gui else "-batch" - super().__init__(flow_cfg_file, hjson_data, args, mk_config) + super().__init__( + flow_cfg_file=flow_cfg_file, + project_cfg=project_cfg, + config_data=config_data, + args=args, + child_configs=child_configs, + ) + self.header = [ "name", "errors", @@ -36,8 +56,8 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: ] # Default not to publish child cfg results. - self.publish_report = hjson_data.get("publish_report", False) - self.sub_flow = hjson_data["sub_flow"] + self.publish_report = config_data.get("publish_report", False) + self.sub_flow = config_data["sub_flow"] self.summary_header = ["name", "pass_rate", "formal_cov", "stimuli_cov", "checker_cov"] self.results_title = self.name.upper() + " Formal " + self.sub_flow.upper() + " Results" diff --git a/src/dvsim/flow/hjson.py b/src/dvsim/flow/hjson.py deleted file mode 100644 index 4dd742f0..00000000 --- a/src/dvsim/flow/hjson.py +++ /dev/null @@ -1,190 +0,0 @@ -# Copyright lowRISC contributors (OpenTitan project). -# Licensed under the Apache License, Version 2.0, see LICENSE for details. -# SPDX-License-Identifier: Apache-2.0 - -"""A wrapper for loading hjson files as used by dvsim's FlowCfg.""" - -from dvsim.utils import parse_hjson, subst_wildcards - -# A set of fields that can be overridden on the command line and shouldn't be -# loaded from the hjson in that case. -_CMDLINE_FIELDS = {"tool"} - - -def load_hjson(path, initial_values): - """Load an hjson file and any includes. - - Combines them all into a single dictionary, which is then returned. This - does wildcard substitution on include names (since it might be needed to - find included files), but not otherwise. - - initial_values is a starting point for the dictionary to be returned (which - is not modified). It needs to contain values for anything needed to resolve - include files (typically, this is 'proj_root' and 'tool' (if set)). - - """ - worklist = [path] - seen = {path} - ret = initial_values.copy() - is_first = True - - # Figure out a list of fields that had a value from the command line. These - # should have been passed in as part of initial_values and we need to know - # that we can safely ignore updates. - arg_keys = _CMDLINE_FIELDS & initial_values.keys() - - while worklist: - next_path = worklist.pop() - new_paths = _load_single_file(ret, next_path, is_first, arg_keys) - paths_seen = set(new_paths) & seen - if paths_seen: - msg = ( - f"The files {list(paths_seen)!r} appears more than once " - f"when processing include {next_path!r} for {path!r}." - ) - raise RuntimeError( - msg, - ) - seen |= set(new_paths) - worklist += new_paths - is_first = False - - return ret - - -def _load_single_file(target, path, is_first, arg_keys): - """Load a single hjson file, merging its keys into target. - - Returns a list of further includes that should be loaded. - - """ - hjson = parse_hjson(path) - if not isinstance(hjson, dict): - msg = f"{path!r}: Top-level hjson object is not a dictionary." - raise RuntimeError(msg) - - import_cfgs = [] - for key, dict_val in hjson.items(): - # If this key got set at the start of time and we want to ignore any - # updates: ignore them! - if key in arg_keys: - continue - - # If key is 'import_cfgs', this should be a list. Add each item to the - # list of cfgs to process - if key == "import_cfgs": - if not isinstance(dict_val, list): - msg = f"{path!r}: import_cfgs value is {dict_val!r}, but should be a list." - raise RuntimeError( - msg, - ) - import_cfgs += dict_val - continue - - # 'use_cfgs' is a bit like 'import_cfgs', but is only used for primary - # config files (where it is a list of the child configs). This - # shouldn't be used except at top-level (the first configuration file - # to be loaded). - # - # If defined, check that it's a list, but then allow it to be set in - # the target dictionary as usual. - if key == "use_cfgs": - if not is_first: - msg = f'{path!r}: File is included by another one, but defines "use_cfgs".' - raise RuntimeError( - msg, - ) - if not isinstance(dict_val, list): - msg = f"{path!r}: use_cfgs must be a list. Saw {dict_val!r}." - raise RuntimeError( - msg, - ) - - # Otherwise, update target with this attribute - set_target_attribute(path, target, key, dict_val) - - # Expand the names of imported configuration files as we return them - return [ - subst_wildcards(cfg_path, target, ignored_wildcards=[], ignore_error=False) - for cfg_path in import_cfgs - ] - - -def set_target_attribute(path, target, key, dict_val) -> None: - """Set an attribute on the target dictionary. - - This performs checks for conflicting values and merges lists / - dictionaries. - - """ - old_val = target.get(key) - if old_val is None: - # A new attribute (or the old value was None, in which case it's - # just a placeholder and needs writing). Set it and return. - target[key] = dict_val - return - - if isinstance(old_val, list): - if not isinstance(dict_val, list): - msg = ( - f"{path!r}: Conflicting types for key {key!r}: was " - f"{old_val!r}, a list, but loaded value is {dict_val!r}, " - f"of type {type(dict_val).__name__}." - ) - raise RuntimeError( - msg, - ) - - # Lists are merged by concatenation - target[key] += dict_val - return - - # The other types we support are "scalar" types. - scalar_types = [(str, [""]), (int, [0, -1]), (bool, [False])] - defaults = None - for st_type, st_defaults in scalar_types: - if isinstance(dict_val, st_type): - defaults = st_defaults - break - if defaults is None: - msg = f"{path!r}: Value for key {key!r} is {dict_val!r}, of unknown type {type(dict_val).__name__}." - raise RuntimeError( - msg, - ) - if not isinstance(old_val, st_type): - msg = ( - f"{path!r}: Value for key {key!r} is {dict_val!r}, but " - f"we already had the value {old_val!r}, of an " - "incompatible type." - ) - raise RuntimeError( - msg, - ) - - # The types are compatible. If the values are equal, there's nothing more - # to do - if old_val == dict_val: - return - - old_is_default = old_val in defaults - new_is_default = dict_val in defaults - - # Similarly, if new value looks like a default, ignore it (regardless - # of whether the current value looks like a default). - if new_is_default: - return - - # If the existing value looks like a default and the new value doesn't, - # take the new value. - if old_is_default: - target[key] = dict_val - return - - # Neither value looks like a default. Raise an error. - msg = ( - f"{path!r}: Value for key {key!r} is {dict_val!r}, but " - f"we already had a conflicting value of {old_val!r}." - ) - raise RuntimeError( - msg, - ) diff --git a/src/dvsim/flow/lint.py b/src/dvsim/flow/lint.py index 66f72437..5d91b6b1 100644 --- a/src/dvsim/flow/lint.py +++ b/src/dvsim/flow/lint.py @@ -4,13 +4,16 @@ """Class describing lint configuration object.""" +import logging as log +from argparse import Namespace +from collections.abc import Mapping, Sequence from pathlib import Path from tabulate import tabulate from dvsim.flow.one_shot import OneShotCfg -from dvsim.logging import log from dvsim.msg_buckets import MsgBuckets +from dvsim.project import ProjectMeta from dvsim.utils import check_bool, subst_wildcards @@ -19,7 +22,14 @@ class LintCfg(OneShotCfg): flow = "lint" - def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: + def __init__( + self, + flow_cfg_file: Path, + project_cfg: ProjectMeta, + config_data: Mapping, + args: Namespace, + child_configs: Sequence["LintCfg"] | None = None, + ) -> None: # TODO: check whether this can be replaced with the subflow concept. # This determines whether the flow is for a style lint run. # Format: bool @@ -38,7 +48,13 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: # Format: str self.additional_fusesoc_argument = "" - super().__init__(flow_cfg_file, hjson_data, args, mk_config) + super().__init__( + flow_cfg_file=flow_cfg_file, + project_cfg=project_cfg, + config_data=config_data, + args=args, + child_configs=child_configs, + ) if self.is_style_lint == "": self.is_style_lint = False diff --git a/src/dvsim/flow/one_shot.py b/src/dvsim/flow/one_shot.py index 1a30f295..8be61da2 100644 --- a/src/dvsim/flow/one_shot.py +++ b/src/dvsim/flow/one_shot.py @@ -5,12 +5,16 @@ """Class describing a one-shot build configuration object.""" import os +from argparse import Namespace from collections import OrderedDict +from collections.abc import Mapping, Sequence +from pathlib import Path from dvsim.flow.base import FlowCfg from dvsim.job.deploy import CompileOneShot from dvsim.logging import log from dvsim.modes import BuildMode, Mode +from dvsim.project import ProjectMeta from dvsim.utils import rm_path @@ -21,7 +25,14 @@ class OneShotCfg(FlowCfg): ignored_wildcards = [*FlowCfg.ignored_wildcards, "build_mode", "index", "test"] - def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: + def __init__( + self, + flow_cfg_file: Path, + project_cfg: ProjectMeta, + config_data: Mapping, + args: Namespace, + child_configs: Sequence["OneShotCfg"] | None = None, + ) -> None: # Options set from command line self.tool = args.tool self.verbose = args.verbose @@ -72,16 +83,19 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: self.deploy = [] self.cov = args.cov - super().__init__(flow_cfg_file, hjson_data, args, mk_config) + super().__init__( + flow_cfg_file=flow_cfg_file, + project_cfg=project_cfg, + config_data=config_data, + args=args, + child_configs=child_configs, + ) - def _merge_hjson(self, hjson_data) -> None: - # If build_unique is set, then add current timestamp to uniquify it + def _expand(self) -> None: + """Expand wildcards.""" if self.build_unique: self.build_dir += "_" + self.timestamp - super()._merge_hjson(hjson_data) - - def _expand(self) -> None: super()._expand() # Stuff below only pertains to individual cfg (not primary cfg). diff --git a/src/dvsim/flow/rdc.py b/src/dvsim/flow/rdc.py index fcb50ab3..7d76336a 100644 --- a/src/dvsim/flow/rdc.py +++ b/src/dvsim/flow/rdc.py @@ -4,7 +4,12 @@ """RDC Configuration Class.""" +from argparse import Namespace +from collections.abc import Mapping, Sequence +from pathlib import Path + from dvsim.flow.lint import LintCfg +from dvsim.project import ProjectMeta class RdcCfg(LintCfg): @@ -12,9 +17,22 @@ class RdcCfg(LintCfg): flow = "rdc" - def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: + def __init__( + self, + flow_cfg_file: Path, + project_cfg: ProjectMeta, + config_data: Mapping, + args: Namespace, + child_configs: Sequence["RdcCfg"] | None = None, + ) -> None: self.waves = args.waves or "" - super().__init__(flow_cfg_file, hjson_data, args, mk_config) + super().__init__( + flow_cfg_file=flow_cfg_file, + project_cfg=project_cfg, + config_data=config_data, + args=args, + child_configs=child_configs, + ) self.results_title = f"{self.name.upper()} RDC Results" diff --git a/src/dvsim/flow/sim.py b/src/dvsim/flow/sim.py index 21b5796d..85a07da6 100644 --- a/src/dvsim/flow/sim.py +++ b/src/dvsim/flow/sim.py @@ -9,8 +9,9 @@ import os import re import sys +from argparse import Namespace from collections import OrderedDict, defaultdict -from collections.abc import Mapping +from collections.abc import Mapping, Sequence from datetime import datetime, timezone from pathlib import Path from typing import ClassVar @@ -21,6 +22,7 @@ from dvsim.job.deploy import CompileSim, CovAnalyze, CovMerge, CovReport, CovUnr, Deploy, RunTest from dvsim.logging import log from dvsim.modes import BuildMode, Mode, RunMode, find_mode +from dvsim.project import ProjectMeta from dvsim.regression import Regression from dvsim.sim_results import SimResults from dvsim.test import Test @@ -57,7 +59,17 @@ class SimCfg(FlowCfg): "sw_build_opts", ] - def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: + # TODO: move args processing outside and only take config_data + def __init__( + self, + *, + flow_cfg_file: Path, + project_cfg: ProjectMeta, + config_data: Mapping, + args: Namespace, + child_configs: Sequence["SimCfg"] | None = None, + ) -> None: + """Initialise a Sim flow Configuration.""" # Options set from command line self.tool = args.tool self.build_opts = [] @@ -147,7 +159,13 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: self.cov_report_deploy = None self.results_summary = OrderedDict() - super().__init__(flow_cfg_file, hjson_data, args, mk_config) + super().__init__( + flow_cfg_file=flow_cfg_file, + project_cfg=project_cfg, + config_data=config_data, + args=args, + child_configs=child_configs, + ) def _expand(self) -> None: # Choose a wave format now. Note that this has to happen after parsing diff --git a/src/dvsim/flow/syn.py b/src/dvsim/flow/syn.py index 333620bc..9a377f8b 100644 --- a/src/dvsim/flow/syn.py +++ b/src/dvsim/flow/syn.py @@ -4,6 +4,8 @@ """Class describing synthesis configuration object.""" +from argparse import Namespace +from collections.abc import Mapping, Sequence from pathlib import Path import hjson @@ -11,6 +13,7 @@ from dvsim.flow.one_shot import OneShotCfg from dvsim.logging import log +from dvsim.project import ProjectMeta from dvsim.utils import print_msg_list, subst_wildcards @@ -19,8 +22,22 @@ class SynCfg(OneShotCfg): flow = "syn" - def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: - super().__init__(flow_cfg_file, hjson_data, args, mk_config) + def __init__( + self, + flow_cfg_file: Path, + project_cfg: ProjectMeta, + config_data: Mapping, + args: Namespace, + child_configs: Sequence["SynCfg"] | None = None, + ) -> None: + super().__init__( + flow_cfg_file=flow_cfg_file, + project_cfg=project_cfg, + config_data=config_data, + args=args, + child_configs=child_configs, + ) + # Set the title for synthesis results. self.results_title = self.name.upper() + " Synthesis Results" From 9e4b6cd36dc4eeaecac2b80d559f9556d67ea3d7 Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Tue, 15 Apr 2025 17:33:58 +0100 Subject: [PATCH 07/13] refactor: [config] save the config to JSON file in the scratch dir Currently there is no tracability in terms of what config was actually used to produce the generated results. This commit adds serialisation to save the config file to the root of the scratch dir for later reference. At some point this could be used to restart or finish off a job that failed for some reason. Signed-off-by: James McCorrie --- src/dvsim/cli.py | 52 +++++----- src/dvsim/flow/factory.py | 47 ++++----- src/dvsim/project.py | 202 +++++++++++++++++++++++++------------- 3 files changed, 182 insertions(+), 119 deletions(-) diff --git a/src/dvsim/cli.py b/src/dvsim/cli.py index cb57f13d..62a8e3fe 100644 --- a/src/dvsim/cli.py +++ b/src/dvsim/cli.py @@ -22,14 +22,15 @@ import argparse import datetime -import logging as log +import json import os import random import sys import textwrap +from collections.abc import Sequence from pathlib import Path -from dvsim.flow.factory import make_cfg +from dvsim.flow.factory import make_flow from dvsim.job.deploy import RunTest from dvsim.launcher.base import Launcher from dvsim.launcher.factory import set_launcher_type @@ -38,8 +39,8 @@ from dvsim.launcher.nc import NcLauncher from dvsim.launcher.sge import SgeLauncher from dvsim.launcher.slurm import SlurmLauncher -from dvsim.logging import configure_logging -from dvsim.project import init_project +from dvsim.logging import configure_logging, log +from dvsim.project import Project from dvsim.utils import TS_FORMAT, TS_FORMAT_LONG, Timer # TODO: add dvsim_cfg.hjson to retrieve this info @@ -661,15 +662,19 @@ def main() -> None: debug=args.verbose == "debug", ) - project_cfg = init_project( + project_cfg = Project.init( cfg_path=Path(args.cfg), proj_root=Path(args.proj_root) if args.proj_root else None, scratch_root=Path(args.scratch_root) if args.scratch_root else None, branch=args.branch, + job_prefix=args.job_prefix, + purge=args.purge, dry_run=args.dry_run, remote=args.remote, - job_prefix=args.job_prefix, ) + project_cfg.save() + + log.set_logfile(path=project_cfg.logfile) # Add timestamp to args that all downstream objects can use. curr_ts = datetime.datetime.now(datetime.UTC) @@ -696,53 +701,56 @@ def main() -> None: # Build infrastructure from hjson file and create the list of items to # be deployed. - cfg = make_cfg( - project_cfg=project_cfg, + config_data = project_cfg.load_config( select_cfgs=args.select_cfgs, args=args, ) + project_cfg.run_dir.mkdir(parents=True, exist_ok=True) + (project_cfg.run_dir / "config.json").write_text(json.dumps(config_data)) + + flow = make_flow( + project_cfg=project_cfg, + config_data=config_data, + args=args, + ) # List items available for run if --list switch is passed, and exit. if args.list is not None: - cfg.print_list() + flow.print_list() sys.exit(0) # Purge the scratch path if --purge option is set. if args.purge: - cfg.purge() + flow.purge() # If --cov-unr is passed, run UNR to generate report for unreachable # exclusion file. if args.cov_unr: - cfg.cov_unr() - cfg.deploy_objects() + flow.cov_unr() + flow.deploy_objects() sys.exit(0) # In simulation mode: if --cov-analyze switch is passed, then run the GUI # tool. if args.cov_analyze: - cfg.cov_analyze() - cfg.deploy_objects() + flow.cov_analyze() + flow.deploy_objects() sys.exit(0) # Deploy the builds and runs if args.items: # Create deploy objects. - cfg.create_deploy_objects() - results = cfg.deploy_objects() + flow.create_deploy_objects() + results = flow.deploy_objects() # Generate results. - cfg.gen_results(results) + flow.gen_results(results) else: log.error("Nothing to run!") sys.exit(1) # Exit with non-zero status if there were errors or failures. - if cfg.has_errors(): + if flow.has_errors(): log.error("Errors were encountered in this run.") sys.exit(1) - - -if __name__ == "__main__": - main() diff --git a/src/dvsim/flow/factory.py b/src/dvsim/flow/factory.py index d9d5a741..ad93dc20 100644 --- a/src/dvsim/flow/factory.py +++ b/src/dvsim/flow/factory.py @@ -5,9 +5,8 @@ """Factory to generate a flow config.""" from argparse import Namespace -from collections.abc import Sequence +from collections.abc import Mapping -from dvsim.config.load import load_cfg from dvsim.flow.base import FlowCfg from dvsim.flow.cdc import CdcCfg from dvsim.flow.formal import FormalCfg @@ -16,7 +15,9 @@ from dvsim.flow.sim import SimCfg from dvsim.flow.syn import SynCfg from dvsim.logging import log -from dvsim.project import ProjectMeta +from dvsim.project import Project + +__all__ = ("make_flow",) FLOW_HANDLERS = { "cdc": CdcCfg, @@ -51,9 +52,12 @@ def _get_flow_handler_cls(flow: str) -> type[FlowCfg]: return FLOW_HANDLERS[flow] -def make_cfg( - project_cfg: ProjectMeta, - select_cfgs: Sequence[str] | None, +# TODO: move this to ProjectMeta -> ProjectCfg + + +def make_flow( + project_cfg: Project, + config_data: Mapping, args: Namespace, ) -> FlowCfg: """Make a flow config by loading the config file at path. @@ -68,34 +72,17 @@ def make_cfg( config file. """ - log.info("Loading primary config file: %s", project_cfg.top_cfg_path) - - # load the whole project config data - primary_cfg = dict( - load_cfg( - path=project_cfg.top_cfg_path, - path_resolution_wildcards={ - "proj_root": project_cfg.root_path, - }, - select_cfgs=select_cfgs, - ), - ) - - # Tool specified on CLI overrides the file based config - if args.tool is not None: - primary_cfg["tool"] = args.tool - - if "flow" not in primary_cfg: + if "flow" not in config_data: msg = 'No value for the "flow" key. Are you sure this is a dvsim configuration file?' raise RuntimeError( msg, ) - cls = _get_flow_handler_cls(str(primary_cfg["flow"])) + cls = _get_flow_handler_cls(str(config_data["flow"])) child_flow_handlers = [] - if "cfgs" in primary_cfg: - for child_cfg_path, child_cfg_data in primary_cfg["cfgs"].items(): + if "cfgs" in config_data: + for child_cfg_path, child_cfg_data in config_data["cfgs"].items(): # Tool specified on CLI overrides the file based config if args.tool is not None: child_cfg_data["tool"] = args.tool @@ -117,8 +104,8 @@ def make_cfg( log.info( "Constructing top level '%s' %s flow with config: '%s'", - primary_cfg["name"], - primary_cfg["flow"], + config_data["name"], + config_data["flow"], project_cfg.top_cfg_path, ) log.info("Constructing top level flow handler with %s", cls.__name__) @@ -126,7 +113,7 @@ def make_cfg( return cls( flow_cfg_file=project_cfg.top_cfg_path, project_cfg=project_cfg, - config_data=primary_cfg, + config_data=config_data, args=args, child_configs=child_flow_handlers, ) diff --git a/src/dvsim/project.py b/src/dvsim/project.py index 899ffd0d..a71387a1 100644 --- a/src/dvsim/project.py +++ b/src/dvsim/project.py @@ -8,22 +8,33 @@ import shlex import subprocess import sys -from dataclasses import dataclass +from argparse import Namespace +from collections.abc import Mapping, Sequence from pathlib import Path +from pydantic import BaseModel, ConfigDict + +from dvsim.config.load import load_cfg from dvsim.logging import log from dvsim.utils import ( rm_path, run_cmd_with_timeout, ) -__all__ = ("init_project",) +__all__ = ("Project",) + + +class FlowConfig(BaseModel): + """Flow configuration data.""" + model_config = ConfigDict(frozen=True, extra="allow") -@dataclass(frozen=True, kw_only=True) -class ProjectMeta: + +class Project(BaseModel): """Project meta data.""" + model_config = ConfigDict(frozen=True, extra="forbid") + top_cfg_path: Path root_path: Path src_path: Path @@ -31,73 +42,130 @@ class ProjectMeta: branch: str job_prefix: str + logfile: Path + run_dir: Path -def init_project( - cfg_path: Path, - proj_root: Path | None, - scratch_root: Path | None, - branch: str, - *, - job_prefix: str = "", - purge: bool = False, - dry_run: bool = False, - remote: bool = False, -) -> ProjectMeta: - """Initialise a project workspace. - - If --remote switch is set, a location in the scratch area is chosen as the - new proj_root. The entire repo is copied over to this location. Else, the - proj_root is discovered using get_proj_root() method, unless the user - overrides it on the command line. - - This function returns the updated proj_root src and destination path. If - --remote switch is not set, the destination path is identical to the src - path. Likewise, if --dry-run is set. - """ - if not cfg_path.exists(): - log.fatal("Path to config file %s appears to be invalid.", cfg_path) - sys.exit(1) + def save(self) -> None: + """Save project meta to file.""" + meta_json = self.model_dump_json(indent=2) - branch = resolve_branch(branch) + log.debug("Project meta:\n%s", meta_json) - src_path = Path(proj_root) if proj_root else get_proj_root() + self.run_dir.mkdir(parents=True, exist_ok=True) - scratch_path = resolve_scratch_root( - scratch_root, - src_path, - ) + (self.run_dir / "project.json").write_text(meta_json) - # Check if jobs are dispatched to external compute machines. If yes, - # then the repo needs to be copied over to the scratch area - # accessible to those machines. - # If --purge arg is set, then purge the repo_top that was copied before. - if remote and not dry_run: - root_path = scratch_path / branch / "repo_top" - if purge: - rm_path(root_path) - copy_repo(src_path, root_path) - else: - root_path = src_path - - log.info("[proj_root]: %s", root_path) - - # Create an empty FUSESOC_IGNORE file in scratch_root. This ensures that - # any fusesoc invocation from a job won't search within scratch_root for - # core files. - (scratch_path / "FUSESOC_IGNORE").touch() - - cfg_path = cfg_path.resolve() - if remote: - cfg_path = root_path / cfg_path.relative_to(src_path) - - return ProjectMeta( - top_cfg_path=cfg_path, - root_path=root_path, - src_path=src_path, - scratch_path=scratch_path, - branch=branch, - job_prefix=job_prefix, - ) + def load_config( + self, + select_cfgs: Sequence[str] | None, + args: Namespace, + ) -> Mapping: + """Load the project configuration. + + Args: + project_cfg: metadata about the project + select_cfgs: list of child config names to use from the primary config + args: are the arguments passed to the CLI + + Returns: + Project configuration. + + """ + log.info("Loading primary config file: %s", self.top_cfg_path) + + # load the whole project config data + cfg = dict( + load_cfg( + path=self.top_cfg_path, + path_resolution_wildcards={ + "proj_root": self.root_path, + }, + select_cfgs=select_cfgs, + ), + ) + + # Tool specified on CLI overrides the file based config + if args.tool is not None: + cfg["tool"] = args.tool + + return cfg + + @staticmethod + def load(path: Path) -> "Project": + """Load project meta from file.""" + data = (path / "project.json").read_text() + return Project.model_validate_json(data) + + @staticmethod + def init( + cfg_path: Path, + proj_root: Path | None, + scratch_root: Path | None, + branch: str, + *, + job_prefix: str = "", + purge: bool = False, + dry_run: bool = False, + remote: bool = False, + ) -> "Project": + """Initialise a project workspace. + + If --remote switch is set, a location in the scratch area is chosen as the + new proj_root. The entire repo is copied over to this location. Else, the + proj_root is discovered using get_proj_root() method, unless the user + overrides it on the command line. + + This function returns the updated proj_root src and destination path. If + --remote switch is not set, the destination path is identical to the src + path. Likewise, if --dry-run is set. + """ + if not cfg_path.exists(): + log.fatal("Path to config file %s appears to be invalid.", cfg_path) + sys.exit(1) + + branch = _resolve_branch(branch) + + src_path = Path(proj_root) if proj_root else get_proj_root() + + scratch_path = resolve_scratch_root( + scratch_root, + src_path, + ) + + # Check if jobs are dispatched to external compute machines. If yes, + # then the repo needs to be copied over to the scratch area + # accessible to those machines. + # If --purge arg is set, then purge the repo_top that was copied before. + if remote and not dry_run: + root_path = scratch_path / branch / "repo_top" + if purge: + rm_path(root_path) + copy_repo(src_path, root_path) + else: + root_path = src_path + + log.info("[proj_root]: %s", root_path) + + # Create an empty FUSESOC_IGNORE file in scratch_root. This ensures that + # any fusesoc invocation from a job won't search within scratch_root for + # core files. + (scratch_path / "FUSESOC_IGNORE").touch() + + cfg_path = cfg_path.resolve() + if remote: + cfg_path = root_path / cfg_path.relative_to(src_path) + + run_dir = scratch_path / branch + return Project( + top_cfg_path=cfg_path, + root_path=root_path, + src_path=src_path, + scratch_path=scratch_path, + branch=branch, + job_prefix=job_prefix, + logfile=run_dir / "run.log", + run_dir=run_dir, + ) def _network_dir_accessible_and_exists( @@ -270,7 +338,7 @@ def copy_repo(src: Path, dest: Path) -> None: log.info("Done.") -def resolve_branch(branch: str | None) -> str: +def _resolve_branch(branch: str | None) -> str: """Choose a branch name for output files. If the --branch argument was passed on the command line, the branch From e1ce0ba657315a5639c0b28a95a63a12e799659e Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Wed, 16 Apr 2025 10:22:55 +0100 Subject: [PATCH 08/13] refactor: [project] rename ProjectMeta to Project Given the ProjectMeta object is becoming more than just meta data. Signed-off-by: James McCorrie --- src/dvsim/flow/base.py | 4 ++-- src/dvsim/flow/cdc.py | 4 ++-- src/dvsim/flow/factory.py | 5 +---- src/dvsim/flow/formal.py | 4 ++-- src/dvsim/flow/lint.py | 4 ++-- src/dvsim/flow/one_shot.py | 4 ++-- src/dvsim/flow/rdc.py | 4 ++-- src/dvsim/flow/sim.py | 4 ++-- src/dvsim/flow/syn.py | 4 ++-- tests/test_project.py | 43 ++++++++++++++++++++++++++++++++++++++ 10 files changed, 60 insertions(+), 20 deletions(-) create mode 100644 tests/test_project.py diff --git a/src/dvsim/flow/base.py b/src/dvsim/flow/base.py index 2d21a336..072e65bd 100644 --- a/src/dvsim/flow/base.py +++ b/src/dvsim/flow/base.py @@ -17,7 +17,7 @@ from dvsim.job.deploy import Deploy from dvsim.launcher.factory import get_launcher_cls from dvsim.logging import log -from dvsim.project import ProjectMeta +from dvsim.project import Project from dvsim.scheduler import Scheduler from dvsim.utils import ( find_and_substitute_wildcards, @@ -46,7 +46,7 @@ def __init__( self, *, flow_cfg_file: Path, - project_cfg: ProjectMeta, + project_cfg: Project, config_data: Mapping, args: Namespace, child_configs: Sequence["FlowCfg"] | None = None, diff --git a/src/dvsim/flow/cdc.py b/src/dvsim/flow/cdc.py index ba846172..70f91116 100644 --- a/src/dvsim/flow/cdc.py +++ b/src/dvsim/flow/cdc.py @@ -9,7 +9,7 @@ from pathlib import Path from dvsim.flow.lint import LintCfg -from dvsim.project import ProjectMeta +from dvsim.project import Project class CdcCfg(LintCfg): @@ -20,7 +20,7 @@ class CdcCfg(LintCfg): def __init__( self, flow_cfg_file: Path, - project_cfg: ProjectMeta, + project_cfg: Project, config_data: Mapping, args: Namespace, child_configs: Sequence["CdcCfg"] | None = None, diff --git a/src/dvsim/flow/factory.py b/src/dvsim/flow/factory.py index ad93dc20..701508b7 100644 --- a/src/dvsim/flow/factory.py +++ b/src/dvsim/flow/factory.py @@ -52,9 +52,6 @@ def _get_flow_handler_cls(flow: str) -> type[FlowCfg]: return FLOW_HANDLERS[flow] -# TODO: move this to ProjectMeta -> ProjectCfg - - def make_flow( project_cfg: Project, config_data: Mapping, @@ -64,7 +61,7 @@ def make_flow( Args: project_cfg: metadata about the project - select_cfgs: list of child config names to use from the primary config + config_data: project configuration data args: are the arguments passed to the CLI Returns: diff --git a/src/dvsim/flow/formal.py b/src/dvsim/flow/formal.py index d8bb43d0..7a0663e5 100644 --- a/src/dvsim/flow/formal.py +++ b/src/dvsim/flow/formal.py @@ -13,7 +13,7 @@ from dvsim.flow.one_shot import OneShotCfg from dvsim.logging import log -from dvsim.project import ProjectMeta +from dvsim.project import Project from dvsim.utils import subst_wildcards @@ -25,7 +25,7 @@ class FormalCfg(OneShotCfg): def __init__( self, flow_cfg_file: Path, - project_cfg: ProjectMeta, + project_cfg: Project, config_data: Mapping, args: Namespace, child_configs: Sequence["FormalCfg"] | None = None, diff --git a/src/dvsim/flow/lint.py b/src/dvsim/flow/lint.py index 5d91b6b1..0baeace2 100644 --- a/src/dvsim/flow/lint.py +++ b/src/dvsim/flow/lint.py @@ -13,7 +13,7 @@ from dvsim.flow.one_shot import OneShotCfg from dvsim.msg_buckets import MsgBuckets -from dvsim.project import ProjectMeta +from dvsim.project import Project from dvsim.utils import check_bool, subst_wildcards @@ -25,7 +25,7 @@ class LintCfg(OneShotCfg): def __init__( self, flow_cfg_file: Path, - project_cfg: ProjectMeta, + project_cfg: Project, config_data: Mapping, args: Namespace, child_configs: Sequence["LintCfg"] | None = None, diff --git a/src/dvsim/flow/one_shot.py b/src/dvsim/flow/one_shot.py index 8be61da2..c66b96a8 100644 --- a/src/dvsim/flow/one_shot.py +++ b/src/dvsim/flow/one_shot.py @@ -14,7 +14,7 @@ from dvsim.job.deploy import CompileOneShot from dvsim.logging import log from dvsim.modes import BuildMode, Mode -from dvsim.project import ProjectMeta +from dvsim.project import Project from dvsim.utils import rm_path @@ -28,7 +28,7 @@ class OneShotCfg(FlowCfg): def __init__( self, flow_cfg_file: Path, - project_cfg: ProjectMeta, + project_cfg: Project, config_data: Mapping, args: Namespace, child_configs: Sequence["OneShotCfg"] | None = None, diff --git a/src/dvsim/flow/rdc.py b/src/dvsim/flow/rdc.py index 7d76336a..118cfb8f 100644 --- a/src/dvsim/flow/rdc.py +++ b/src/dvsim/flow/rdc.py @@ -9,7 +9,7 @@ from pathlib import Path from dvsim.flow.lint import LintCfg -from dvsim.project import ProjectMeta +from dvsim.project import Project class RdcCfg(LintCfg): @@ -20,7 +20,7 @@ class RdcCfg(LintCfg): def __init__( self, flow_cfg_file: Path, - project_cfg: ProjectMeta, + project_cfg: Project, config_data: Mapping, args: Namespace, child_configs: Sequence["RdcCfg"] | None = None, diff --git a/src/dvsim/flow/sim.py b/src/dvsim/flow/sim.py index 85a07da6..7102c55a 100644 --- a/src/dvsim/flow/sim.py +++ b/src/dvsim/flow/sim.py @@ -22,7 +22,7 @@ from dvsim.job.deploy import CompileSim, CovAnalyze, CovMerge, CovReport, CovUnr, Deploy, RunTest from dvsim.logging import log from dvsim.modes import BuildMode, Mode, RunMode, find_mode -from dvsim.project import ProjectMeta +from dvsim.project import Project from dvsim.regression import Regression from dvsim.sim_results import SimResults from dvsim.test import Test @@ -64,7 +64,7 @@ def __init__( self, *, flow_cfg_file: Path, - project_cfg: ProjectMeta, + project_cfg: Project, config_data: Mapping, args: Namespace, child_configs: Sequence["SimCfg"] | None = None, diff --git a/src/dvsim/flow/syn.py b/src/dvsim/flow/syn.py index 9a377f8b..cc2e698c 100644 --- a/src/dvsim/flow/syn.py +++ b/src/dvsim/flow/syn.py @@ -13,7 +13,7 @@ from dvsim.flow.one_shot import OneShotCfg from dvsim.logging import log -from dvsim.project import ProjectMeta +from dvsim.project import Project from dvsim.utils import print_msg_list, subst_wildcards @@ -25,7 +25,7 @@ class SynCfg(OneShotCfg): def __init__( self, flow_cfg_file: Path, - project_cfg: ProjectMeta, + project_cfg: Project, config_data: Mapping, args: Namespace, child_configs: Sequence["SynCfg"] | None = None, diff --git a/tests/test_project.py b/tests/test_project.py new file mode 100644 index 00000000..3222b0ac --- /dev/null +++ b/tests/test_project.py @@ -0,0 +1,43 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Test project meta functions.""" + +from collections.abc import Mapping +from pathlib import Path + +import pytest +from hamcrest import assert_that, equal_to + +from dvsim.project import Project + +__all__ = () + + +@pytest.mark.parametrize( + "data", + [ + { + "top_cfg_path": Path("cfg_path.hjson"), + "root_path": Path("root_path"), + "src_path": Path("src_path"), + "branch": "branch", + "job_prefix": "job_prefix", + "logfile": Path("logfile"), + }, + ], +) +def test_project_meta(data: Mapping, tmp_path: Path) -> None: + """Test Project saving and loading.""" + meta = Project( + **data, + scratch_path=tmp_path, + run_dir=tmp_path / data["branch"], + ) + + meta.save() + + loaded_meta = Project.load(path=meta.run_dir) + + assert_that(loaded_meta, equal_to(meta)) From c19c4546210ea2b940e2e74b17a3d9b9bf7c8ef0 Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Wed, 16 Apr 2025 11:56:22 +0100 Subject: [PATCH 09/13] refactor: [config] move the flow config into the project config This means the project config contains the whole project configuration including the flow config. This can be broken down later into smaller Pydantic objects as the fields and types become better defined. Signed-off-by: James McCorrie --- src/dvsim/cli.py | 12 +--- src/dvsim/flow/factory.py | 5 +- src/dvsim/project.py | 116 ++++++++++++++++++++++++++------------ tests/test_project.py | 36 ++++++++---- 4 files changed, 110 insertions(+), 59 deletions(-) diff --git a/src/dvsim/cli.py b/src/dvsim/cli.py index 62a8e3fe..7f9df627 100644 --- a/src/dvsim/cli.py +++ b/src/dvsim/cli.py @@ -22,12 +22,10 @@ import argparse import datetime -import json import os import random import sys import textwrap -from collections.abc import Sequence from pathlib import Path from dvsim.flow.factory import make_flow @@ -671,6 +669,8 @@ def main() -> None: purge=args.purge, dry_run=args.dry_run, remote=args.remote, + select_cfgs=args.select_cfgs, + args=args, ) project_cfg.save() @@ -701,16 +701,8 @@ def main() -> None: # Build infrastructure from hjson file and create the list of items to # be deployed. - config_data = project_cfg.load_config( - select_cfgs=args.select_cfgs, - args=args, - ) - project_cfg.run_dir.mkdir(parents=True, exist_ok=True) - (project_cfg.run_dir / "config.json").write_text(json.dumps(config_data)) - flow = make_flow( project_cfg=project_cfg, - config_data=config_data, args=args, ) diff --git a/src/dvsim/flow/factory.py b/src/dvsim/flow/factory.py index 701508b7..5df4a187 100644 --- a/src/dvsim/flow/factory.py +++ b/src/dvsim/flow/factory.py @@ -5,7 +5,6 @@ """Factory to generate a flow config.""" from argparse import Namespace -from collections.abc import Mapping from dvsim.flow.base import FlowCfg from dvsim.flow.cdc import CdcCfg @@ -54,7 +53,6 @@ def _get_flow_handler_cls(flow: str) -> type[FlowCfg]: def make_flow( project_cfg: Project, - config_data: Mapping, args: Namespace, ) -> FlowCfg: """Make a flow config by loading the config file at path. @@ -69,6 +67,9 @@ def make_flow( config file. """ + # convert back to simple dict + config_data = project_cfg.config.model_dump() + if "flow" not in config_data: msg = 'No value for the "flow" key. Are you sure this is a dvsim configuration file?' raise RuntimeError( diff --git a/src/dvsim/project.py b/src/dvsim/project.py index a71387a1..211e6d02 100644 --- a/src/dvsim/project.py +++ b/src/dvsim/project.py @@ -29,6 +29,21 @@ class FlowConfig(BaseModel): model_config = ConfigDict(frozen=True, extra="allow") + flow: str + name: str + + +class TopFlowConfig(BaseModel): + """Flow configuration data.""" + + model_config = ConfigDict(frozen=True, extra="allow") + + flow: str + project: str + revision: str + + cfgs: Mapping[Path, FlowConfig] + class Project(BaseModel): """Project meta data.""" @@ -41,6 +56,7 @@ class Project(BaseModel): scratch_path: Path branch: str job_prefix: str + config: TopFlowConfig | FlowConfig logfile: Path run_dir: Path @@ -55,41 +71,6 @@ def save(self) -> None: (self.run_dir / "project.json").write_text(meta_json) - def load_config( - self, - select_cfgs: Sequence[str] | None, - args: Namespace, - ) -> Mapping: - """Load the project configuration. - - Args: - project_cfg: metadata about the project - select_cfgs: list of child config names to use from the primary config - args: are the arguments passed to the CLI - - Returns: - Project configuration. - - """ - log.info("Loading primary config file: %s", self.top_cfg_path) - - # load the whole project config data - cfg = dict( - load_cfg( - path=self.top_cfg_path, - path_resolution_wildcards={ - "proj_root": self.root_path, - }, - select_cfgs=select_cfgs, - ), - ) - - # Tool specified on CLI overrides the file based config - if args.tool is not None: - cfg["tool"] = args.tool - - return cfg - @staticmethod def load(path: Path) -> "Project": """Load project meta from file.""" @@ -102,6 +83,8 @@ def init( proj_root: Path | None, scratch_root: Path | None, branch: str, + select_cfgs: Sequence[str] | None, + args: Namespace, *, job_prefix: str = "", purge: bool = False, @@ -118,6 +101,19 @@ def init( This function returns the updated proj_root src and destination path. If --remote switch is not set, the destination path is identical to the src path. Likewise, if --dry-run is set. + + Args: + args: are the arguments passed to the CLI + branch: version control branch + cfg_path: path to the top flow config + dry_run: do not run any jobs just go through the motions + job_prefix: prefix for the job name + proj_root: path to the project root + purge: bool = False, + remote: remote execution + scratch_root: path to the scratch dir + select_cfgs: list of child config names to use from the primary config + """ if not cfg_path.exists(): log.fatal("Path to config file %s appears to be invalid.", cfg_path) @@ -156,6 +152,14 @@ def init( cfg_path = root_path / cfg_path.relative_to(src_path) run_dir = scratch_path / branch + + config = _load_flow_config( + top_cfg_path=cfg_path, + root_path=root_path, + select_cfgs=select_cfgs, + args=args, + ) + return Project( top_cfg_path=cfg_path, root_path=root_path, @@ -165,9 +169,51 @@ def init( job_prefix=job_prefix, logfile=run_dir / "run.log", run_dir=run_dir, + config=config, ) +def _load_flow_config( + top_cfg_path: Path, + root_path: Path, + select_cfgs: Sequence[str] | None, + args: Namespace, +) -> TopFlowConfig | FlowConfig: + """Load the project configuration. + + Args: + top_cfg_path: path to the top level config file + root_path: path to the root of the project, + select_cfgs: list of child config names to use from the primary config + args: are the arguments passed to the CLI + + Returns: + Project configuration. + + """ + log.info("Loading primary config file: %s", top_cfg_path) + + # load the whole project config data + cfg = dict( + load_cfg( + path=top_cfg_path, + path_resolution_wildcards={ + "proj_root": root_path, + }, + select_cfgs=select_cfgs, + ), + ) + + # Tool specified on CLI overrides the file based config + if args.tool is not None: + cfg["tool"] = args.tool + + if "cfgs" in cfg: + return TopFlowConfig.model_validate(cfg) + + return FlowConfig.model_validate(cfg) + + def _network_dir_accessible_and_exists( path: Path, timeout: int = 1, diff --git a/tests/test_project.py b/tests/test_project.py index 3222b0ac..5d732df1 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -8,27 +8,38 @@ from pathlib import Path import pytest -from hamcrest import assert_that, equal_to +from hamcrest import assert_that, equal_to, instance_of -from dvsim.project import Project +from dvsim.project import FlowConfig, Project, TopFlowConfig __all__ = () @pytest.mark.parametrize( - "data", + ("data", "flow_config_cls"), [ - { - "top_cfg_path": Path("cfg_path.hjson"), - "root_path": Path("root_path"), - "src_path": Path("src_path"), - "branch": "branch", - "job_prefix": "job_prefix", - "logfile": Path("logfile"), - }, + ( + { + "top_cfg_path": Path("cfg_path.hjson"), + "root_path": Path("root_path"), + "src_path": Path("src_path"), + "branch": "branch", + "job_prefix": "job_prefix", + "logfile": Path("logfile"), + "config": { + "flow": "flow", + "name": "name", + }, + }, + FlowConfig, + ), ], ) -def test_project_meta(data: Mapping, tmp_path: Path) -> None: +def test_project_config( + data: Mapping, + flow_config_cls: type[FlowConfig | TopFlowConfig], + tmp_path: Path, +) -> None: """Test Project saving and loading.""" meta = Project( **data, @@ -41,3 +52,4 @@ def test_project_meta(data: Mapping, tmp_path: Path) -> None: loaded_meta = Project.load(path=meta.run_dir) assert_that(loaded_meta, equal_to(meta)) + assert_that(loaded_meta.config, instance_of(flow_config_cls)) From c6d9d7f3c384d4303fcdccdf3e2a3d202411c9c9 Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Fri, 18 Apr 2025 08:21:05 +0100 Subject: [PATCH 10/13] refactor: small tidy up refactor Signed-off-by: James McCorrie --- src/dvsim/cli.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/dvsim/cli.py b/src/dvsim/cli.py index 7f9df627..2c086e99 100644 --- a/src/dvsim/cli.py +++ b/src/dvsim/cli.py @@ -729,19 +729,18 @@ def main() -> None: flow.deploy_objects() sys.exit(0) - # Deploy the builds and runs - if args.items: - # Create deploy objects. - flow.create_deploy_objects() - results = flow.deploy_objects() - - # Generate results. - flow.gen_results(results) - - else: + if not args.items: log.error("Nothing to run!") sys.exit(1) + # Deploy the builds and runs + # Create deploy objects. + flow.create_deploy_objects() + results = flow.deploy_objects() + + # Generate results. + flow.gen_results(results) + # Exit with non-zero status if there were errors or failures. if flow.has_errors(): log.error("Errors were encountered in this run.") From 973b20147fa035cbba5b34a031790c9e2174f95a Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Fri, 18 Apr 2025 10:22:41 +0100 Subject: [PATCH 11/13] fix: [config] add explicit relative path field to the config loading The rel_path is a calculated field that is critical to determining where files are placed and found in the scratch dir and also the reports dir. At the moment the field is not created until the flow objects are created. Which means that the saved config doesn't have the information unless it's overridden in the config itself. This commit moves the calculated field out of the flow base class init into the config loading stage where it is saved the filesystem in the scratch dir along with the rest of the loaded config. Which means it can be used to navigate the scratch dir after a run. Signed-off-by: James McCorrie --- src/dvsim/project.py | 13 +++++++++++++ tests/test_project.py | 2 ++ 2 files changed, 15 insertions(+) diff --git a/src/dvsim/project.py b/src/dvsim/project.py index 211e6d02..c626f5a0 100644 --- a/src/dvsim/project.py +++ b/src/dvsim/project.py @@ -32,6 +32,9 @@ class FlowConfig(BaseModel): flow: str name: str + self_dir: Path + rel_path: Path + class TopFlowConfig(BaseModel): """Flow configuration data.""" @@ -42,6 +45,9 @@ class TopFlowConfig(BaseModel): project: str revision: str + self_dir: Path + rel_path: Path + cfgs: Mapping[Path, FlowConfig] @@ -209,6 +215,13 @@ def _load_flow_config( cfg["tool"] = args.tool if "cfgs" in cfg: + # add any missing rel_path fields + for child_cfg in cfg["cfgs"].values(): + if "rel_path" not in child_cfg: + child_cfg["rel_path"] = child_cfg["self_dir"].relative_to( + root_path, + ) + return TopFlowConfig.model_validate(cfg) return FlowConfig.model_validate(cfg) diff --git a/tests/test_project.py b/tests/test_project.py index 5d732df1..5c5e0b23 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -29,6 +29,8 @@ "config": { "flow": "flow", "name": "name", + "self_dir": Path("self_dir"), + "rel_path": Path("rel_path"), }, }, FlowConfig, From 53246fd5e2e2d7edf2f31e7d324cc7199e4fb3f2 Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Fri, 18 Apr 2025 16:31:20 +0100 Subject: [PATCH 12/13] refactor: [config] move overrides from flow base to config loading stage This is to ensure that the saved project config is already resolved otherwise the overrides mechanism would need to be implemented in any code that loads the project config. Without this change, the report generation code can't see the correct rel_path field as it is overridden in the flow object after being saved as part of the project config. Signed-off-by: James McCorrie --- src/dvsim/config/load.py | 20 ++++++++++++++++++++ src/dvsim/flow/base.py | 19 +++++++------------ 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/dvsim/config/load.py b/src/dvsim/config/load.py index 6214f5bd..b0955213 100644 --- a/src/dvsim/config/load.py +++ b/src/dvsim/config/load.py @@ -421,6 +421,23 @@ def _merge_use_cfgs( return {k: v for k, v in cfg.items() if k != "use_cfgs"} +def _apply_overrides(cfg: Mapping[str, object]) -> Mapping[str, object]: + """Apply overrides that are found in the config.""" + if "overrides" not in cfg: + return cfg + + overrides = cfg["overrides"] + log.debug("applying overrides: %s", overrides) + + # copy and filter out the overrides field + new_cfg = {k: v for k, v in cfg.items() if k != "overrides"} + + # apply the overrides + new_cfg.update({o["name"]: o["value"] for o in overrides}) + + return new_cfg + + def load_cfg( path: Path, *, @@ -502,6 +519,9 @@ def load_cfg( wildcard_values=path_resolution_wildcards, ) + # Apply overrides + cfg_data = _apply_overrides(cfg=cfg_data) + # Import any use_cfgs child config files return _merge_use_cfgs( top_cfg=cfg_data, diff --git a/src/dvsim/flow/base.py b/src/dvsim/flow/base.py index 072e65bd..92464b86 100644 --- a/src/dvsim/flow/base.py +++ b/src/dvsim/flow/base.py @@ -81,10 +81,6 @@ def __init__( # process' environment. self.exports = [] - # Add overrides using the overrides keyword - existing attributes - # are overridden with the override values. - self.overrides = [] - # Add a notion of "primary" cfg - this is indicated using # a special key 'use_cfgs' within the hjson cfg. self.is_primary_cfg = child_configs is not None @@ -127,15 +123,14 @@ def __init__( # Merge in the values from the loaded config file. self.__dict__.update(self._config_data) - # Is this a primary config? If so, we need to load up all the child - # configurations at this point. If not, we place ourselves into - # self.cfgs and consider ourselves a sort of "degenerate primary - # configuration". + # Expand wildcards. If subclasses need to mess around with parameters + # after merging the hjson but before expansion, they can override + # _expand and add the code at the start. + self._expand() - if self.rel_path == "": - self.rel_path = self.flow_cfg_file.parent.relative_to( - self._project_cfg.root_path, - ) + # Construct the path variables after variable expansion. + self.results_dir = Path(self.scratch_base_path) / "reports" / self.rel_path + self.results_page = self.results_dir / self.results_html_name def _expand(self) -> None: """Expand wildcards after merging hjson. From 2113e819a30b09925c2e9f78917f77b6bdbf91a2 Mon Sep 17 00:00:00 2001 From: James McCorrie Date: Fri, 18 Apr 2025 17:00:51 +0100 Subject: [PATCH 13/13] refactor: [config] resolve wildcards on load Resolve as many wildcards as possible on config load. Now that the overrides have moved back into the config loading stage the overridden fields are not having their wildcards resolved for some reason. It is useful to do this anyway as we then have more resolved config values in the saved project config Signed-off-by: James McCorrie --- src/dvsim/project.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/dvsim/project.py b/src/dvsim/project.py index c626f5a0..d340c728 100644 --- a/src/dvsim/project.py +++ b/src/dvsim/project.py @@ -20,6 +20,7 @@ rm_path, run_cmd_with_timeout, ) +from dvsim.utils.wildcards import find_and_substitute_wildcards __all__ = ("Project",) @@ -216,12 +217,21 @@ def _load_flow_config( if "cfgs" in cfg: # add any missing rel_path fields - for child_cfg in cfg["cfgs"].values(): + for child_cfg_path in cfg["cfgs"]: + child_cfg = cfg["cfgs"][child_cfg_path] + if "rel_path" not in child_cfg: child_cfg["rel_path"] = child_cfg["self_dir"].relative_to( root_path, ) + # resolve as many wildcards as possible at this stage + cfg["cfgs"][child_cfg_path] = find_and_substitute_wildcards( + obj=child_cfg, + wildcard_values=child_cfg, + ignore_error=True, + ) + return TopFlowConfig.model_validate(cfg) return FlowConfig.model_validate(cfg)