Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix several tests on Windows. #11048

Merged
merged 2 commits into from
Nov 4, 2022
Merged

Fix several tests on Windows. #11048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5cc8cf6
Fix several tests on Windows.
ScottTodd Nov 3, 2022
7f68f60
Use PurePath instead of Path in a few places.
ScottTodd Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions build_tools/benchmarks/collect_compilation_statistics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,14 @@ def match_module_cmake_target(module_path: str) -> Optional[str]:
return None
if os.path.splitext(path_parts[3])[1] != MODULE_FILE_EXTENSION:
return None
return os.path.join(*path_parts)
# Join to get the CMake target name. This is *not* a filesystem path, so we
# don't want \ separators on Windows that we would get with os.path.join().
return '/'.join(path_parts)


def parse_compilation_time_from_ninja_log(log: TextIO) -> Dict[str, int]:
"""Retrieve the compilation time (ms) from the Ninja build log.
Returns:
Map of target name and compilation time in ms.
"""
Expand Down
40 changes: 22 additions & 18 deletions build_tools/benchmarks/common/benchmark_config_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,17 @@ class BenchmarkConfigTest(unittest.TestCase):
def setUp(self):
self.build_dir = tempfile.TemporaryDirectory()
self.tmp_dir = tempfile.TemporaryDirectory()
self.normal_tool_dir = os.path.join(self.build_dir.name, "normal_tool")
self.normal_tool_dir = os.path.realpath(
os.path.join(self.build_dir.name, "normal_tool"))
os.mkdir(self.normal_tool_dir)
self.traced_tool_dir = os.path.join(self.build_dir.name, "traced_tool")
self.traced_tool_dir = os.path.realpath(
os.path.join(self.build_dir.name, "traced_tool"))
os.mkdir(self.traced_tool_dir)
self.trace_capture_tool = tempfile.NamedTemporaryFile()
os.chmod(self.trace_capture_tool.name, stat.S_IEXEC)

def tearDown(self):
self.trace_capture_tool.close()
self.tmp_dir.cleanup()
self.build_dir.cleanup()

Expand All @@ -43,26 +46,27 @@ def test_build_from_args(self):

config = BenchmarkConfig.build_from_args(args=args, git_commit_hash="abcd")

per_commit_tmp_dir = os.path.join(self.tmp_dir.name, "abcd")
per_commit_tmp_dir = os.path.realpath(
os.path.join(self.tmp_dir.name, "abcd"))
expected_trace_capture_config = TraceCaptureConfig(
traced_benchmark_tool_dir=self.traced_tool_dir,
trace_capture_tool=self.trace_capture_tool.name,
trace_capture_tool=os.path.realpath(self.trace_capture_tool.name),
capture_tarball=os.path.realpath("capture.tar"),
capture_tmp_dir=os.path.join(per_commit_tmp_dir, "captures"))
self.assertEqual(
config,
BenchmarkConfig(root_benchmark_dir=os.path.join(self.build_dir.name,
"benchmark_suites"),
benchmark_results_dir=os.path.join(
per_commit_tmp_dir, "benchmark-results"),
Comment on lines -52 to -57
Copy link
Member Author

@ScottTodd ScottTodd Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This test got messy to update, since BenchmarkConfig calls os.path.realpath in many places.

This led to failures when comparing paths like

'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\tmpm6qhk0oy\\benchmark_suites'
'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmpm6qhk0oy\\benchmark_suites'

Could we just add a way to "assert equal, after canonicalizing all paths"?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we stored pathlib.Path objects instead of strings, the comparisons should be safer too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with the current solution. We should refactor the tool to take pathlib.Path instead eventually. Can you file a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

#11055

git_commit_hash="abcd",
normal_benchmark_tool_dir=self.normal_tool_dir,
trace_capture_config=expected_trace_capture_config,
driver_filter="a",
model_name_filter="b",
mode_filter="c",
keep_going=True,
benchmark_min_time=10))
expected_config = BenchmarkConfig(
root_benchmark_dir=os.path.realpath(
os.path.join(self.build_dir.name, "benchmark_suites")),
benchmark_results_dir=os.path.realpath(
os.path.join(per_commit_tmp_dir, "benchmark-results")),
git_commit_hash="abcd",
normal_benchmark_tool_dir=self.normal_tool_dir,
trace_capture_config=expected_trace_capture_config,
driver_filter="a",
model_name_filter="b",
mode_filter="c",
keep_going=True,
benchmark_min_time=10)
self.assertEqual(config, expected_config)

