Cycles render tests: Add the option to test with OSL enabled #124601

Alaska · 2024-07-12T14:01:34+02:00

Alaska commented

2024-07-12 14:01:34 +02:00

This commit adds a new cmake variable WITH_CYCLES_TEST_OSL
that runs every Cycles test a second time with OSL enabled.

At the moment only CPU OSL is enabled. There are plans to enable
OptiX OSL in the future when stability issues with OptiX OSL
have been resolved.

Some render tests have been blocked from running until we can figure
out a fix. The most notiable being all the Pricincipled BSDF tests
as some of them are failing due to noise differences.

TODO/Questions:

Enable by default on build bot?
- Currently enabled
Enable OSL tests by default for everyone?
- Risks OSL being the reference image when a developer updates tests with BLENDER_TEST_UPDATE=1 ctest..., when ideally SVM should be the reference. But this could be resolved by scheduling OSL tests first, then SVM second. And it shouldn't matter as OSL and SVM should match.
- Sergey recommends leaving off by default.
Create a proper blocklist for failing tests after investigating why each test fails, and fixing them when possible.
- Alaska has already investigated a lot of cases and submitted pull requests for the ones they could fix.
- There are some more complex cases (noise in the Principled BSDF tests) that needs a deeper investigation. These tests can be temporarily disabled and an investigation and fix can be submitted later.
Test on build bot, see if anything is failing on other platforms
Backport to 4.2 LTS to allow for more thorough testing there? There are merge conflicts due to other changes like Use blocklist name for blocked tests and Enable MNEE tests on Metal. Should those also be backported? Or should I create a 4.2 version of just this PR? - Up to Philipp to decide
General code review

Ref #123012

This commit adds a new cmake variable `WITH_CYCLES_TEST_OSL` that runs every Cycles test a second time with OSL enabled. At the moment only CPU OSL is enabled. There are plans to enable OptiX OSL in the future when stability issues with OptiX OSL have been resolved. Some render tests have been blocked from running until we can figure out a fix. The most notiable being all the Pricincipled BSDF tests as some of them are failing due to noise differences. --- TODO/Questions: - [x] Enable by default on build bot? - Currently enabled - [x] Enable OSL tests by default for everyone? - Risks OSL being the reference image when a developer updates tests with `BLENDER_TEST_UPDATE=1 ctest...`, when ideally SVM should be the reference. But this could be resolved by scheduling OSL tests first, then SVM second. And it shouldn't matter as OSL and SVM should match. - Sergey recommends leaving off by default. - [x] Create a proper blocklist for failing tests after investigating why each test fails, and fixing them when possible. - Alaska has already investigated a lot of cases and submitted pull requests for the ones they could fix. - There are some more complex cases (noise in the Principled BSDF tests) that needs a deeper investigation. These tests can be temporarily disabled and an investigation and fix can be submitted later. - [x] Test on build bot, see if anything is failing on other platforms - [x] Backport to 4.2 LTS to allow for more thorough testing there? There are merge conflicts due to other changes like `Use blocklist name for blocked tests` and `Enable MNEE tests on Metal`. Should those also be backported? Or should I create a 4.2 version of just this PR? - Up to Philipp to decide - [x] General code review Ref #123012

Alaska added 1 commit 2024-07-12 14:01:36 +02:00

Add the option to test with OSL enabled in Cycles render tests 94de03142c

Alaska added this to the Render & Cycles project 2024-07-12 14:01:39 +02:00

Alaska referenced this pull request

2024-07-12 14:07:18 +02:00

Cycles: Improve the coverage of automated regression tests #123012

Alaska added 2 commits 2024-07-12 14:52:06 +02:00

Adjust OSL test name de0e72d129

Adjust how OSL argument is added f3a15ba8ff

Alaska added 3 commits 2024-07-12 15:53:41 +02:00

Adjust comment for CYCLES_TEST_OSL 884235c59e

