fix dask meta and output_dtypes error

[mathause]

• Closes 🟠 Test failure on master #5444

This was changed in dask/dask#7669. Looks like they did not deprecate this behavior (i.e. passing both meta and output_dtypes). I'd suggest to follow dask's example here and not add a deprecation cycle. Thoughts?

[dcherian]

Thanks @mathause

[mathause]

thanks for the review - let's merge soon

[max-sixty]

Thanks @mathause !

[github-actions]

Unit Test Results

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 34dc577. ± Comparison against base commit 34dc577.

[jrbourbeau]

Apologies for the breaking change. I'm happy to make sure we ping an Xarray dev next time we have a significant change to apply_gufunc to avoid surprises like this in the future.

[gjoseph92]

@mathause sorry for breaking things here. Note that passing output_dtypes didn't work as it was supposed to before, and also didn't cause a cast. We went back and forth on whether output_types should cause explicit casting, and whether it was sensible to provide both it and meta. Ultimately we decided they should be mutually exclusive, and should not cause casting, but without much knowledge of how downstream libraries were using these arguments. So maybe we should revisit that choice in dask?

Also I think maybe this test should be changed rather than skipped. Saying output_dtypes=[int] and then assert float == actual.dtype just seems weird to me. Perhaps removing one of output_dtypes or meta from the test would be the best solution.

[dcherian]

but without much knowledge of how downstream libraries were using these arguments.

IIRC xarray doesn't actually use it. xarray.apply_ufunc is a convenient wrapper for apply_gufunc so we just pass along what the user provides.

@kmuehlbauer added that test so maybe he can provide more context

Perhaps removing one of output_dtypes or meta from the test would be the best solution.

OK good point! There are a few tests for output_dtypes but we don't actually test for providing dtype in meta, so perhaps we should do that.

[mathause]

No worries. I agree that only allowing one is cleaner. And definitively +1 on adding tests using meta.

[kmuehlbauer]

Thanks for the ping @dcherian.

There is this test:

xarray/xarray/tests/test_computation.py

Lines 1314 to 1330 in da0489f

	def test_vectorize_dask_dtype_meta():
	# meta dtype takes precedence
	data_array = xr.DataArray([[0, 1, 2], [1, 2, 3]], dims=("x", "y"))
	expected = xr.DataArray([1, 2], dims=["x"])
	actual = apply_ufunc(
	pandas_median,
	data_array.chunk({"x": 1}),
	input_core_dims=[["y"]],
	vectorize=True,
	dask="parallelized",
	output_dtypes=[int],
	dask_gufunc_kwargs=dict(meta=np.ndarray((0, 0), dtype=float)),
	)
	assert_identical(expected, actual)
	assert float == actual.dtype

which provides dtype in meta. Maybe these different tests have to be aligned/combined? Unfortunately memory gets hazy over time, so I can't say much more about it without a deeper dive into.