Skip to content

use numpys SupportsDtype #7521

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

Merged
merged 1 commit into from
Feb 28, 2023
Merged

use numpys SupportsDtype #7521

merged 1 commit into from
Feb 28, 2023

Conversation

headtr1ck
Copy link
Collaborator

I don't know how I feel about using private numpy classes that might change anytime.
Maybe within a if TYPE_CHECKING block it is not too bad?

@headtr1ck
Copy link
Collaborator Author

@Illviljan I was away for quite some time, do you have an idea what is wrong with mypy-3.9?

@Illviljan
Copy link
Contributor

I don't get the error. Distributed has a py.typed file https://github.com/dask/distributed/blob/main/distributed/py.typed
And if it was py.typed issue we should have seen it in other PRs and I haven't seen one yet that has this error..

@TomNicholas added py.typed to package data, xarray-contrib/datatree@927749a
But still, if distributed also needs to do this we should see this error in more PRs.

      ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 49.2/49.2 kB 3.6 MB/s eta 0:00:00
Collecting types-docutils
  Downloading types_docutils-0.19.1.3-py3-none-any.whl (16 kB)
Installing collected packages: types-PyYAML, types-pytz, types-docutils, types-setuptools
Successfully installed types-PyYAML-6.0.12.5 types-docutils-0.19.1.3 types-pytz-2022.7.1.0 types-setuptools-67.2.0.1
xarray/tests/test_distributed.py:14: error: Skipping analyzing "distributed": module is installed, but missing library stubs or py.typed marker  [import]
xarray/tests/test_distributed.py:21: error: Skipping analyzing "distributed.client": module is installed, but missing library stubs or py.typed marker  [import]
xarray/tests/test_distributed.py:21: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
xarray/tests/test_distributed.py:22: error: Skipping analyzing "distributed.utils_test": module is installed, but missing library stubs or py.typed marker  [import]
Generated Cobertura report: /home/runner/work/xarray/xarray/mypy_report/cobertura.xml
Installing missing stub packages:
/home/runner/micromamba-root/envs/xarray-tests/bin/python -m pip install types-PyYAML types-pytz types-setuptools


Generated Cobertura report: /home/runner/work/xarray/xarray/mypy_report/cobertura.xml
Found 3 errors in 1 file (checked 140 source files)

@headtr1ck
Copy link
Collaborator Author

I vaguely remember seeing this error before, but no idea how it got fixed.
Maybe this run uses an older version of distributed?

@Illviljan
Copy link
Contributor

dask-core                 2023.2.0           pyhd8ed1ab_0    conda-forge
distarray                 2.12.2             pyh050c7b8_4    conda-forge
distlib                   0.3.6              pyhd8ed1ab_0    conda-forge
distributed               2021.4.1         py39hf3d152e_1    conda-forge

Yeah looks like an old version. py.typed was around september so it makes sense that part.

@headtr1ck
Copy link
Collaborator Author

headtr1ck commented Feb 11, 2023

Is it some caching issue, since this PR has been lying around for some time?
Edit: nvmd, haha. I just created it literally yesterday.

@headtr1ck headtr1ck added the plan to merge Final call for comments label Feb 24, 2023
@dcherian
Copy link
Contributor

Thanks @headtr1ck

Do you know if there's a plan to make something like this public on the numpy end?

@dcherian dcherian merged commit 6531b57 into pydata:main Feb 28, 2023
@headtr1ck
Copy link
Collaborator Author

Do you know if there's a plan to make something like this public on the numpy end?

No idea, the static typing of numpy is still changing a lot and has many bugs (same with xarray, haha).

I'm sure they will make it public is we request it.

@dcherian
Copy link
Contributor

dcherian commented Mar 1, 2023

Ugh I think I clicked merge too early.

Shall we just keep it on our end till it become public? I defer to your judgement here.

@headtr1ck
Copy link
Collaborator Author

I have opened numpy/numpy#23308

It should be an easy change on numpys side if they decide to go for it.
Only problem, we again have to special case this based on bumpy version, haha.

@headtr1ck headtr1ck deleted the dtype branch March 2, 2023 20:23
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 9, 2023
* main:
  Preserve `base` and `loffset` arguments in `resample` (pydata#7444)
  ignore the `pkg_resources` deprecation warning (pydata#7594)
  Update contains_cftime_datetimes to avoid loading entire variable array (pydata#7494)
  Support first, last with dask arrays (pydata#7562)
  update the docs environment (pydata#7442)
  Add xCDAT to list of Xarray related projects (pydata#7579)
  [pre-commit.ci] pre-commit autoupdate (pydata#7565)
  fix nczarr when libnetcdf>4.8.1 (pydata#7575)
  use numpys SupportsDtype (pydata#7521)
@headtr1ck
Copy link
Collaborator Author

It seems that numpy is somewhat against this change, so maybe we are better off to restore the previous state to not have a mess with supporting different numpy versions again.
Lets keep that in mind before the next release but wait if something changes on numpys side.

@headtr1ck
Copy link
Collaborator Author

I guess we should revert this change and use our own definition as proposed by the numpy defs.
If I find time I'll make a new PR such that we can be independent off numpys version in this regard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments topic-typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use NumPy's SupportsDType
3 participants