Adjust naming for OSL tests 71a2cb6157

Start OPTIX_OSL blacklist c76c83c442

Alaska added 3 commits 2024-07-12 16:39:48 +02:00

Turn off OptiX OSL tests b7d76cb62c

Adjust blacklist_osl name 6aec2f5ab8

Add OSL blacklist 0673715e5b

Alaska force-pushed enable-osl-tests from 5d8f82c912 to f8a4d702d8

2024-07-13 07:22:18 +02:00

Compare

Alaska added 5 commits 2024-07-13 18:45:40 +02:00

Adjust how OSL tests are activated ea0a45e16b

Black list image loading/mip-mapping differences 7706d6f119

Add failing bump + displacement tests b64fc4d39e

Add comment about other failing tests c81fd06feb

Remove Bad UVs scene as a bug fix has been submitted 72d8c1a33b

Alaska added 3 commits 2024-07-15 06:00:29 +02:00

Only enable OSL test if the cmake parameter is set 1e8d15097c

Merge branch 'main' into enable-osl-tests 436678cadd

Adjust code comments 1a3c61fb87

Alaska added 1 commit 2024-07-15 07:16:18 +02:00

Add more tests to OptiX OSL blacklist 14b1eb55ec

Weizhen Huang referenced this pull request

2024-07-15 17:52:16 +02:00

Cycles: Improve the coverage of automated regression tests #123012

Alaska referenced this pull request from blender/blender-test-data

2024-07-17 06:07:24 +02:00

Add Cycles render tests for texture nodes #6

Alaska added 4 commits 2024-07-17 07:45:03 +02:00

Add more OptiX OSL blacklist tests 32a33e3bc6

Simplify image OSL blacklist 9739afe409

Merge branch 'main' into enable-osl-tests 76b175fe0b

Adjust names of some block lists be3732e466

Change to "Blocklist" on my new lists to conform with a upcoming change.
Change BLOCKLIST_OSL_TESTS to BLOCKLIST_EXPLICIT_OSL_TESTS

Alaska added 4 commits 2024-07-17 09:44:35 +02:00

Continue adjustment to block list f7a607ec5f

Add image mapping UDIM to block list 7ae7eb3daa

Add image alpha tests 6dde1800a8

Adjust comments about blocked tests 6fce54d61b

Alaska commented

2024-07-17 09:53:08 +02:00

@Sergey and @weizhen can I get your input on some things?

I would like to target supporting CPU and OptiX OSL in these tests, but more tests than expected need to be blocked in OptiX OSL due to issues like #122779.
- Would you like me to strive for OptiX OSL support, and just block every OptiX OSL test that's failing with comments explaining why? Or should I leave OptiX OSL disabled and we can enable OptiX OSL once those issues are fixed?
A few tests are failing due to noise differences and I'm not sure how to fix them. (See Failing noise tests image below)
- Every single one of these tests are failing because of noise differences with shader mixing/layering in the Principled BSDF in OSL. With the exception of blackbody which I assume is failing because OSL uses a LUT that's slightly different from the SVM implementation of the node.
  - How would you like these handled? Block them? Adjust noise thresholds? Something else?

Note: If you run this locally you will notice there are three other tests I haven't blocked that are failing. I have submitted a PR that fixes two of them: !124673

The other is the īmage_log_osl test. The īmage_log_osl test is expected to be different between OSL and SVM. So there are OSL and SVM variants of this test. The issue is that the OSL variant doesn't have OSL turned on, and so the test was wrong. This pull request just exposes that.

Has been fixed in main.

