-
Notifications
You must be signed in to change notification settings - Fork 300
Lockfile updates plus adapt to Dask changes to delayed and internals #6438
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 all commits
58b30fb
9e6d82c
0ae051d
98e44bc
d76a9d8
ddbf640
4459201
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 | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |||||||||||||||||||||
| # importing anything else. | ||||||||||||||||||||||
| import iris.tests as tests # isort:skip | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import contextlib | ||||||||||||||||||||||
| from copy import deepcopy | ||||||||||||||||||||||
| from unittest import mock | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -91,7 +92,6 @@ def test_deferred_fix_lbrow_lbnpt(self): | |||||||||||||||||||||
| self.assertEqual(f1.lbrow, 3) | ||||||||||||||||||||||
| self.assertEqual(f1.lbnpt, 4) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @tests.skip_data | ||||||||||||||||||||||
| def test_landsea_unpacking_uses_dask(self): | ||||||||||||||||||||||
| # Ensure that the graph of the (lazy) landsea-masked data contains an | ||||||||||||||||||||||
| # explicit reference to a (lazy) landsea-mask field. | ||||||||||||||||||||||
|
|
@@ -112,21 +112,32 @@ def test_landsea_unpacking_uses_dask(self): | |||||||||||||||||||||
| lazy_mask_array = landsea_mask.core_data() | ||||||||||||||||||||||
| lazy_soildata_array = soil_temp.core_data() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Work out the main dask key for the mask data, as used by 'compute()'. | ||||||||||||||||||||||
| mask_toplev_key = (lazy_mask_array.name,) + (0,) * lazy_mask_array.ndim | ||||||||||||||||||||||
| # Get the 'main' calculation entry. | ||||||||||||||||||||||
| mask_toplev_item = lazy_mask_array.dask[mask_toplev_key] | ||||||||||||||||||||||
| # This should be a task (a simple fetch). | ||||||||||||||||||||||
| self.assertTrue(callable(mask_toplev_item)) | ||||||||||||||||||||||
| # Get the key (name) of the array that it fetches. | ||||||||||||||||||||||
| mask_data_name = mask_toplev_item.args[0].key | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Check that the item this refers to is a PPDataProxy. | ||||||||||||||||||||||
| self.assertIsInstance(lazy_mask_array.dask[mask_data_name], pp.PPDataProxy) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Check that the soil-temp graph references the *same* lazy element, | ||||||||||||||||||||||
| # showing that the mask+data calculation is handled by dask. | ||||||||||||||||||||||
| self.assertIn(mask_data_name, lazy_soildata_array.dask.keys()) | ||||||||||||||||||||||
| def is_pp_layer(obj): | ||||||||||||||||||||||
| result = False | ||||||||||||||||||||||
| if hasattr(obj, "values"): | ||||||||||||||||||||||
| if len(obj) == 1: | ||||||||||||||||||||||
| (result,) = [hasattr(v, "_lbpack") for v in obj.values()] | ||||||||||||||||||||||
| return result | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Find the single reference to the PPDataProxy within the dask graph. | ||||||||||||||||||||||
| (layer,) = [ | ||||||||||||||||||||||
| lay for lay in lazy_mask_array.dask.layers.values() if is_pp_layer(lay) | ||||||||||||||||||||||
| ] | ||||||||||||||||||||||
| ((layer_key, proxy),) = layer.items() | ||||||||||||||||||||||
| # Replace the PPDataProxy with a mock. | ||||||||||||||||||||||
| # By replacing directly within the dask graph, we can prove whether an | ||||||||||||||||||||||
| # operation is relying on the graph in the expected way. | ||||||||||||||||||||||
| mock_proxy = mock.MagicMock(spec=pp.PPDataProxy, wraps=proxy) | ||||||||||||||||||||||
| layer.mapping[layer_key] = mock_proxy | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| self.assertFalse(mock_proxy.__getitem__.called) | ||||||||||||||||||||||
| with contextlib.suppress(TypeError): | ||||||||||||||||||||||
| # Expect this to crash - data realisation is complex, and it is not | ||||||||||||||||||||||
| # worth mocking this fully. | ||||||||||||||||||||||
| _ = lazy_soildata_array.compute() | ||||||||||||||||||||||
| # Proves that the soil data graph includes the mask graph - computing | ||||||||||||||||||||||
| # the soil data has accessed the mask graph object which we mocked. | ||||||||||||||||||||||
| self.assertTrue(mock_proxy.__getitem__.called) | ||||||||||||||||||||||
|
Comment on lines
+115
to
+140
Contributor
Author
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. This change in approach is more agnostic to Dask's internals (although still somewhat vulnerable). It successfully runs, only failing in a classic test failure rather than falling over. But all I have managed to do is prove that Dask Results of updated test
Understanding if this is cause for concern - and if so, how to fix things - will be complex. The development team are fully booked on other projects for the foreseeable, so I would suggest a patch release with a
Contributor
Author
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. Discussed at length with @pp-mo, who explained that this test was not written to demonstrate the other changes in #3255, but because there was a concern about something else similar happening again. Our increased experience with Dask in the 6 years since has given us the confidence that:
It looks like the refactoring that made this test fail (dask/dask#11736) was technical debt work to make Dask safer and more robust, and is NOT evidence that we have become vulnerable to nested Based on all of this, we have decided to remove the test. |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if __name__ == "__main__": | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.