-
Notifications
You must be signed in to change notification settings - Fork 790
[UR][Benchmarks] Support for Unitrace for all benchmarks #19320
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: sycl
Are you sure you want to change the base?
Conversation
Signed-off-by: Mateusz P. Nowak <[email protected]>
Signed-off-by: Mateusz P. Nowak <[email protected]>
Signed-off-by: Mateusz P. Nowak <[email protected]>
"-DBUILD_WITH_XPTI=1", | ||
"-DBUILD_WITH_MPI=0", | ||
], | ||
ld_library=get_oneapi().ld_libraries() + [f"{options.sycl}/lib"], |
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?
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.
🤔 removed
@@ -0,0 +1,182 @@ | |||
# Copyright (C) 2024-2025 Intel Corporation |
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.
2025
print(f"Moved {pid_json_files[0]} to {dst}") | ||
|
||
# Prune old unitrace directories | ||
prune_unitrace_dirs(options.unitrace_res_dir, FILECNT=5) |
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.
You use this only here. Why bother with defining a default value of an argument that you are going to override anyway?
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.
Debugging leftover, removed
|
||
shutil.move(os.path.join(options.benchmark_cwd, pid_json_files[0]), dst) | ||
if options.verbose: | ||
print(f"Moved {pid_json_files[0]} to {dst}") |
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.
all the logs will need to be changed after #19158 is merged.
return bench_dir, unitrace_output, unitrace_command | ||
|
||
|
||
def handle_unitrace_output(bench_dir, unitrace_output, timestamp): |
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 feel like this is all very complicated. Maybe we can use directory structure to help us?
Let's say we have two benchmarks: "SubmitKernel" and "Hashtable". We can organize the unitrace output such that each trace file is in its own directory:
.../results/traces/SubmitKernel/YYYYMMDD_HHMMSS_[name].PID.out
.../results/traces/Hashtable/YYYYMMDD_HHMMSS_[name].PID.out
Removing old results would become simply removing all but 5 newest files in a directory. We wouldn't have to find, rename and move files, since we could make unitrace output them to the destination place upfront.
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.
Done, various approaches are possible, no problem to follow yours.
devops/scripts/benchmarks/main.py
Outdated
parser.add_argument( | ||
"--unitrace", | ||
action="store_true", | ||
help="Unitrace tracing for sigle iteration of benchmarks", | ||
) | ||
|
||
parser.add_argument( | ||
"--unitrace-inclusive", | ||
action="store_true", | ||
help="Regular run of benchmarks iterations and unitrace tracing in single additional run", | ||
) |
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.
please do --unitrace inclusive
and --unitrace
as a single option, not two separate ones. See how --output-html
is done.
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.
Done
@@ -224,6 +224,71 @@ def parse_unit_type(compute_unit): | |||
|
|||
|
|||
class ComputeBenchmark(Benchmark): | |||
|
|||
# list of benchmarks to exclude from unitrace due to SIGSEGV, SIGABRT or timeouts | |||
unitrace_exclusion_list = [ |
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.
We should just let things fail. If unitrace is failing, it's an urgent bug that needs to be fixed.
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.
Well, ok.
Signed-off-by: Mateusz P. Nowak <[email protected]>
print(f"Renamed {src} to {dst}") | ||
break | ||
|
||
# Handle {name}.{pid}.json files in cwd: move and rename to {self.name()}_{timestamp}.json |
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.
Doesn't unitrace
already have an option for changing the default name of jsons and .pid
logs?
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.
unitrace produces two kinds of output files - traces, configurable with --output (pid is added anyway), and jsons for visualization in Perfetto UI, created with no control over their names - just ..json in cwd
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.
Bummer. Maybe we could change unitrace to use the output name for both outputs. The visualization json could be something like name.pid.vis.json
.
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.
One general comment - when under tracing, we should reduce the number of iterations etc, to shorten the duration of the benchmark.
If we really need to disable a benchmark (because it's difficult to make it sufficiently short for example), the Benchmark class should have something like traceable()
to filter out which benchmarks should / shouldn't be traced.
Added in Benchmark, and a stub in ComputeBenchmark |
Signed-off-by: Mateusz P. Nowak <[email protected]>
Signed-off-by: Mateusz P. Nowak <[email protected]>
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.
Pull Request Overview
This PR adds support for capturing Unitrace logs alongside benchmark runs.
- Introduces a new
Unitrace
utility to clone, build, and manage Unitrace traces - Extends CLI (
--unitrace
) andoptions.py
to configure trace capture - Updates benchmark runner (
run_bench
,main.py
, and individual benches) and history saving to integrate Unitrace outputs
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
devops/scripts/benchmarks/utils/unitrace.py | Add Unitrace tool clone/build, setup/cleanup, and output handling |
devops/scripts/benchmarks/options.py | Add save_name option and change build_jobs calculation |
devops/scripts/benchmarks/main.py | Introduce --unitrace flag, create/get instance, and update run flow |
devops/scripts/benchmarks/history.py | Incorporate Unitrace timestamp when saving run history |
devops/scripts/benchmarks/benches/base.py | Extend run_bench and signature of run to pass run_unitrace flag |
devops/scripts/benchmarks/benches/* | Update all benchmark run methods to accept run_unitrace and forward flag |
Comments suppressed due to low confidence (4)
devops/scripts/benchmarks/main.py:550
- [nitpick] The
--unitrace
flag overload with optional values is confusing; consider defining separate flags (e.g.,--unitrace
and--unitrace-inclusive
) for clearer CLI design.
choices=["inclusive", True],
devops/scripts/benchmarks/utils/unitrace.py:66
- [nitpick] The parameter
FILECNT
uses uppercase naming that deviates from snake_case; consider renaming tofile_count
ormax_files
for consistency.
def _prune_unitrace_dirs(self, dir: str, FILECNT: int = 10):
devops/scripts/benchmarks/utils/unitrace.py:14
- [nitpick] Public
Unitrace
class and its methods lack docstrings; consider adding docstrings to explain their purpose, parameters, and behavior.
class Unitrace:
devops/scripts/benchmarks/utils/unitrace.py:1
- This new
unitrace.py
module introduces complex clone/build/cleanup logic but no tests; consider adding unit tests or integration tests to verify its behavior.
# Copyright (C) 2025 Intel Corporation
@@ -68,7 +70,7 @@ class Options: | |||
build_igc: bool = False | |||
current_run_name: str = "This PR" | |||
preset: str = "Full" | |||
build_jobs: int = multiprocessing.cpu_count() | |||
build_jobs: int = len(os.sched_getaffinity(0)) # Cores available for the process. |
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.
Using os.sched_getaffinity may not be supported on non-Linux platforms; consider adding a fallback (e.g., os.cpu_count()) for portability.
build_jobs: int = len(os.sched_getaffinity(0)) # Cores available for the process. | |
try: | |
build_jobs: int = len(os.sched_getaffinity(0)) # Cores available for the process. | |
except (AttributeError, NotImplementedError): | |
build_jobs: int = os.cpu_count() # Fallback for non-Linux platforms. |
Copilot uses AI. Check for mistakes.
def _prune_unitrace_dirs(self, dir: str, FILECNT: int = 10): | ||
files = os.listdir(dir) | ||
files.sort() # Lexicographical sort matches timestamp order | ||
if len(files) > 2 * FILECNT: | ||
for f in files[: len(files) - 2 * FILECNT]: | ||
full_path = os.path.join(dir, f) |
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.
[nitpick] Parameter name dir
shadows the built-in function dir
; rename it (e.g., dir_path
) to avoid confusion.
def _prune_unitrace_dirs(self, dir: str, FILECNT: int = 10): | |
files = os.listdir(dir) | |
files.sort() # Lexicographical sort matches timestamp order | |
if len(files) > 2 * FILECNT: | |
for f in files[: len(files) - 2 * FILECNT]: | |
full_path = os.path.join(dir, f) | |
def _prune_unitrace_dirs(self, dir_path: str, FILECNT: int = 10): | |
files = os.listdir(dir_path) | |
files.sort() # Lexicographical sort matches timestamp order | |
if len(files) > 2 * FILECNT: | |
for f in files[: len(files) - 2 * FILECNT]: | |
full_path = os.path.join(dir_path, f) |
Copilot uses AI. Check for mistakes.
Run main.py script with --unitrace to get unitrace logs for every benchmark.
Use --unitrace-inclusive to get benchmark results and unitrace logs.