@Sergey and @weizhen can I get your input on some things? - I would like to target supporting CPU and OptiX OSL in these tests, but more tests than expected need to be blocked in OptiX OSL due to issues like #122779. - Would you like me to strive for OptiX OSL support, and just block every OptiX OSL test that's failing with comments explaining why? Or should I leave OptiX OSL disabled and we can enable OptiX OSL once those issues are fixed? - A few tests are failing due to noise differences and I'm not sure how to fix them. (See `Failing noise tests` image below) - Every single one of these tests are failing because of noise differences with shader mixing/layering in the Principled BSDF in OSL. With the exception of blackbody which I assume is failing because OSL uses a LUT that's slightly different from the SVM implementation of the node. - How would you like these handled? Block them? Adjust noise thresholds? Something else? Note: If you run this locally you will notice there are three other tests I haven't blocked that are failing. I have submitted a PR that fixes two of them: !124673 ~~The other is the `īmage_log_osl` test. The `īmage_log_osl` test is expected to be different between OSL and SVM. So there are OSL and SVM variants of this test. The issue is that the OSL variant doesn't have OSL turned on, and so the test was wrong. This pull request just exposes that.~~ Has been fixed in main.

Failing noise tests.jpg

1.6 MiB

Sergey Sharybin commented

2024-07-17 12:21:54 +02:00

I would suggest narrowing the scope of the first step. Surely ideally all permutations will be tested, especially on the CI/CD. Adding WITH_CYCLES_TEST_OSL disabled by default which will enable rendering all tests with OSL will be the good first step.

On the implementation level it is quite strange to have render-engine-specific code in a generic base class. More ideally would be to subclass render_report.Report in cycles_render_tests.py and implement all specific logic as method overrides.

Image tests will be too different between OSL and SVN due to the difference in image sampling. But the rest of the files is not immediately clear to me why they are so different.

I would suggest narrowing the scope of the first step. Surely ideally all permutations will be tested, especially on the CI/CD. Adding `WITH_CYCLES_TEST_OSL` disabled by default which will enable rendering all tests with OSL will be the good first step. On the implementation level it is quite strange to have render-engine-specific code in a generic base class. More ideally would be to subclass `render_report.Report` in `cycles_render_tests.py` and implement all specific logic as method overrides. Image tests will be too different between OSL and SVN due to the difference in image sampling. But the rest of the files is not immediately clear to me why they are so different.

Alaska added 2 commits 2024-07-17 14:17:17 +02:00

Switch to method override system b2774e21c6

Adjust blacklist to blocklist name that was missed previously 1104921007

Sergey Sharybin reviewed 2024-07-17 14:43:24 +02:00

tests/python/CMakeLists.txt Outdated

						
				@ -702,6 +702,22 @@ if(WITH_CYCLES OR WITH_GPU_RENDER_TESTS)

				            -blacklist ${_cycles_blacklist}

				          )

				          if(WITH_CYCLES_TEST_OSL AND WITH_CYCLES_OSL)

Sergey Sharybin commented

2024-07-17 14:43:04 +02:00

Will this create cycles_osl_osl test, since we already have osl folder?

Will this create `cycles_osl_osl` test, since we already have osl folder?

Alaska commented

2024-07-17 14:56:11 +02:00

You mean the commandline output from make test? It's cycles_osl_cpu_osl

You mean the commandline output from `make test`? It's `cycles_osl_cpu_osl`

Sergey Sharybin commented

2024-07-17 14:58:10 +02:00

Ah indeed. But is it just cycles_osl_cpu_osl, or do we have both cycles_osl_cpu_osl and cycles_osl_cpu with this change?

Ah indeed. But is it just `cycles_osl_cpu_osl`, or do we have both `cycles_osl_cpu_osl` and `cycles_osl_cpu` with this change?

Alaska commented

2024-07-17 15:00:16 +02:00

It creates both. Would you like me to change it so the tests in the OSL folder to only run if WITH_CYCLES_TEST_OSL is enabled?

It creates both. Would you like me to change it so the tests in the OSL folder to only run if `WITH_CYCLES_TEST_OSL` is enabled?

Sergey Sharybin commented

2024-07-17 15:31:49 +02:00

We could make it so:

