[SYCL][E2E] Add option to build tests on run-only mode if marked as REQUIRES: build-and-run-mode
#16306
Conversation
sycl/test-e2e/format.py
Outdated
| @@ -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( | |||
There was a problem hiding this comment.
sorry so is this saying we still will report it as UNSUPPORTED by try to compile it anyway?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
sycl/test-e2e/lit.cfg.py
Outdated
| @@ -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") | |||
There was a problem hiding this comment.
just looking at the if-else, adding this seems weird. is there another way to achieve the result while keeping the features accurate?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
i prefer option 3, not sure about @aelovikov-intel or @uditagarwal97
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok i've enabled this through a parameter instead.
ayylol
changed the title
REQUIRES: build-and-run-modeREQUIRES: build-and-run-mode
sycl/test-e2e/format.py
Outdated
| @@ -154,10 +154,14 @@ def execute(self, test, litConfig): | |||
| if isinstance(script, lit.Test.Result): | |||
| return script | |||
|
|
|||
| unsplit_test = False | |||
There was a problem hiding this comment.
I don't know why, but something in me has a really huge dislike towards "unsplit" anywhere in regard to this feature...
There was a problem hiding this comment.
Changed it to ignore_line_filtering, is that better?
sycl/test-e2e/format.py
Outdated
| if ( | ||
| "build-and-run-mode" in test.requires | ||
| and test.config.fallback_build_run_only | ||
| ): | ||
| ignore_line_filtering = True |
There was a problem hiding this comment.
Why cannot that be done at the place when ignore_line_filtering will be used?
There was a problem hiding this comment.
Ok, i put this right outside of the loop where ignore_line_filtering is used.
|
LGTM, but give @sarnex and @uditagarwal97 some time for a chance to review this before merging as well. |
|
@intel/llvm-gatekeepers This one is ready to merge, thanks. |
Prior if a test was marked as
REQUIRES: build-and-run-modeit would be reported as unsupported inrun-onlymode, because there would be no test binaries created frombuild-onlymode to be able to run the test.When calling lit with
--param test-mode=run-only --param fallback-to-build-if-requires-build-and-run=True, tests that are marked withREQUIRES: build-and-run-modewill override theRUN:line filtering, and run all lines, including those that compile the test binaries. This makes it so we still have coverage of tests marked as exceptions for splitting on therun-onlysystem.