Skip to content
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

Existing tensorflow stubs modules and classes can not infered by pyright/pylance? #11052

Closed
alkatar21 opened this issue Nov 21, 2023 · 10 comments

Comments

@alkatar21
Copy link

alkatar21 commented Nov 21, 2023

I just tried the stubs with the types-tensorflow package with VSCode and pyright(I use pylance) and don't quite understand why in the following example both layers and Dense are Incomplete?
grafik
As this is implemented:

class Dense(Layer[tf.Tensor, tf.Tensor]):

Originally posted by @alkatar21 in #7144 (comment)

@alkatar21
Copy link
Author

If anybody has any specific issues with our tensorflow stubs, please feel free to open a followup issue.

I'd prefer moving the discussion to a new issue. That being said, I wonder if pyright is picking up def __getattr__(name: str) -> Incomplete: ... as overriding explicit re-exports
https://github.com/python/typeshed/blob/main/stubs/tensorflow/tensorflow/keras/__init__.pyi#L14
(Untested here, on my phone)

Originally posted by @Avasam in #7144 (comment)

@alkatar21 alkatar21 changed the title Existing modules and classes are not infered in tensorflow stubs by pyright? Existing tensorflow stubs modules and classes can not infered by pyright? Nov 21, 2023
@srittau
Copy link
Collaborator

srittau commented Nov 21, 2023

This simple script confirms this:

import tensorflow as tf

reveal_type(tf)
reveal_type(tf.keras)

mypy prints:

foo.py:3: note: Revealed type is "types.ModuleType"
foo.py:4: note: Revealed type is "types.ModuleType"

While pylance (which uses pyright under the hood, as far as I know) reports:

Der Typ von "tf" ist "Module("tensorflow")".
Der Typ von "tf.keras" ist "Unknown".

(Please ignore the German error messages. For some reason the pylance developers though it was a good idea to translate error messages, without an apparent way to configure the language used.)

Maybe @erictraut can shed some light on this issue.

@erictraut
Copy link
Contributor

Hmm, I'm not able to repro the problem if I install the types-tensorflow stubs (pip install types-tensorflow).

image

I am able to repro the behavior reported by the OP.

Pyright is evaluating the expression tf.keras.layers as type Incomplete because of the __getattr__ function defined in the keras/__init__.pyi stub. That same stub also includes the statement from tensorflow.keras import layers as layers. I'm not sure what this statement is intended to do because it seems to be attempting to import its own module. Pyright accordingly reports an error for this statement and ignores it.

@hmc-cs-mdrissi
Copy link
Contributor

hmc-cs-mdrissi commented Nov 21, 2023

The intent of from tensorflow.keras import layers as layers was to allow

import tensorflow as tf

reveal_type(tf.keras.layers) # Module

When I remove explicit re-export locally then pylance gives me an error there of,

"layers" is not a known member of module "tensorflow.keras"Pylance[reportGeneralTypeIssues](https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportGeneralTypeIssues)

My own local stubs don't have getattr as for typeshed I leave incomplete stubs with getattr to avoid giving false positives, but my personal/work usage I tend to be stricter and take false positives instead of false negatives for tensorflow. Although adding 1 getattr to tensorflow/keras/__init__.pyi doesn't seem to make a difference for me in pylance but my local stubs differ fair amount as they're still more complete then open-sourced ones.

@erictraut
Copy link
Contributor

Digging into this further, I think there is a bug in pyright here. It appears to be in some code added by the pylance team to support some language server features, and it affects stub packages where the "py.typed" file indicates they are "partial" stubs. I'll work with the pylance team to figure out what's going on here.

@alkatar21
Copy link
Author

Thanks @erictraut.

Apart from the issue in pyright, I also thought of something else:
@hmc-cs-mdrissi

My own local stubs don't have getattr as for typeshed I leave incomplete stubs with getattr to avoid giving false positives, but my personal/work usage I tend to be stricter and take false positives instead of false negatives for tensorflow. Although adding 1 getattr to tensorflow/keras/init.pyi doesn't seem to make a difference for me in pylance but my local stubs differ fair amount as they're still more complete then open-sourced ones.

This is the first time I've consciously tried to deal with partial stubs. I'm not quite sure whether I understand PEP561 correctly, haven't looked at how typecheckers have implemented it and haven't had time to look at how other partial stubs on typeshed deal with it, but I'm not quite sure if __getattr__ is that optimal?
If I understand the first two paragraphs in https://peps.python.org/pep-0561/#partial-stub-packages correctly, for installed partial stubs the parts that are present from the stubs are used and any modules that are missing are inferred from the actual sources. I am unsure whether __getattr__ might interfere with this?

@alkatar21 alkatar21 changed the title Existing tensorflow stubs modules and classes can not infered by pyright? Existing tensorflow stubs modules and classes can not infered by pyright/pylance? Nov 22, 2023
@hmc-cs-mdrissi
Copy link
Contributor

getattr is necessary as partial applies at level of separate files. It allows files to be entirely missing and fallback to remaining search. But a module in partial stub package like tensorflow/keras/__init__.pyi if it does not have getattr will be treated as complete for that module and any accesses to methods/classes not mentioned would become an error. It would be sometimes useful if there was a partial mechanism for modules.

This comment touches that idea. That would likely need discussion here. I'm unsure it's good choice for tensorflow though just because tensorflow abuses import system heavily. For a more normal large library I could see partial stub modules being useful.

Currently tensorflow stubs have many partial files (happy to review contributions) so getattr is necessary to avoid false positives.

@heejaechang
Copy link

heejaechang commented Nov 27, 2023

@alkatar21
according to this - https://pypi.org/project/types-tensorflow/ -, types-tensorflow is partial stub, so I assume you installed tensorflow package to your environment? pylance/pyright don't ship tensorflow in our bundled typeshed

without package installed, partial stub won't work.

@erictraut
Copy link
Contributor

@heejaechang, I provided detailed repro steps in this issue. If you weren't able to repro the problem following those steps, please respond in that issue, and we can work together to figure out why you're seeing something different.

@srittau
Copy link
Collaborator

srittau commented Nov 28, 2023

As this seems like a Pylance issue (microsoft/pylance-release#5139), I'm closing this here. Thanks to everyone for analyzing this!

@srittau srittau closed this as not planned Won't fix, can't repro, duplicate, stale Nov 28, 2023
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

No branches or pull requests

5 participants