If WITH_CYCLES_TEST_OSL=OFF then we add cycles_osl_cpu as we do it now (so nothing changes compared to the state prior to this PR, for neither test coverage, nor for people).
If WITH_CYCLES_TEST_OSL=ON then we add cycles_osl_cpu_osl (and potentially cycles_osl_optix_osl), so that we test everything supported on OSL, at a cost of potentially longer test run times).

Sounds a bit convoluted, but it avoids running essentially the same test twice, and also keeps it so the basics of OSL are always tested, even by people who don't necessarily want to test all device/shading models permutations.

How does this sound to you?

We could make it so: - If `WITH_CYCLES_TEST_OSL=OFF` then we add `cycles_osl_cpu` as we do it now (so nothing changes compared to the state prior to this PR, for neither test coverage, nor for people). - If `WITH_CYCLES_TEST_OSL=ON` then we add `cycles_osl_cpu_osl` (and potentially `cycles_osl_optix_osl`), so that we test everything supported on OSL, at a cost of potentially longer test run times). Sounds a bit convoluted, but it avoids running essentially the same test twice, and also keeps it so the basics of OSL are always tested, even by people who don't necessarily want to test all device/shading models permutations. How does this sound to you?

Alaska marked this conversation as resolved

Sergey Sharybin commented

2024-07-17 16:49:41 +02:00

For the buildbot-only configurations I've just landed #124892 which makes it much easier to tweak CMake options which we want to have on the buildbot but not on a local configurations.

👍 1

Alaska commented

2024-07-17 17:06:05 +02:00

So if we want OSL tests enabled by default on the build bot, I should just add set(WITH_CYCLES_TEST_OSL ON CACHE BOOL "" FORCE) to /build_files/cmake/config/blender_release.cmake?

And do you want this enabled by default on the build bot in this first iteration?

So if we want OSL tests enabled by default on the build bot, I should just add `set(WITH_CYCLES_TEST_OSL ON CACHE BOOL "" FORCE)` to `/build_files/cmake/config/blender_release.cmake`? And do you want this enabled by default on the build bot in this first iteration?

Sergey Sharybin commented

2024-07-17 17:13:55 +02:00

No, you add them to build_files/buildbot/config/blender_{liinux,macos,windows}.cmake. Similarly to the set(WITH_DOC_MANPAGE OFF CACHE BOOL "" FORCE).
The blender_release.cmake is what make release will use, and what buildbot is basing its configuration on, with ability override some things.

For enabling by default on buildbot -- it depends. If we can do things to pass quickly, then we might as well. One way forward with this would be to have somewhat longer block-list than it will be in ideal world.
If there are scenes where we need to have a more dedicated look then i'm totally open for having it as a disabled-for-now but make-it-easy-to-look-into option.

No, you add them to `build_files/buildbot/config/blender_{liinux,macos,windows}.cmake`. Similarly to the `set(WITH_DOC_MANPAGE OFF CACHE BOOL "" FORCE)`. The `blender_release.cmake` is what `make release` will use, and what buildbot is basing its configuration on, with ability override some things. For enabling by default on buildbot -- it depends. If we can do things to pass quickly, then we might as well. One way forward with this would be to have somewhat longer block-list than it will be in ideal world. If there are scenes where we need to have a more dedicated look then i'm totally open for having it as a disabled-for-now but make-it-easy-to-look-into option.

Alaska commented

2024-07-17 17:35:45 +02:00

I've limited OSL tests to one run throughout the entire test run.
I've enabled these tests on the build bot.
To make sure tests pass on the build bot, I've blocked every test that's failing with notes on why they were failing.
- This blocking is a bit "wide" (E.g. Using render_passes.*.blend and principled.*.blend in the block list when only a few tests that match that expression are failing). But it means people can interate in those areas and not be afraid of Cycles OSL suddenly reporting a failure.

