-
Notifications
You must be signed in to change notification settings - Fork 74
Adding the End to End flow #278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| from flow.utils.paths import BenchmarkPaths, simpoint_analysis_root | ||
| from flow.utils.util import CommandError, Util | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the TraceGenerator class definition was removed. Was there a specific reason for this change? The original idea behind the class was to make it easier to reuse in other scripts if needed.
| single = sub.add_parser("single", help="Generate a single STF trace window") | ||
| single.add_argument("binary", help="Path to workload binary") | ||
| single.add_argument("--emulator", required=True, choices=["spike", "qemu"]) | ||
| single.add_argument("--mode", choices=["macro", "insn_count", "pc_count"], default="macro") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were the mode subparsers removed? Using subparsers helped validate arguments more easily and also made --help output more readable.
| except CommandError as err: | ||
| Util.warn(f"stf_dump failed: {err}") | ||
| return | ||
| trace_path.with_suffix(".dump").write_text(dump_result.stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stf_dump tool part is repeated in the run_single method
| ) -> subprocess.CompletedProcess: | ||
| docker_cmd = self._docker_prefix(workdir, interactive) + ["bash", "-lc", shlex.join(command)] | ||
| Util.info("Docker exec: " + " ".join(docker_cmd)) | ||
| result = subprocess.run(docker_cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason the Docker Python library was removed? I found that using it made the code easier to read and maintain.
|
|
||
|
|
||
| # output generated by the end to end flow | ||
| outputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure if this change affects anything related to the Olympia setup. To be safer, I’d suggest ignoring only the outputs inside the docker stf trace gen folder instead of applying it more broadly.
| Util.warn("stf_count not available; skipping verification") | ||
| return {"verified": None, "counted": None} | ||
| try: | ||
| result = Util.run_cmd([str(count_tool), str(trace_path)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the stf_dump tool. This stf_count should probably be called using the Docker image rather than directly from the host.
|
@Jatin-exe Please provide an ETA to address the review comments. |
Apologies for the delay. I will be pushing changes by EOD. Thank you for your patience. |
Hello @Jatin-exe do you have an update? When do you expect to push all the changes needed for the review? |
Implemented:
To do: