From 8e305a0befcfb98075df365d1ab0df544cada1c3 Mon Sep 17 00:00:00 2001 From: Mark Harfouche Date: Mon, 30 Dec 2024 08:39:02 -0500 Subject: [PATCH 1/6] Improve error reporting with screenshots and provide a method to test locally --- README.md | 13 ++++++ examples/tests/test_examples.py | 71 ++++++++++++++++++++++--------- wgpu/backends/wgpu_native/_api.py | 32 ++++++++++++++ 3 files changed, 95 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 3e432a8d..8349dab7 100644 --- a/README.md +++ b/README.md @@ -178,3 +178,16 @@ differences. If you want to update the reference screenshot for a given example, you can grab those from the build artifacts as well and commit them to your branch. + +### Testing Locally + +Testing locally is possible, however pixel perfect results will differ from +those on the CIs due to discrepencies in hardware, and driver (we use llvmpipe) +versions. + +If you want to force the usage of LLVMPIPE to speed up local testing you +may do so with the WGPUPY_WGPU_ADAPTER_NAME environment variable + +``` +WGPUPY_WGPU_ADAPTER_NAME=llvmpipe pytest -v examples/ +``` diff --git a/examples/tests/test_examples.py b/examples/tests/test_examples.py index a9b9b1d1..955df62e 100644 --- a/examples/tests/test_examples.py +++ b/examples/tests/test_examples.py @@ -94,7 +94,10 @@ def unload_module(): # the first part of the test everywhere else; ensuring that examples # can at least import, run and render something if not is_lavapipe: - pytest.skip("screenshot comparisons are only done when using lavapipe") + pytest.skip( + "screenshot comparisons are only done when using lavapipe. " + "Rerun your tests with WGPUPY_WGPU_ADAPTER_NAME=llvmpipe" + ) # regenerate screenshot if requested screenshots_dir.mkdir(exist_ok=True) @@ -108,36 +111,62 @@ def unload_module(): ), "found # test_example = true but no reference screenshot available" stored_img = imageio.imread(screenshot_path) # assert similarity - is_similar = np.allclose(img, stored_img, atol=1) - update_diffs(module, is_similar, img, stored_img) - assert is_similar, ( - f"rendered image for example {module} changed, see " - f"the {diffs_dir.relative_to(ROOT).as_posix()} folder" - " for visual diffs (you can download this folder from" - " CI build artifacts as well)" - ) + atol = 1 + try: + np.testing.assert_allclose(img, stored_img, atol=atol) + is_similar = True + except Exception as e: + is_similar = False + raise AssertionError( + f"rendered image for example {module_name} changed, see " + f"the {diffs_dir.relative_to(ROOT).as_posix()} folder" + " for visual diffs (you can download this folder from" + " CI build artifacts as well)" + ) from e + finally: + update_diffs(module_name, is_similar, img, stored_img, atol=atol) -def update_diffs(module, is_similar, img, stored_img): +def update_diffs(module, is_similar, img, stored_img, *, atol): diffs_dir.mkdir(exist_ok=True) + + if is_similar: + for path in [ + # Keep filename in sync with the ones generated below + diffs_dir / f"{module}-rgb.png", + diffs_dir / f"{module}-alpha.png", + diffs_dir / f"{module}-rgb-above_atol.png", + diffs_dir / f"{module}-alpha-above_atol.png", + diffs_dir / f"{module}.png", + ]: + if path.exists(): + path.unlink() + return + # cast to float32 to avoid overflow # compute absolute per-pixel difference diffs_rgba = np.abs(stored_img.astype("f4") - img) + + diffs_rgba_above_atol = diffs_rgba.copy() + diffs_rgba_above_atol[diffs_rgba <= atol] = 0 + # magnify small values, making it easier to spot small errors diffs_rgba = ((diffs_rgba / 255) ** 0.25) * 255 # cast back to uint8 diffs_rgba = diffs_rgba.astype("u1") - # split into an rgb and an alpha diff - diffs = { - diffs_dir / f"{module}-rgb.png": diffs_rgba[..., :3], - diffs_dir / f"{module}-alpha.png": diffs_rgba[..., 3], - } - - for path, diff in diffs.items(): - if not is_similar: - imageio.imwrite(path, diff) - elif path.exists(): - path.unlink() + + diffs_rgba_above_atol = ((diffs_rgba_above_atol / 255) ** 0.25) * 255 + diffs_rgba_above_atol = diffs_rgba_above_atol.astype("u1") + # And highlight differences that are above the atol + imageio.imwrite(diffs_dir / f"{module}-rgb.png", diffs_rgba[..., :3]) + imageio.imwrite(diffs_dir / f"{module}-alpha.png", diffs_rgba[..., 3]) + imageio.imwrite( + diffs_dir / f"{module}-rgb-above_atol.png", diffs_rgba_above_atol[..., :3] + ) + imageio.imwrite( + diffs_dir / f"{module}-alpha-above_atol.png", diffs_rgba_above_atol[..., 3] + ) + imageio.imwrite(diffs_dir / f"{module}.png", img) @pytest.mark.parametrize("module", examples_to_run) diff --git a/wgpu/backends/wgpu_native/_api.py b/wgpu/backends/wgpu_native/_api.py index 4b2177ba..c559a4dd 100644 --- a/wgpu/backends/wgpu_native/_api.py +++ b/wgpu/backends/wgpu_native/_api.py @@ -367,6 +367,21 @@ def request_adapter_sync( This is the implementation based on wgpu-native. """ check_can_use_sync_variants() + # Similar to https://github.com/gfx-rs/wgpu?tab=readme-ov-file#environment-variables + # It seems that the environment variables are only respected in their + # testing environments maybe???? + # In Dec 2024 we couldn't get the use of their environment variables to work + # This should only be used in testing environments and API users + # should beware + # We chose the variable name WGPUPY_WGPU_ADAPTER_NAME instead WGPU_ADAPTER_NAME + # to avoid a clash + if adapter_name := os.getenv(("WGPUPY_WGPU_ADAPTER_NAME")): + adapters = self.enumerate_adapters_sync() + adapters_llvm = [a for a in adapters if adapter_name in a.summary] + if not adapters_llvm: + raise ValueError(f"Adapter with name '{adapter_name}' not found.") + return adapters_llvm[0] + awaitable = self._request_adapter( power_preference=power_preference, force_fallback_adapter=force_fallback_adapter, @@ -394,6 +409,23 @@ async def request_adapter_async( canvas : The canvas that the adapter should be able to render to. This can typically be left to None. If given, the object must implement ``WgpuCanvasInterface``. """ + # Similar to https://github.com/gfx-rs/wgpu?tab=readme-ov-file#environment-variables + # It seems that the environment variables are only respected in their + # testing environments maybe???? + # In Dec 2024 we couldn't get the use of their environment variables to work + # This should only be used in testing environments and API users + # should beware + # We chose the variable name WGPUPY_WGPU_ADAPTER_NAME instead WGPU_ADAPTER_NAME + # to avoid a clash + if adapter_name := os.getenv(("WGPUPY_WGPU_ADAPTER_NAME")): + # Is this correct for async??? I know nothing of async... + awaitable = self.enumerate_adapters_async() + adapters = await awaitable + adapters_llvm = [a for a in adapters if adapter_name in a.summary] + if not adapters_llvm: + raise ValueError(f"Adapter with name '{adapter_name}' not found.") + return adapters_llvm[0] + awaitable = self._request_adapter( power_preference=power_preference, force_fallback_adapter=force_fallback_adapter, From f82b81037d2c510cff8765f26369ab14fa9af206 Mon Sep 17 00:00:00 2001 From: Mark Harfouche Date: Tue, 31 Dec 2024 09:26:59 -0500 Subject: [PATCH 2/6] improve readme. --- README.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8349dab7..e96908f8 100644 --- a/README.md +++ b/README.md @@ -185,9 +185,21 @@ Testing locally is possible, however pixel perfect results will differ from those on the CIs due to discrepencies in hardware, and driver (we use llvmpipe) versions. -If you want to force the usage of LLVMPIPE to speed up local testing you -may do so with the WGPUPY_WGPU_ADAPTER_NAME environment variable +On linux, it is possible to force to force the usage of LLVMPIPE in the test suite +and compare the generated results of screenshots. Beware, the results on your machine +may differ to those on the CI. We always include the CI screenshots in the test suite +to improve the repeatability of the tests. + +If you have access to a linux machine with llvmpipe installed, you may run the +example pixel comparison testing by setting the WGPUPY_WGPU_ADAPTER_NAME +environment variable appropriately. For example + ``` WGPUPY_WGPU_ADAPTER_NAME=llvmpipe pytest -v examples/ ``` + +The `WGPUPY_WGPU_ADAPTER_NAME` variable is modeled after the +https://github.com/gfx-rs/wgpu?tab=readme-ov-file#environment-variables +and should only be used for testing the wgpupy library itself. +It is not part of the supported wgpy-py interface. From 61e7234d89a0e54128087290f9afceb47024447b Mon Sep 17 00:00:00 2001 From: Mark Harfouche Date: Tue, 31 Dec 2024 09:41:04 -0500 Subject: [PATCH 3/6] Break everything because i don't understand async --- wgpu/backends/wgpu_native/_api.py | 47 +++++++++++++------------------ 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/wgpu/backends/wgpu_native/_api.py b/wgpu/backends/wgpu_native/_api.py index c559a4dd..8800197a 100644 --- a/wgpu/backends/wgpu_native/_api.py +++ b/wgpu/backends/wgpu_native/_api.py @@ -367,20 +367,6 @@ def request_adapter_sync( This is the implementation based on wgpu-native. """ check_can_use_sync_variants() - # Similar to https://github.com/gfx-rs/wgpu?tab=readme-ov-file#environment-variables - # It seems that the environment variables are only respected in their - # testing environments maybe???? - # In Dec 2024 we couldn't get the use of their environment variables to work - # This should only be used in testing environments and API users - # should beware - # We chose the variable name WGPUPY_WGPU_ADAPTER_NAME instead WGPU_ADAPTER_NAME - # to avoid a clash - if adapter_name := os.getenv(("WGPUPY_WGPU_ADAPTER_NAME")): - adapters = self.enumerate_adapters_sync() - adapters_llvm = [a for a in adapters if adapter_name in a.summary] - if not adapters_llvm: - raise ValueError(f"Adapter with name '{adapter_name}' not found.") - return adapters_llvm[0] awaitable = self._request_adapter( power_preference=power_preference, @@ -409,6 +395,17 @@ async def request_adapter_async( canvas : The canvas that the adapter should be able to render to. This can typically be left to None. If given, the object must implement ``WgpuCanvasInterface``. """ + + awaitable = self._request_adapter( + power_preference=power_preference, + force_fallback_adapter=force_fallback_adapter, + canvas=canvas, + ) # no-cover + return await awaitable + + def _request_adapter( + self, *, power_preference=None, force_fallback_adapter=False, canvas=None + ): # Similar to https://github.com/gfx-rs/wgpu?tab=readme-ov-file#environment-variables # It seems that the environment variables are only respected in their # testing environments maybe???? @@ -418,24 +415,18 @@ async def request_adapter_async( # We chose the variable name WGPUPY_WGPU_ADAPTER_NAME instead WGPU_ADAPTER_NAME # to avoid a clash if adapter_name := os.getenv(("WGPUPY_WGPU_ADAPTER_NAME")): - # Is this correct for async??? I know nothing of async... - awaitable = self.enumerate_adapters_async() - adapters = await awaitable + adapters = self._enumerate_adapters() adapters_llvm = [a for a in adapters if adapter_name in a.summary] if not adapters_llvm: raise ValueError(f"Adapter with name '{adapter_name}' not found.") - return adapters_llvm[0] - - awaitable = self._request_adapter( - power_preference=power_preference, - force_fallback_adapter=force_fallback_adapter, - canvas=canvas, - ) # no-cover - return await awaitable + awaitable = WgpuAwaitable( + 'llvm adapter', + callback=lambda: (), + finalizer=lambda x: x, + ) + awaitable.set_result(adapters_llvm[0]) - def _request_adapter( - self, *, power_preference=None, force_fallback_adapter=False, canvas=None - ): + return awaitable # ----- Surface ID # Get surface id that the adapter must be compatible with. If we From ad28b5b703bf8c51dc0e8688cf4d17428bbb336a Mon Sep 17 00:00:00 2001 From: Mark Harfouche Date: Thu, 2 Jan 2025 18:00:03 -0500 Subject: [PATCH 4/6] run ruff format... --- wgpu/backends/wgpu_native/_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wgpu/backends/wgpu_native/_api.py b/wgpu/backends/wgpu_native/_api.py index 8800197a..3a3a3d4a 100644 --- a/wgpu/backends/wgpu_native/_api.py +++ b/wgpu/backends/wgpu_native/_api.py @@ -420,7 +420,7 @@ def _request_adapter( if not adapters_llvm: raise ValueError(f"Adapter with name '{adapter_name}' not found.") awaitable = WgpuAwaitable( - 'llvm adapter', + "llvm adapter", callback=lambda: (), finalizer=lambda x: x, ) From b70510ea2c91e92f89ba8f725b6b4e1e6db9efbe Mon Sep 17 00:00:00 2001 From: Mark Harfouche Date: Thu, 2 Jan 2025 18:43:09 -0500 Subject: [PATCH 5/6] spelling --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e96908f8..4cde5157 100644 --- a/README.md +++ b/README.md @@ -201,5 +201,5 @@ WGPUPY_WGPU_ADAPTER_NAME=llvmpipe pytest -v examples/ The `WGPUPY_WGPU_ADAPTER_NAME` variable is modeled after the https://github.com/gfx-rs/wgpu?tab=readme-ov-file#environment-variables -and should only be used for testing the wgpupy library itself. -It is not part of the supported wgpy-py interface. +and should only be used for testing the wgpu-py library itself. +It is not part of the supported wgpu-py interface. From 6310016c29701e9c51ad0e17fd4fad3686fdb851 Mon Sep 17 00:00:00 2001 From: Mark Harfouche Date: Thu, 2 Jan 2025 18:44:51 -0500 Subject: [PATCH 6/6] minimize changes --- wgpu/backends/wgpu_native/_api.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/wgpu/backends/wgpu_native/_api.py b/wgpu/backends/wgpu_native/_api.py index 3a3a3d4a..dffd010c 100644 --- a/wgpu/backends/wgpu_native/_api.py +++ b/wgpu/backends/wgpu_native/_api.py @@ -367,7 +367,6 @@ def request_adapter_sync( This is the implementation based on wgpu-native. """ check_can_use_sync_variants() - awaitable = self._request_adapter( power_preference=power_preference, force_fallback_adapter=force_fallback_adapter, @@ -395,7 +394,6 @@ async def request_adapter_async( canvas : The canvas that the adapter should be able to render to. This can typically be left to None. If given, the object must implement ``WgpuCanvasInterface``. """ - awaitable = self._request_adapter( power_preference=power_preference, force_fallback_adapter=force_fallback_adapter,