Skip to content

[SYCL][E2E] Add option to build tests on run-only mode if marked as REQUIRES: build-and-run-mode #16306

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

Merged
againull merged 12 commits into from
Dec 19, 2024
Merged

[SYCL][E2E] Add option to build tests on run-only mode if marked as REQUIRES: build-and-run-mode #16306

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a61495f
Build tests on run-only if marked as exceptions
ayylol Dec 9, 2024
ca76a74
Change whitespace
ayylol Dec 9, 2024
ad80c2e
Add option to build over skip in run-only
ayylol Dec 13, 2024
09db374
Merge branch 'sycl' into run-unsplit
ayylol Dec 13, 2024
fbbbd8e
Remove unsplit name
ayylol Dec 13, 2024
d30d2a5
Format changes
ayylol Dec 13, 2024
1b79472
Move setting ignore-line-filtering to when it is used
ayylol Dec 16, 2024
3f80c04
Move + format ignore_line_filtering
ayylol Dec 16, 2024
5d8ce83
Move usage of ignore_line_fitlering
ayylol Dec 18, 2024
7c98bf7
Remove redundant `run-only` mode check
ayylol Dec 18, 2024
caaeaf1
Format
ayylol Dec 18, 2024
0d4d13c
Change fallback option name
ayylol Dec 18, 2024
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
8 changes: 6 additions & 2 deletions sycl/test-e2e/format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,14 @@ def execute(self, test, litConfig):
if isinstance(script, lit.Test.Result):
return script

unsplit_test = False
Copy link
Contributor

@aelovikov-intel aelovikov-intel Dec 13, 2024

Choose a reason for hiding this comment

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

I don't know why, but something in me has a really huge dislike towards "unsplit" anywhere in regard to this feature...

Copy link
Contributor Author

@ayylol ayylol Dec 13, 2024

Choose a reason for hiding this comment

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

Changed it to ignore_line_filtering, is that better?

if "build-and-run-mode" in test.requires:
unsplit_test = True

devices_for_test = []
triples = set()
if test.config.test_mode == "build-only":
if "build-and-run-mode" in test.requires or "true" in test.unsupported:
if unsplit_test or "true" in test.unsupported:
return lit.Test.Result(
lit.Test.UNSUPPORTED, "Test unsupported for this environment"
)
Expand Down Expand Up @@ -232,7 +236,7 @@ def get_extra_env(sycl_devices):
continue

# Filter commands based on split-mode
is_run_line = any(
is_run_line = unsplit_test or any(
Copy link
Contributor

@sarnex sarnex Dec 10, 2024

Choose a reason for hiding this comment

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

sorry so is this saying we still will report it as UNSUPPORTED by try to compile it anyway?

Copy link
Contributor Author

@ayylol ayylol Dec 10, 2024

Choose a reason for hiding this comment

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

on run-only It will no longer be marked as unsupported. Here what happens is ignore the line filtering that happens in that mode so we end up building and running the test.

Copy link
Contributor

@sarnex sarnex Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

sorry, i must be missing it. where is the code that would have marked it unsupported before but now passes? the line changed here on 164 seems like it would have not been hit ever because of the build-only check

Copy link
Contributor Author

@ayylol ayylol Dec 10, 2024

Choose a reason for hiding this comment

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

So in build-only mode I needed to explicitly check for it to mark as unsupported because other features are not taken into account, for run-only mode it was originally marked as unsupported because the mode did not have the build-and-run-mode feature. I'm adding that feature in the lit.cfg.py so now those tests would run in run-only mode

Copy link
Contributor

@sarnex sarnex Dec 11, 2024

Choose a reason for hiding this comment

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

ah i see thx

i in directive.command
for i in ["%{run}", "%{run-unfiltered-devices}", "%if run-mode"]
)
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/lit.cfg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
elif config.test_mode == "run-only":
lit_config.note("run-only test mode enabled, only executing tests")
config.available_features.add("run-mode")
config.available_features.add("build-and-run-mode")
Copy link
Contributor

@sarnex sarnex Dec 10, 2024

Choose a reason for hiding this comment

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

just looking at the if-else, adding this seems weird. is there another way to achieve the result while keeping the features accurate?

Copy link
Contributor Author

@ayylol ayylol Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, I had my doubts about doing it this way. I think we have three options here not too sure what would be preferred.

1 - Introduce a testing-mode that does what this pr is making run-only do, and keep run-only unchanged
2 - Change the name of run-only to reflect the fact that it can also run tests marked as exceptions by falling back to building and running
3 - remove the build-and-run-mode feature and mark exceptions with REQUIRES: run-mode instead.

Personally I like option 3 the best but didn't want to jump to it right away without some discussion since it would require me to change the usage of build-and-run-mode in all the tests.

Copy link
Contributor

@sarnex sarnex Dec 11, 2024

Choose a reason for hiding this comment

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

so about option 3, does that mean we always execute build for run-only tests? is the list of tests that will start being compiled be different than if this PR is merged as is?

Copy link
Contributor Author

@ayylol ayylol Dec 11, 2024

Choose a reason for hiding this comment

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

With this pr as is we will compile tests that are currently marked with REQUIRES: build-and-run-mode when in run-only mode. What option 3 changes would be the feature that we would use to mark the exceptions, so instead we would use REQUIRES: run-mode, which both run-only and full testing modes already have.

The list of tests that are marked as splitting exceptions would not change, we would just need to update how they are marked.

Also should point out the motivation for this change is so that if we are replacing one of the ci jobs that tests in full mode with split build and run, that we retain the same test coverage on the run-only side. (Working on these changes here #16321)

Copy link
Contributor

@sarnex sarnex Dec 11, 2024

Choose a reason for hiding this comment

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

i prefer option 3, not sure about @aelovikov-intel or @uditagarwal97

Copy link
Contributor

@aelovikov-intel aelovikov-intel Dec 12, 2024

Choose a reason for hiding this comment

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

IMO, don't fix this, at least don't do any long-term fixes. The reason we have REQUIRES: build-and-run-mode is because we started with the simplest possible implementation of this feature and to avoid any hacks/smartness. What we really need is to provide some generic enough logic to avoid these exceptions altogether.

The priority should be switching gen12 precommit to this new mode first and asap. If we have to exclude 50 or so tests from it, let it be. 12vs40 mins justify that. We'll keep full coverage in a) post-commit/nightly, b) once e2e on PVC is re-enabled (someone is working on this, right?).

Now, if we want to enable something like this for the very short-term, let it be, and it doesn't matter much how exactly because it would be temporary anyway. I'd personally introduce a separate option like --param prefer_build_in_run_mode_over_skip=True to do that.

Copy link
Contributor

@sarnex sarnex Dec 12, 2024

Choose a reason for hiding this comment

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

thats also fine with me

Copy link
Contributor Author

@ayylol ayylol Dec 13, 2024

Choose a reason for hiding this comment

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

Ok i've enabled this through a parameter instead.

elif config.test_mode == "build-only":
lit_config.note("build-only test mode enabled, only compiling tests")
config.sycl_devices = []
Expand Down
Loading