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

tickets/DM-40289: Add support for extra annotations on pod #148

Merged
merged 8 commits into from
Aug 14, 2023

Conversation

athornton
Copy link
Member

Needed for T&S and their bridged-networking magic.

@athornton
Copy link
Member Author

There's some commentary about why we need to ignore typing for two tests. Strangely, I can't reproduce the bug with a minimal class that does the same thing, so it's at least a little subtle.

@athornton
Copy link
Member Author

Here's the test case which fails to fail like jupyterlab-controller does.

First, the requirements.txt for the virtualenv you should run it in:

mypy==1.4
pytest

And here's bug_test.py, which should be run with pytest:

import pytest
from enum import Enum


class MyEnum(Enum):
    INITIAL = "initial"
    FINAL = "final"


class EnumFlipper:
    def __init__(self) -> None:
        self.state = MyEnum.INITIAL

    def finalize(self) -> None:
        self.state = MyEnum.FINAL


def test_bug() -> None:
    obj = EnumFlipper()
    assert obj.state == MyEnum.INITIAL
    obj.finalize()
    assert obj.state == MyEnum.FINAL

But this behaves exactly as you'd expect:

adam@m1-wired:~/Documents/src/mypy-bug$ pytest .
============================= test session starts ==============================
platform darwin -- Python 3.11.4, pytest-7.4.0, pluggy-1.2.0
rootdir: /Users/adam/Documents/src/mypy-bug
collected 1 item

bug_test.py .                                                            [100%]

============================== 1 passed in 0.00s ===============================
(mypybug)
adam@m1-wired:~/Documents/src/mypy-bug$ mypy .
Success: no issues found in 1 source file
(mypybug)

@rra
Copy link
Member

rra commented Aug 7, 2023

Type narrowing on object attributes in mypy is always a challenge. mypy does want to narrow the discovered type, since otherwise it will get other types of false positives, but it doesn't know whether calling a method could change the value of the attribute, and both decisions can cause problems. They've been struggling with finding the right heuristics for a while. I'm guessing that your test case works because it's all in one file, so mypy has more global knowledge of what the methods do.

See python/mypy#11969 for example.

I worked around this mypy limitation and updated the dependencies in the pending neophile PR so that I could merge it, so you should be able to drop all of the changes to the test suite and rebase on main now.

@rra
Copy link
Member

rra commented Aug 7, 2023

Oh, even more likely than it being all in one file, your example isn't a dataclass, which I suspect changes the way mypy thinks about attributes. (Classes like dataclasses and Pydantic models are more likely to be immutable, and thus calls to apparently unrelated methods are less likely to change the attribute values than a regular class.)

@rra
Copy link
Member

rra commented Aug 7, 2023

Oh, and sorry, that was ambiguous: by drop changes to the test suite, I just meant the ones working around the mypy limitation, not the new tests you added (which are fine).

@@ -12,6 +12,7 @@ asgi-lifespan
coverage[toml]
httpx-sse
mypy
# mypy==1.3
Copy link
Member

Choose a reason for hiding this comment

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

This can now be removed.

@@ -155,6 +155,7 @@ def test_collection() -> None:
SystemRandom().shuffle(shuffled_images)
assert images[0].aliases == set()
collection = RSPImageCollection(shuffled_images)
# Same mypy error as above in test_resolve_alias
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is no longer relevant.

@athornton athornton merged commit 90a4dc8 into main Aug 14, 2023
5 checks passed
@athornton athornton deleted the tickets/DM-40289 branch August 14, 2023 20:07
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.

2 participants