Feel free to ask the build bot to run these tests. I've primarily been testing on ARM macOS and there may be more failing tests I missed on other platforms.

- I've limited OSL tests to one run throughout the entire test run. - I've enabled these tests on the build bot. - To make sure tests pass on the build bot, I've blocked every test that's failing with notes on why they were failing. - This blocking is a bit "wide" (E.g. Using `render_passes.*.blend` and `principled.*.blend` in the block list when only a few tests that match that expression are failing). But it means people can interate in those areas and not be afraid of Cycles OSL suddenly reporting a failure. Feel free to ask the build bot to run these tests. I've primarily been testing on ARM macOS and there may be more failing tests I missed on other platforms.

Alaska added 8 commits 2024-07-17 17:35:55 +02:00

Adjust code comment for render pass test fd6387ed25

Add TODO about enabling OptiX ee7e20374f

Adjust BLOCKLIST_EXPLICIT_OSL name a8e15ed3a5

Expand the details of the blocklist code comments 86a012d9d6

Only run OSL tests once 830ad1023d

Merge branch 'main' into enable-osl-tests 9fa324a6b0

Enable by default on build bot f8af8e8920

Block failing OSL tests to pass on build bot 779c2286a5

Alaska reviewed 2024-07-17 17:38:25 +02:00

Alaska added 1 commit 2024-07-17 17:43:37 +02:00

Fix formatting a8a42b4c5d

Alaska added 2 commits 2024-07-17 18:12:55 +02:00

Adjust block list for render_pass and principled 181adab55e

Adjust formatting in builtbot cmake file 663613b412

Alaska added 1 commit 2024-07-18 05:42:00 +02:00

Simplfy the 'single run of OSL per test' cmake 3ec72ad8c0

Alaska added 4 commits 2024-07-18 08:29:32 +02:00

Expand on code comment in _command_arguments b34f2052a1

Remove extra line ff1bc05f7c

Fix comment about blackbody. It's not the wavelength node 86e34471af

Merge branch 'main' into enable-osl-tests 045316b898

Alaska changed title from ~~WIP: Add the option to test with OSL enabled in Cycles render tests~~ to Add the option to test with OSL enabled in Cycles render tests

2024-07-18 18:10:52 +02:00

Alaska added 2 commits 2024-07-18 18:10:55 +02:00

Merge branch 'main' into enable-osl-tests 30086a5f8a

Fix issues setting test properties in some configurations 4fd1dadec4

Alaska requested review from Sergey Sharybin 2024-07-18 18:11:05 +02:00

Alaska requested review from Weizhen Huang 2024-07-18 18:11:06 +02:00

Alaska added 2 commits 2024-07-20 07:02:09 +02:00

Align new method with HWRT PR 3e66b14333

Merge branch 'main' into enable-osl-tests 0bd8995b42

Alaska changed title from ~~Add the option to test with OSL enabled in Cycles render tests~~ to Cycles render tests: Add the option to test with OSL enabled

2024-07-20 07:02:12 +02:00

Alaska added 2 commits 2024-07-24 14:18:58 +02:00

Merge branch 'main' into enable-osl-tests 21c513ce6c

Remove some tests from block list after the 124673 fix e37a7f2347

Alaska added 2 commits 2024-07-24 15:30:49 +02:00

Add set_and_warn for WITH_CYCLES_TEST_OSL 4beb266d84

Remove blackbody from blocklist and increase noise threshold instead

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

59f9943af8

Ray molenkamp commented

2024-07-24 15:37:04 +02:00

@blender-bot build

Alaska added 2 commits 2024-07-24 17:02:38 +02:00

Adjust threshold for OSL hair, and unify with other threshold code 4292774e81

Add vector_math to block list while a fix is worked on 99a74fe924

Alaska added 2 commits 2024-07-25 06:42:46 +02:00

Merge branch 'main' into enable-osl-tests 347f9a4be1

Clarify some code comments

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

2d5049d17e

