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

mypy error: Variable "watchdog.observers.Observer" is not valid as a type #982

Open
JohnStrunk opened this issue Mar 21, 2023 · 4 comments

Comments

@JohnStrunk
Copy link

JohnStrunk commented Mar 21, 2023

Since the release of 3.0.0, I'm getting errors from mypy when I use Observer as a type in my function definitions:

from watchdog.observers import Observer

def myfn(obs: Observer) -> None:
    pass

myobs = Observer()
myfn(myobs)
> mypy zz.py
zz.py:3: error: Variable "watchdog.observers.Observer" is not valid as a type  [valid-type]
zz.py:3: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
Found 1 error in 1 file (checked 1 source file)

> python --version
Python 3.11.0

Platform: Windows

@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 22, 2023

Thanks @JohnStrunk for such a clean report :)

@altendky would it be possible to have a look 🙏🏻 ?

@altendky
Copy link
Contributor

Yep, thanks for the clear report. I see how what I added fixed one issue but not another and it needs to be fixed. I think this may involve some other changes in approach elsewhere. Let's look at this a bit and start with considering just the Linux case as an example.

if sys.platform.startswith("linux"):
try:
from .inotify import InotifyObserver as Observer
except UnsupportedLibc:
from .polling import PollingObserver as Observer

So on Linux I think we want the hint to applications to be Union[Type[InotifyObserver], Type[PollingObserver]] since the intent, as I read it, is to have the library prefer inotify when possible but for the application to be able to work properly with either in case inotify is not available. We can't presently hint that because we can't even get the InotifyObserver type unless inotify is actually available. I guess at hint time mypy wouldn't care about the exception, but it would be good to separate the type availability from the ability to use it.

if (
not hasattr(libc, "inotify_init")
or not hasattr(libc, "inotify_add_watch")
or not hasattr(libc, "inotify_rm_watch")
):
raise UnsupportedLibc(f"Unsupported libc version found: {libc._name}")

So we will want to adjust this so that the mechanism for checking availability doesn't block access to the class. Then we can import both possibilities and provide a hint covering them both before identify which to actually use and assigning it to watchdog.observers.Observer.

I'll try to make some time for this but I disappeared due to some more urgent activities at work. I do still plan to come back and work on both tests here and more hinting. I expect hinting more ourselves would uncover issues like this in CI so that users don't have to hit them after we release. Sorry for the hassle @JohnStrunk.

Side note, we should add mypy checking for both BSD and 'unknown' systems since we have code trying to handle them. That can be done by passing a --platform to mypy.

I just tried to do a bit of a hacky workaround but ran into some issues. I'll let you know if I come up with something. Let me know if you do. :]

@altendky
Copy link
Contributor

Maybe this is a relevant minimal example of the issue.

https://mypy-play.net/?mypy=1.1.1&python=3.11&gist=08674707b9b4b878a182dc85a145c91f

from typing import Type, Union


class Base:
    ...

class M(Base):
    ...

class N(Base):
    ...

# a runtime condition that mypy should not resolve
runtime_condition: bool

C: Union[Type[M], Type[N]]
D: Type[Base]

if runtime_condition:
    C = M
    D = M
else:
    C = N
    D = N

ia: C
ib: D

if runtime_condition:
    E = M
else:
    E = N

ic: E
main.py:26: error: Variable "__main__.C" is not valid as a type  [valid-type]
main.py:26: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
main.py:27: error: Variable "__main__.D" is not valid as a type  [valid-type]
main.py:27: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
main.py:32: error: Cannot assign multiple types to name "E" without an explicit "Type[...]" annotation  [misc]
main.py:32: error: Incompatible types in assignment (expression has type "Type[N]", variable has type "Type[M]")  [assignment]
Found 4 errors in 1 file (checked 1 source file)

@nhairs
Copy link

nhairs commented May 23, 2023

I've also encountered this issue.

Doing some digging it looks like a lot of the problem is stemming from the fact that Observer is not actually a type but a type alias. I'll be honest, reading the docs on the matter does my head in a little bit, so maybe you'll have better luck.

I also feel like trying to narrow the type of Observer when type checking is a mistake as the type checker can't actually guarantee the actual type of Observer at runtime (proof being that you can coerce the platform in MyPy). If you can indeed guarantee the platform at runtime you shouldn't be importing Observer but instead the concrete implementation that you will be using.

As such I believe the correct type of Observer should actually be watchdog.oberservers.api.BaseObserver as this is the only typing information you can guarantee at runtime.
(While I'm not sure I fully understand it, it feels like attempting to narrow the type is breaking Liskov's Substitution Principal)

I was also wondering if Protocols might help, but it looks like that is what you are already attempting which might be the cause of the problem? (watchdog.observers.__init__ ref, watchdog.observers.api ref)

Alternatively, while the "black-magic" of replacing Observer with the best implementation based on platform is super cool. Perhaps it's actually too much black-magic and instead we should be using a function that returns the "best" implementation. (Explicit is better than implicit)

def get_best_observer_class() -> watchdog.observers.api.BaseObserver:
    "Get the best observer class available for your platform"
    if sys.platform.startswith("linux"):
        ...

Work around for libraries depending on Watchdog

The work-around for code calling watchdog is to replace watchdog.observers.Observer with watchdog.obersvers.api.BaseObserver in your type hinting.

Rixxan added a commit to EDCD/EDMarketConnector that referenced this issue Jul 23, 2023
Also updates Observer type hints based on gorakhargosh/watchdog#982
C1701D added a commit to EDCD/EDMarketConnector that referenced this issue Jul 23, 2023
#2031 Remove EDDB Code
# Added
- Imported temporary type hints per gorakhargosh/watchdog#982

# Changed
- A few defaults for URL settings
- Updated Dependencies to latest working versions
- Minor documentation updates

# Removed
- Removes EDDB Plugin and References
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants