Make caching warn if some of the args are unhashable #2585

mraspaud · 2023-10-04T07:17:52Z

This PR make the caching decorator fail if one of the arguments in unhashable where it was silently ignoring it previously and not caching.

Tests added

djhoese

Works for me. I'm not sure why I did it this way originally. Maybe I had hoped that we could still cache based on all the other inputs...but that doesn't really make sense.

Feel free to merge once tests pass and we'll see what breaks.

djhoese · 2023-10-04T11:57:01Z

Ah the tests have revealed it. We still want to be able to run the function even if it can't be cached. Maybe a warning? But the warning would have to go after the check to see if caching is requested (enabled) and after we know that there is an uncacheable type (ex. SwathDefinition).

codecov · 2023-10-10T13:36:17Z

Codecov Report

Merging #2585 (f189402) into main (95eb01a) will increase coverage by 0.27%.
Report is 167 commits behind head on main.
The diff coverage is 97.72%.

@@            Coverage Diff             @@
##             main    #2585      +/-   ##
==========================================
+ Coverage   94.91%   95.18%   +0.27%     
==========================================
  Files         351      354       +3     
  Lines       51215    51270      +55     
==========================================
+ Hits        48611    48802     +191     
+ Misses       2604     2468     -136

Flag	Coverage Δ
behaviourtests	`4.24% <6.81%> (-0.06%)`	⬇️
unittests	`95.81% <97.72%> (+0.29%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
satpy/tests/modifier_tests/test_angles.py	`99.50% <100.00%> (-0.50%)`	⬇️
satpy/modifiers/angles.py	`99.21% <96.55%> (+0.04%)`	⬆️

... and 28 files with indirect coverage changes

djhoese · 2023-10-10T18:45:09Z

satpy/modifiers/angles.py

@@ -250,6 +248,7 @@ def _hash_args(*args, unhashable_types=DEFAULT_UNCACHE_TYPES):
    hashable_args = []
    for arg in args:
        if isinstance(arg, unhashable_types):
+            warnings.warn(f"Unhashable type in function signature ({type(arg)}), cannot be cached.", stacklevel=2)


I'm unsure about this. For one the stacklevel should be at least 3 in my opinion as 2 only gets us to __call__ of this helper class.

As for warning versus something else, I think it is confusing. It is perfectly valid to call these functions with an uncacheable/unhashable type like a SwathDefinition or a StackedAreaDefinition. There shouldn't be a warning for these cases. In my previous comment I pointed out that this warning needs to go after the should_cache check which checks if the user has caching enabled. Otherwise users could be getting this when they know they aren't going to cache things (uncacheable types) and they haven't even requested that it be attempted (the config key is disabled/off).

Good point! Let me fix that.

This is now fixed. Thanks for pointing this out.

mraspaud · 2023-10-11T09:08:45Z

satpy/modifiers/angles.py

-                self._cache_results(res, zarr_format)
+            self._warn_if_irregular_input_chunks(args, args_to_use)
+            self._cache_results(res, zarr_format)
+
        # if we did any caching, let's load from the zarr files


@djhoese while refactoring this function a bit, I got confused by this: Why do we load from the zarr files just after we cached it to disk?

So that we're caching immediately and using those cached values.

So that future calls to this function that result in the same cached results will use the same cached values and when .compute() is finally called there is no redundant computation. By that I mean if we generate these angles once but then continue to use the dask array and then a different product generates these angles and detects the cached value and uses the cached arrays, then when we finally do dask.array.compute(composite1, composite2) we would get one set of computations based on the original dask array and one set of computations based on the cached array even though they should be the same numbers.

ah, ok, good. I might improve the comment to explain point 2, for future us.

djhoese

I think this all looks good. I walked through the logic and it seems consistent with what was there before. I guess my only request is wondering if it can be split into helper functions/methods to make it any more clear? If that seems too hard I'm fine with this being merged.

mraspaud · 2023-10-11T19:19:52Z

Indeed I unfolded the logic a bit to make it clearer but at the expense of the size of the function. I'll try refactoring now that you also shed light on the details I was missing.

mraspaud · 2023-10-24T10:58:39Z

I'm ready to merge this, @djhoese any more comments?

djhoese

One small naming question that shouldn't hold up merging, but otherwise I remember agreeing with the logic before the refactor and this still looks understandable so 👍

satpy/modifiers/angles.py

djhoese · 2023-11-15T19:59:00Z

I think this has been good for a while, we just forgot to merge it. Merging now...

djhoese · 2023-11-15T20:28:02Z

Oh great, CI on main is failing now! I'll see if I can fix it tonight.

mraspaud · 2023-11-15T20:29:23Z

Wtf?

djhoese · 2023-11-15T20:43:22Z

 FAILED satpy/tests/modifier_tests/test_angles.py::TestAngleGeneration::test_cache_get_angles[_get_angle_test_data_rgb_nodims-9-exp_zarr_chunks5-_diff_sat_pos_datetime-False-6-True] - AssertionError: assert 17 == (9 * (1 + 1))
 +  where 17 = <MagicMock name='get_observer_look' id='1888278755600'>.call_count
 +  and   1 = int(True)

djhoese · 2023-11-15T20:43:30Z

Might be a hiccup or race condition.

mraspaud · 2023-11-15T20:52:15Z

Or could the caching behaviour have changed and change the call count

djhoese · 2023-11-15T20:54:08Z

Restarting just to see...

djhoese · 2023-11-15T23:42:27Z

Restarting and it seems to pass

mraspaud added 2 commits October 4, 2023 09:12

Make caching fail if one of the args is unhashable

04cfbd5

Remove redundant hashability check

9ad6b16

mraspaud added the bug label Oct 4, 2023

mraspaud self-assigned this Oct 4, 2023

mraspaud requested a review from djhoese as a code owner October 4, 2023 07:17

djhoese approved these changes Oct 4, 2023

View reviewed changes

Make caching using wrong types a warning instead of an error

6a59c85

djhoese reviewed Oct 10, 2023

View reviewed changes

Issue warning for caching only when caching is requested

aaec89a

mraspaud commented Oct 11, 2023

View reviewed changes

Improve comment

1d63b69

djhoese approved these changes Oct 11, 2023

View reviewed changes

Refactor cache helper's call

5038b50

mraspaud changed the title ~~Make caching fail if one of the args is unhashable~~ Make caching warn if some of the args are unhashable Oct 17, 2023

djhoese approved these changes Oct 24, 2023

View reviewed changes

satpy/modifiers/angles.py Outdated Show resolved Hide resolved

Rename zarr_format to zarr_file_pattern

f189402

djhoese merged commit ce41239 into pytroll:main Nov 15, 2023
18 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make caching warn if some of the args are unhashable #2585

Make caching warn if some of the args are unhashable #2585

mraspaud commented Oct 4, 2023

djhoese left a comment •

edited

Loading

djhoese commented Oct 4, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

djhoese Oct 10, 2023

mraspaud Oct 11, 2023

mraspaud Oct 11, 2023

mraspaud Oct 11, 2023

djhoese Oct 11, 2023

mraspaud Oct 11, 2023

djhoese left a comment

mraspaud commented Oct 11, 2023

mraspaud commented Oct 24, 2023

djhoese left a comment

djhoese commented Nov 15, 2023

djhoese commented Nov 15, 2023

mraspaud commented Nov 15, 2023

djhoese commented Nov 15, 2023

djhoese commented Nov 15, 2023

mraspaud commented Nov 15, 2023

djhoese commented Nov 15, 2023

djhoese commented Nov 15, 2023

Make caching warn if some of the args are unhashable #2585

Make caching warn if some of the args are unhashable #2585

Conversation

mraspaud commented Oct 4, 2023

djhoese left a comment • edited Loading

Choose a reason for hiding this comment

djhoese commented Oct 4, 2023

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

djhoese Oct 10, 2023

Choose a reason for hiding this comment

mraspaud Oct 11, 2023

Choose a reason for hiding this comment

mraspaud Oct 11, 2023

Choose a reason for hiding this comment

mraspaud Oct 11, 2023

Choose a reason for hiding this comment

djhoese Oct 11, 2023

Choose a reason for hiding this comment

mraspaud Oct 11, 2023

Choose a reason for hiding this comment

djhoese left a comment

Choose a reason for hiding this comment

mraspaud commented Oct 11, 2023

mraspaud commented Oct 24, 2023

djhoese left a comment

Choose a reason for hiding this comment

djhoese commented Nov 15, 2023

djhoese commented Nov 15, 2023

mraspaud commented Nov 15, 2023

djhoese commented Nov 15, 2023

djhoese commented Nov 15, 2023

mraspaud commented Nov 15, 2023

djhoese commented Nov 15, 2023

djhoese commented Nov 15, 2023

djhoese left a comment •

edited

Loading

codecov bot commented Oct 10, 2023 •

edited

Loading