Sergey Sharybin commented

2024-07-25 09:41:47 +02:00

@blender-bot build

Alaska added 2 commits 2024-07-26 12:24:03 +02:00

Merge branch 'main' into enable-osl-tests d9b52705bd

Remove vector math from block list 5ac82256ab

Alaska commented

2024-07-31 05:40:24 +02:00

Just to recap the current state of this PR.

The code changes to begin testing OSL on the CPU regularly is setup.
OptiX OSL testing is currently disabled due to bugs on that platform, and can be re-investigated once those are fixed.
Failing tests have been identified, and different courses of action have been taken for each:
- Expected differences (E.g. Image differences) - Added to a block list
- Simple Unexpected differences (E.g. UV difference) - I have fixed as many as I could in seperate pull requests.
- Complex Unexpected differences (E.g. Noise in Principled BSDF tests) - Added to the blocklist to be investigated later.

Open questions:

Should we fix the failing Principled BSDF tests before committing this PR? Or leave them in the blocklist and investigate it later?
Should the OSL tests be scheduled before the SVM tests to avoid OSL becoming a reference image? This will only have an impact on tests that subtly differ when using OSL (at the moment these are the Blackbody and one of the microfacet hair tests)

Backport to 4.2 LTS to allow for more thorough testing there? There are merge conflicts due to other changes like Use blocklist name for blocked tests and Enable MNEE tests on Metal. Should those also be backported? Or should I create a 4.2 version of this PR?

Just to recap the current state of this PR. - The code changes to begin testing OSL on the CPU regularly is setup. - OptiX OSL testing is currently disabled due to bugs on that platform, and can be re-investigated once those are fixed. - Failing tests have been identified, and different courses of action have been taken for each: - Expected differences (E.g. Image differences) - Added to a block list - Simple Unexpected differences (E.g. UV difference) - I have fixed as many as I could in seperate pull requests. - Complex Unexpected differences (E.g. Noise in Principled BSDF tests) - Added to the blocklist to be investigated later. Open questions: - Should we fix the failing Principled BSDF tests before committing this PR? Or leave them in the blocklist and investigate it later? - Should the OSL tests be scheduled before the SVM tests to avoid OSL becoming a reference image? This will only have an impact on tests that subtly differ when using OSL (at the moment these are the Blackbody and one of the microfacet hair tests) > - [ ] Backport to 4.2 LTS to allow for more thorough testing there? There are merge conflicts due to other changes like `Use blocklist name for blocked tests` and `Enable MNEE tests on Metal`. Should those also be backported? Or should I create a 4.2 version of this PR?

Sergey Sharybin reviewed 2024-08-07 12:10:57 +02:00

Sergey Sharybin left a comment

Should we fix the failing Principled BSDF tests before committing this PR? Or leave them in the blocklist and investigate it later?

Leave them in the blocklist for now, re-iterate later.
We should try to keep scoping narrow, and look into discovered issues as a follow-up.

Should the OSL tests be scheduled before the SVM tests to avoid OSL becoming a reference image?

I don't really think so. It is not different from the metal tests running after CPU i.e.
When I update reference image i explicitly specify the test with the device name in it to avoid the exact issue you've described. I think others do the same.

We can update the report template to explicitly include device type there, for easier copy-paste.

If you can update the PR against the latest main, I think it will be ready to land. From reading the code I can't spot stoppers (but hasn't been able to test it locally yet, but I can't imagine anything big will be discovered then).

> Should we fix the failing Principled BSDF tests before committing this PR? Or leave them in the blocklist and investigate it later? Leave them in the blocklist for now, re-iterate later. We should try to keep scoping narrow, and look into discovered issues as a follow-up. > Should the OSL tests be scheduled before the SVM tests to avoid OSL becoming a reference image? I don't really think so. It is not different from the metal tests running after CPU i.e. When I update reference image i explicitly specify the test with the device name in it to avoid the exact issue you've described. I think others do the same. We can update the report template to explicitly include device type there, for easier copy-paste. If you can update the PR against the latest `main`, I think it will be ready to land. From reading the code I can't spot stoppers (but hasn't been able to test it locally yet, but I can't imagine anything big will be discovered then).

