-
Notifications
You must be signed in to change notification settings - Fork 794
[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 21 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 |
|---|---|---|
|
|
@@ -50,6 +50,14 @@ jobs: | |
| target_devices: ext_oneapi_hip:gpu | ||
| tests_selector: e2e | ||
|
|
||
| - name: Intel Build Tests Only | ||
ayylol marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| runner: '["Linux", "build"]' | ||
| image: ghcr.io/intel/llvm/ubuntu2204_intel_drivers:alldeps | ||
| image_options: -u 1001 --privileged --cap-add SYS_ADMIN | ||
| target_devices: opencl:cpu | ||
|
||
| tests_selector: e2e | ||
| extra_lit_opts: --param split-mode=build | ||
|
|
||
| - name: Intel L0 GPU | ||
| runner: '["Linux", "gen12"]' | ||
|
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 other e2e runs aren't using the tests build from the previous step yet right? if it does i dont see how that's happening :) 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, this pr does not include any of the running side of the split mode. Basically what we would have to do is make this job act similar to how the job for building the compiler works by spitting out an artifact which would include the built tests, and then on the running side we would take that artifact and run the tests with 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. got it, probably udit/myself/andrei can help with that if you are stuck |
||
| image: ghcr.io/intel/llvm/ubuntu2204_intel_drivers:latest | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,6 +98,53 @@ def getMatchedFromList(self, features, alist): | |
| except ValueError as e: | ||
| raise ValueError("Error in UNSUPPORTED list:\n%s" % str(e)) | ||
|
|
||
| def make_default_features_list(self, expr, triple, add_default=True): | ||
ayylol marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| # Dictionary of features which we know are always/never present for a | ||
| # given triple (or the system in general). | ||
ayylol marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| exceptions = {} | ||
| exceptions["spir64"] = { | ||
| "cuda": False, | ||
| "hip": False, | ||
| "hip_amd": False, | ||
| "hip_nvidia": False, | ||
| "native_cpu": False, | ||
| } | ||
| exceptions["system"] = { | ||
| "linux": True, | ||
| "windows": False, | ||
| "system-windows": False, | ||
| "run-mode": False, | ||
| "TEMPORARY_DISABLED": False, | ||
| } | ||
| features_queried_by_test = [] | ||
| for f in expr: | ||
| features_queried_by_test = features_queried_by_test + re.findall( | ||
| "[-+=._a-zA-Z0-9]+", f | ||
| ) | ||
| features = set() | ||
| for f in features_queried_by_test: | ||
| if exceptions[triple].get(f, exceptions["system"].get(f, add_default)): | ||
| features.add(f) | ||
| return features | ||
|
|
||
| def select_triples_for_test(self, test): | ||
| # Check Triples | ||
| triples = set() | ||
| possible_triples = ["spir64"] | ||
| for triple in possible_triples: | ||
| unsupported = self.make_default_features_list( | ||
| test.unsupported, triple, False | ||
| ) | ||
| required = self.make_default_features_list(test.requires, triple) | ||
| features = unsupported.union(required) | ||
| if test.getMissingRequiredFeaturesFromList(features): | ||
| continue | ||
| if self.getMatchedFromList(features, test.unsupported): | ||
| continue | ||
| triples.add(triple) | ||
|
|
||
| return triples | ||
|
|
||
| def select_devices_for_test(self, test): | ||
| devices = [] | ||
| for d in test.config.sycl_devices: | ||
|
|
@@ -154,18 +201,26 @@ 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)) | ||
| 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 |
||
| triples = self.select_triples_for_test(test) | ||
| if not triples: | ||
| return lit.Test.Result( | ||
| lit.Test.UNSUPPORTED, "No supported backend to build for" | ||
| ) | ||
| 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 +278,17 @@ def get_extra_env(sycl_devices): | |
| new_script.append(directive) | ||
| continue | ||
|
|
||
| # Filter commands based on split-mode | ||
| is_run_line = 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 +344,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?