Skip to content

Fix pyright error when importing from distributed#6904

Merged
crusaderky merged 1 commit intodask:mainfrom
ianliu:main
Aug 24, 2022
Merged

Fix pyright error when importing from distributed#6904
crusaderky merged 1 commit intodask:mainfrom
ianliu:main

Conversation

@ianliu
Copy link
Copy Markdown
Contributor

@ianliu ianliu commented Aug 18, 2022

Consider the following snippet of code:

from distributed import Client

When type-checking with pyright, this gives an error stating that "Client" is not exported from module "distributed". This is because pyright is following PEP 484, where I extracted the following excerpt:

Modules and variables imported into the stub are not considered exported from the stub unless the import uses the import ... as ... form or the equivalent from ... import ... as ... form

Although distributed is not a stub, this rule is being applied because of the py.typed file.

Closes #6903

  • Tests added / passed

Where are the tests? The CONTRIBUTING.md tells me to run py.test, but I couldn't find how.

  • Passes pre-commit run --all-files

@GPUtester
Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 18, 2022

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   6h 36m 31s ⏱️ - 9m 6s
  3 041 tests ±0    2 953 ✔️  - 2    83 💤 ±0  5 +2 
22 493 runs  ±0  21 511 ✔️  - 5  977 💤 +3  5 +2 

For more details on these failures, see this check.

Results for commit 8c62e7c. ± Comparison against base commit 2a2c3bb.

♻️ This comment has been updated with latest results.

Copy link
Copy Markdown
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ianliu. We're using mypy for type checking. cc @crusaderky for thoughts

@crusaderky
Copy link
Copy Markdown
Collaborator

The paragraph you mention from PEP 484 was written before py.typed existed, and it specifically refers to stub files. PEP 561, which introduces py.typed, does not clarify, so it's up to interpretation.

My personal interpretation is that this is extremely unreadable, and I don't think that treating .py files as stubs just because of py.typed is a good policy. IMHO you should open an upstream issue in pyright.

@ianliu
Copy link
Copy Markdown
Contributor Author

ianliu commented Aug 19, 2022

My personal interpretation is that this is extremely unreadable

I think this is unreadable as well, so I will consider opening an issue on pyright.

I was also searching for another perspective, and pyright errors go away if the symbols are put in the __all__ list as well. Do you think that defining __all__ makes sense for this case?

@crusaderky
Copy link
Copy Markdown
Collaborator

Yes, __all__ would be a bit verbose but definitely cleaner.

@crusaderky
Copy link
Copy Markdown
Collaborator

Opened upstream ticket: python/peps#2768
Could you open a ticket on pyright and link it on the python/peps ticket above?

@gjoseph92
Copy link
Copy Markdown
Collaborator

I also have found this annoying. I'd prefer __all__; it seems a bit more readable to me as well.

@ianliu
Copy link
Copy Markdown
Contributor Author

ianliu commented Aug 23, 2022

Sorry for the delay. I've opened this issue microsoft/pyright#3843 on pyright, and Eric responded with this link: https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#library-interface

I will change the patch to include an __all__ constant.

@ianliu
Copy link
Copy Markdown
Contributor Author

ianliu commented Aug 23, 2022

@crusaderky I was searching the typing-sig mailing list and found this thread: https://mail.python.org/archives/list/typing-sig@python.org/thread/YLJPWECBNPD2K4TRIBRIPISNUZJCRREY/#C2ZZKEM3EESWMDUF3PYZB4ZG2L42XUZT

The thread was started by Eric Traut himself, the author of Pyright, and his concern is exactly how to determine public symbols in py.typed packages. I think the documentation I've linked above is the result of the discussion on the thread.

Consider the following snippet of code:

```python
from distributed import Client
```

When type-checking with pyright, this gives an error stating that
_"Client" is not exported from module "distributed"_. This is because
pyright is following PEP 484, where I extracted the following excerpt:

> Modules and variables imported into the stub are not considered
> exported from the stub unless the import uses the `import ... as ... form`
> or the equivalent `from ... import ... as ... form`

Although `distributed` is not a stub, this rule is being applied because
of the `py.typed` file, according to the following document:

  https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#library-interface
@crusaderky crusaderky merged commit 599708e into dask:main Aug 24, 2022
@crusaderky
Copy link
Copy Markdown
Collaborator

Thank you

gjoseph92 pushed a commit to gjoseph92/distributed that referenced this pull request Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Importing from distributed shows pyright error

5 participants