tests/python/CMakeLists.txt Outdated

						
				@ -708,0 +721,4 @@

				                -testdir "${TEST_SRC_DIR}/render/${render_test}"

				                -outdir "${TEST_OUT_DIR}/cycles_osl"

				                -device ${_cycles_device}

				                -blocklist ""

Sergey Sharybin commented

2024-08-07 12:06:51 +02:00

Can we just skip the -blocklist argument?

Can we just skip the `-blocklist` argument?

Alaska commented

2024-08-07 12:46:15 +02:00

Simply removing results in errors like

  File "cycles_render_tests.py", line 219, in main
    if device != 'CPU' or 'OSL' in args.blocklist:
                          ^^^^^^^^^^^^^^^^^^^^^^^
TypeError: argument of type 'NoneType' is not iterable

This can probably be fixed by adjusting parser.add_argument("-blocklist", nargs="*"), but I haven't looked into yet. I'll test it in a few hours.

Simply removing results in errors like ``` File "cycles_render_tests.py", line 219, in main if device != 'CPU' or 'OSL' in args.blocklist: ^^^^^^^^^^^^^^^^^^^^^^^ TypeError: argument of type 'NoneType' is not iterable ``` This can probably be fixed by adjusting `parser.add_argument("-blocklist", nargs="*")`, but I haven't looked into yet. I'll test it in a few hours.

Sergey Sharybin commented

2024-08-07 14:24:50 +02:00

Ah, good point. I think parser.add_argument("-blocklist", nargs="*", default=[]) would do the trick.

Ah, good point. I think `parser.add_argument("-blocklist", nargs="*", default=[])` would do the trick.

Alaska commented

2024-08-07 14:27:15 +02:00

Was right in the middle of running the test suite to make sure it worked.

Alaska marked this conversation as resolved

