-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL][E2E] Add functionality to split build and run of e2e tests #15728
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
Changes from all commits
6b70797
08297d6
b679fbf
7f7d56b
37df022
c8b0eb6
53592cf
c8588fa
a61cb32
2eac94e
bdfd111
861f17a
e8ece44
92522fd
bd1fd50
07c3c40
ee764e2
b902185
563c765
5f267ec
201712b
e19c435
22b7583
5a8d4e4
96fe4e3
146afa2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,18 +154,31 @@ def execute(self, test, litConfig): | |
if isinstance(script, lit.Test.Result): | ||
return script | ||
|
||
devices_for_test = self.select_devices_for_test(test) | ||
if not devices_for_test: | ||
return lit.Test.Result( | ||
lit.Test.UNSUPPORTED, "No supported devices to run the test on" | ||
) | ||
|
||
substitutions = lit.TestRunner.getDefaultSubstitutions(test, tmpDir, tmpBase) | ||
devices_for_test = [] | ||
triples = set() | ||
for sycl_device in devices_for_test: | ||
(backend, _) = sycl_device.split(":") | ||
triples.add(get_triple(test, backend)) | ||
unsplit_test = False | ||
for l in test.requires: | ||
if "run-mode" in re.findall("[-+=._a-zA-Z0-9]+", l): | ||
unsplit_test = True | ||
break | ||
if "run-mode" not in test.config.available_features: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe swap the logic so the first check isnt a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue with doing this is that the non-split mode sets both run-mode and build-mode features. what i am doing here is specifically checking for the split build only mode. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah ok thx There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't looked through the PR yet, but from previous discussions, I'd expect that we might build more tests/for more triples that are otherwise skipped in the old mode due to XFAIL/UNSUPPORTED. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you mean for tests that compile fine but fail/unsupported at runtime, we always compile those but don't run the test? yeah that sounds cool |
||
if unsplit_test: | ||
return lit.Test.Result( | ||
lit.Test.UNSUPPORTED, "Tests unsupported on split build" | ||
) | ||
triples = {"spir64"} | ||
else: | ||
devices_for_test = self.select_devices_for_test(test) | ||
if not devices_for_test: | ||
return lit.Test.Result( | ||
lit.Test.UNSUPPORTED, "No supported devices to run the test on" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. have you tried building on one system and running on a different system yet? have you have any issues with different paths to python or something like that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, i ran some experiments to run the split side on a separate machine after running the build mode on another. I did not run into issues relating to python path, mostly issues related to the prs i've opened, and AOT tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah ok cool, i expected this part might have insane nonsense to work through but im glad it seems not too bad |
||
) | ||
|
||
for sycl_device in devices_for_test: | ||
(backend, _) = sycl_device.split(":") | ||
triples.add(get_triple(test, backend)) | ||
|
||
substitutions = lit.TestRunner.getDefaultSubstitutions(test, tmpDir, tmpBase) | ||
substitutions.append(("%{sycl_triple}", format(",".join(triples)))) | ||
# -fsycl-targets is needed for CUDA/HIP, so just use it be default so | ||
# -that new tests by default would runnable there (unless they have | ||
|
@@ -223,6 +236,17 @@ def get_extra_env(sycl_devices): | |
new_script.append(directive) | ||
continue | ||
|
||
# Filter commands based on split-mode | ||
is_run_line = unsplit_test or any( | ||
i in directive.command | ||
for i in ["%{run}", "%{run-unfiltered-devices}", "%if run-mode"] | ||
) | ||
|
||
if (is_run_line and "run-mode" not in test.config.available_features) or ( | ||
not is_run_line and "build-mode" not in test.config.available_features | ||
): | ||
directive.command = "" | ||
|
||
if "%{run}" not in directive.command: | ||
new_script.append(directive) | ||
continue | ||
|
@@ -278,7 +302,10 @@ def get_extra_env(sycl_devices): | |
test, litConfig, useExternalSh, script, tmpBase | ||
) | ||
|
||
if len(devices_for_test) > 1: | ||
if ( | ||
len(devices_for_test) > 1 | ||
or "run-mode" not in test.config.available_features | ||
): | ||
return result | ||
|
||
# Single device - might be an XFAIL. | ||
|
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 should go to a separate PR so that we don't run needless CI jobs when this PR is updated.
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 think this only affects the options available for the manual e2e run. I dont think i see it affecting the pre-commit checks being ran.
That being said do you know if there is a way to not run all the checks its doing due to being a sycl-devops-pr, and make it run the checks for a normal pr?