google.cloud.firestore lacks packaged typing

[nipunn1313]

Hi.
google.cloud.firestore does not have types via PEP561
google.cloud.firestore_v1 and friends do.

Per the PEP:

This PEP does not support distributing typing information as part of module-only distributions. The code should be refactored into a package-based distribution and indicate that the package supports typing as described above.

You'll have to refactor firestore.py into a directory structure

google/
  cloud/
    firestore/
      __init__.py
      py.typed 

Repro steps:

# ~/test/test.py
# firestore doesn't have py.typed yet - only firestore_v1 has
from google.cloud.firestore import AsyncClient
from google.cloud.firestore_v1 import AsyncClient

python3 -m venv venv
source venv/bin/activate
pip install google-cloud-firestore
# or `pip install -e ~/src/python-firestore` for a local checkout
mypy --python-executable=venv/bin/python test.py

[tseaver]

google.cloud.firestore is just a backward-compatibility shim: it doesn't define any types at all, except for the (spurious, to me) typing of __all__ as List[str] -- who needs typing for a literal?

If that List[str] is causing actual, rather than theoretical breakage, I'd rather just remove it.

[nipunn1313]

Hiya - thanks for looking
The contents of firestore.py aren't causing any issues - it's all totally fine.

Take a look at #448 which would fix this by renaming firestore.py to firestore/__init__.py and adding a py.typed. The runtime behavior will be fully equivalent (shim will still work).

It's just that according to PEP561, typing information is not distributed to module-only distributions (eg firestore.py). It is only distributed via package distribution (eg firestore/__init__.py and firestore/py.typed). google.cloud.firestore_v1 obeys PEP561 for example.

Type checkers (mypy / pyright) will not currently locate types for google.cloud.firestore - because mypy/pyright obey PEP561. For example

from google.cloud.firestore import AsyncClient
reveal_type(AsyncClient)  # will reveal as `Any` until #448 lands.

[tseaver]

@nipunn1313 firestore.py is not a "module-only distribution" -- it is a module within the namespace package google-cloud, bundled as part of the google-cloud-firestore distribution, which is decidedly a "package distribution."

Because firestore.py has no meaningful typing of its own, it doesn't need a py.typedfile -- if it *did* require one, it could be along-sidefirestore.py` in the namespace package.

[nipunn1313]

Thanks for the clarification!

Because firestore.py has no meaningful typing of its own, it doesn't need a py.typed file

firestore.py does reexports - which I believe type checkers (mypy/pyright) consider to be meaningful types. Without a py.typed - the typecheckers aren't even going to look in there to understand that the reexports exist. I did a quick test and confirmed this w/ both mypy and pyright.

from google.cloud.firestore import AsyncClient as AC1
from google.cloud.firestore_v1 import AsyncClient as AC2
reveal_type(AC1)
reveal_type(AC2)

mypy:

test.py:2: error: Skipping analyzing "google.cloud.firestore": found module but no type hints or library stubs
test.py:2: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
test.py:5: note: Revealed type is "Any"
test.py:6: note: Revealed type is "def (project: Any =, credentials: Any =, database: Any =, client_info: Any =, client_options: Any =) -> google.cloud.firestore_v1.async_client.AsyncClient"
Found 1 error in 1 file (checked 1 source file)

pyright

Found 1 source file
/Users/nipunn/src/pyright_google_repro/test.py
  /Users/nipunn/src/pyright_google_repro/test.py:5:13 - info: Type of "AC1" is "Unknown"
  /Users/nipunn/src/pyright_google_repro/test.py:6:13 - info: Type of "AC2" is "Type[AsyncClient]"

I think there might be some ambiguity in the terminology and spec of PEP561 here - particularly around requirements for modules within namespace packages. I'm in the process of trying to clarify this python/peps#2083, so any feedback appreciated. See this comment microsoft/pyright#2113 (comment) for some of the intention

typecheckers rely on a py.typed file inside the module directory to consider the package inline typed (eg google/cloud/firestore_v1/py.typed). PEP 561 expressly forbids having py.typed in the namespace package - for good reason, because the namespace package (google/cloud in this case) is shared across multiple distributions.

I don't think the PEP explains how to indicate types for something like google/cloud/firestore.py. It says:

For namespace packages (see PEP 420), the py.typed file should be in the submodules of the namespace, to avoid conflicts and for clarity.

This PEP does not support distributing typing information as part of module-only distributions. The code should be refactored into a package-based distribution and indicate that the package supports typing as described above.

My inference here - is that it's encouraging google/cloud/firestore.py to be inlined typed by factoring into google/cloud/firestore/__init__.py with google/cloud/firestore/py.typed.

[nipunn1313]

Hi @tseaver - the PEP-561 was just updated (by me) (python/peps#2083) to clarify this case.

It now states:

For namespace packages (see PEP 420), the py.typed file should be in the submodules of the namespace, to avoid conflicts and for clarity.

This PEP does not support distributing typing information as part of module-only distributions or single-file modules within namespace packages.

The single-file module should be refactored into a package and indicate that the package supports typing as described above.

here - firestore.py is a single file module within a namespace package (google.cloud) which should be refactored into a package (firestore/__init__.py to indicate that the package supports typing. #448 should do this.

[tseaver]

I'm against a change which would make google/cloud/firestore.py a package, purely for a theoretical typing win. There is no interesting typing information in google/cloud/firestore.py -- any type checker which cannot figure out the types of rexported symbols isn't worth humoring. Even the typing for __all__ is silly / redundant -- it is a literal constant, of interest only to tools (and the REPL) which all know that it must be a sequence of strings.

[nipunn1313]

Appreciate you for promptly taking a look and replying. I don't really have a vested interest in the issue (I merely work on mypy and mypy-protobuf and noticed the issue in my travels), so I won't push further. I am confused by your logic, but it's not my judgement call to make.

any type checker which cannot figure out the types of rexported symbols isn't worth humoring. Even the typing for all is silly / redundant -- it is a literal constant, of interest only to tools (and the REPL) which all know that it must be a sequence of strings.

I do want to clarify for future readers that the two major typecheckers (mypy and pyright) are operating exactly to spec here. They certainly can figure out the types of reexported symbols - but they are expected to avoid investigating packages without py.typed (not even opening the files) - per PEP-561. The typing for the __all__ is not really relevant to the issue - but rather the entire module (including reexports) won't be opened, investigated, and typed.

If you (future reader) find that you don't have types and you'd like them - avoid the firestore shim per tseaver's message above. Concretely speaking AC1 will be typed as Any/Unknown until this issue is resolved - because the type checkers aren't looking at firestore as a typed package. If you're looking for types - AC2 will work properly.

from google.cloud.firestore import AsyncClient as AC1
from google.cloud.firestore_v1 import AsyncClient as AC2
reveal_type(AC1)  # Any
reveal_type(AC2)  # Type[AsyncClient]

[nipunn1313]

I would note that the example usage in your README won't get types since it does from google.cloud import firestore

[crwilcox]

#476 improved on types, but we have a bit of a gap to close yet.

currently the code as merged is validating the package, google.cloud.firestore.

We want to evaluate mypy against google and tests

Once https://www.github.com/googleapis/gapic-generator-python/issues/1026 is addressed, we should be unblocked to wrap this up.