tests/python/cycles_render_tests.py Outdated

						
				@ -23,3 +24,3 @@

				BLOCKLIST_OSL = [

				BLOCKLIST_EXPLICIT_OSL = [

				    # OSL only supported on CPU.

Sergey Sharybin commented

2024-08-07 12:05:26 +02:00

The comment needs to be updated to reflect nowadays realities. Maybe just remove it.

I'd also suggest adding some comment around BLOCKLIST_OSL and BLOCKLIST_EXPLICIT_OSL:

# Block list that disables OSL specific tests for configurations that do not support OSL backend.
BLOCKLIST_EXPLICIT_OSL = [...]

# Block list for tests that are enforced to render with OSL by the test to ensure match between SVM and OSL.
BLOCKLIST_OSL = [...]

The comment needs to be updated to reflect nowadays realities. Maybe just remove it. I'd also suggest adding some comment around `BLOCKLIST_OSL` and `BLOCKLIST_EXPLICIT_OSL`: ``` # Block list that disables OSL specific tests for configurations that do not support OSL backend. BLOCKLIST_EXPLICIT_OSL = [...] # Block list for tests that are enforced to render with OSL by the test to ensure match between SVM and OSL. BLOCKLIST_OSL = [...] ```

Alaska marked this conversation as resolved

Alaska added 2 commits 2024-08-07 12:46:49 +02:00

Merge branch 'main' into enable-osl-tests 63bc1cfaf9

Address Sergeys review. 6f6002fbe7

Alaska added 1 commit 2024-08-07 14:47:32 +02:00

Get rid of redundant blocklist in osl test ac2e7c9217

Alaska added 2 commits 2024-08-08 02:51:38 +02:00

Merge branch 'main' into enable-osl-tests 86acffe44e

Remove T49936 from OSL blocklist after fix from Lukas

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

2d9aab575e

Sergey Sharybin commented

2024-08-14 16:24:07 +02:00

@blender-bot build

Sergey Sharybin approved these changes 2024-08-14 16:39:11 +02:00

Sergey Sharybin commented

2024-08-14 16:41:29 +02:00

Thanks for the updates. I'll wait for the tests to officially finish, and land the change.

It adds abut 40sec (on Linux) time to tests, but all things considered I think this is a good trade-off.

For the 4.2, the Use blocklist name for blocked tests seems fine to port. The Enable MNEE tests on Metal i am not fully sure what it is about, can't see an exact commit. Does it require some change that i not in 4.2 yet?

Thanks for the updates. I'll wait for the tests to officially finish, and land the change. It adds abut 40sec (on Linux) time to tests, but all things considered I think this is a good trade-off. For the 4.2, the `Use blocklist name for blocked tests` seems fine to port. The `Enable MNEE tests on Metal` i am not fully sure what it is about, can't see an exact commit. Does it require some change that i not in 4.2 yet?

Sergey Sharybin merged commit 1330d0ddda into main

2024-08-14 17:01:07 +02:00

Sergey Sharybin referenced this issue from a commit

2024-08-14 17:01:08 +02:00

Cycles render tests: Add the option to test with OSL enabled

Sergey Sharybin deleted branch enable-osl-tests

2024-08-14 17:01:11 +02:00

Alaska commented

2024-08-14 17:11:30 +02:00

For the 4.2, the Use blocklist name for blocked tests seems fine to port. The Enable MNEE tests on Metal i am not fully sure what it is about, can't see an exact commit. Does it require some change that i not in 4.2 yet?

I don't know the full list of changes that need to be backported to avoid merge conflicts. All I remember is what I had to deal with when merging main into my branch during development.

The main ones were:
Enable MNEE tests on Metal: ef010da315
Rename to blocklist: 935c49f1cf
Fix typo from the blocklist rename (Thank you Sergey): 35198ec363

> For the 4.2, the `Use blocklist name for blocked tests` seems fine to port. The `Enable MNEE tests on Metal` i am not fully sure what it is about, can't see an exact commit. Does it require some change that i not in 4.2 yet? I don't know the full list of changes that need to be backported to avoid merge conflicts. All I remember is what I had to deal with when merging main into my branch during development. The main ones were: Enable MNEE tests on Metal: ef010da315b596edde5aaf9d8d60101ca3a64373 Rename to blocklist: 935c49f1cf97628d45a24ecee8c64a2e1f397f3b Fix typo from the blocklist rename (Thank you Sergey): 35198ec3633318cae4d203246ef044d7b6f1fee9

Sergey Sharybin commented

2024-08-14 18:05:00 +02:00

I don't see any issue with porting those. Basically, as long as ti is about expanding the coverage of tests that's all fine.
I'd just want to be a bit careful with porting commits that might change the look of renders.

I don't see any issue with porting those. Basically, as long as ti is about expanding the coverage of tests that's all fine. I'd just want to be a bit careful with porting commits that might change the look of renders.

Alaska commented

2024-08-14 18:11:29 +02:00

That makes sense. I've added them to the LTS task
Edit: Temporarily removed because vector math tests will fail due to assert on release on build bot.

That makes sense. I've added them to the LTS task Edit: Temporarily removed because vector math tests will fail due to assert on release on build bot.

Alaska referenced this pull request

2024-08-15 04:32:21 +02:00

Cycles: Improve the coverage of automated regression tests #123012

Jeroen Bakker referenced this issue from a commit

2024-08-15 14:44:50 +02:00

Cycles render tests: Add the option to test with OSL enabled

Alaska referenced this issue from a commit

2024-08-16 13:49:24 +02:00

Cycles render tests: Add the option to test with OSL enabled

Sign in to join this conversation.

No reviewers