def test_build_from_args_benchmark_only(self):
args = build_common_argument_parser().parse_args([
Expand Down
4 changes: 2 additions & 2 deletions build_tools/benchmarks/common/common_arguments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ def check_exe_path(path):
parser.add_argument(
"--normal_benchmark_tool_dir",
"--normal-benchmark-tool-dir",
type=check_exe_path,
type=check_dir_path,
default=None,
help="Path to the normal (non-tracing) iree tool directory")
parser.add_argument("--traced_benchmark_tool_dir",
"--traced-benchmark-tool-dir",
type=check_exe_path,
type=check_dir_path,
default=None,
help="Path to the tracing-enabled iree tool directory")
parser.add_argument("--trace_capture_tool",
Expand Down
14 changes: 9 additions & 5 deletions build_tools/benchmarks/common/common_arguments_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,22 @@
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

import unittest
import shutil
import tempfile

from common.common_arguments import build_common_argument_parser


class CommonArgumentsTest(unittest.TestCase):

def test_build_common_argument_parser(self):
arg_parser = build_common_argument_parser()
arg_parser.parse_args([
"--normal_benchmark_tool_dir=/tmp", "--traced_benchmark_tool_dir=/tmp",
"--trace_capture_tool=/bin/ls", "."
])
with tempfile.TemporaryDirectory() as tempdir:
arg_parser = build_common_argument_parser()
arg_parser.parse_args([
"--normal_benchmark_tool_dir=" + tempdir,
"--traced_benchmark_tool_dir=" + tempdir,
"--trace_capture_tool=" + shutil.which("ls"), "."
])

def test_build_common_argument_parser_check_build_dir(self):
arg_parser = build_common_argument_parser()
Expand Down
6 changes: 4 additions & 2 deletions build_tools/python/e2e_test_artifacts/cmake_generator/iree_rule_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,10 @@ def build_model_import_rule(

model = imported_model.model
if model.source_type == common_definitions.ModelSourceType.EXPORTED_LINALG_MLIR:
if source_model_rule.file_path != str(output_file_path):
raise ValueError("Separate path for Linalg model isn't supported.")
if pathlib.PurePath(source_model_rule.file_path) != output_file_path:
raise ValueError("Separate path for Linalg model isn't supported ('" +
source_model_rule.file_path + "' != '" +
str(output_file_path) + "')")
return IreeModelImportRule(target_name=source_model_rule.target_name,
output_file_path=str(output_file_path),
cmake_rules=[])
Expand Down
3 changes: 2 additions & 1 deletion build_tools/python/e2e_test_artifacts/cmake_generator/iree_rule_generator_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ def test_build_model_import_rule_linalg(self):
output_file_path=pathlib.PurePath(model_rule.file_path))

self.assertEqual(rule.target_name, model_rule.target_name)
self.assertEqual(rule.output_file_path, model_rule.file_path)
self.assertEqual(pathlib.PurePath(rule.output_file_path),
pathlib.PurePath(model_rule.file_path))

def test_build_module_compile_rule(self):
model_import_rule = iree_rule_generator.IreeModelImportRule(
Expand Down
3 changes: 3 additions & 0 deletions runtime/src/iree/base/internal/wait_handle_win32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,9 @@ iree_status_t iree_wait_any(iree_wait_set_t* set, iree_time_t deadline_ns,

iree_status_t iree_wait_one(iree_wait_handle_t* handle,
iree_time_t deadline_ns) {
if (handle->type == IREE_WAIT_PRIMITIVE_TYPE_NONE) {
return iree_ok_status();
}
IREE_TRACE_ZONE_BEGIN(z0);

// Remap absolute timeout to relative timeout, handling special values as
Expand Down