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

Pyright regression with dagster metaclass config #7556

Closed
Hunterlige opened this issue Mar 25, 2024 · 4 comments
Closed

Pyright regression with dagster metaclass config #7556

Hunterlige opened this issue Mar 25, 2024 · 4 comments
Labels
addressed in next version Issue is fixed and will appear in next published version bug Something isn't working

Comments

@Hunterlige
Copy link

Describe the bug
For a fix version of dagster, updating pyright from 1.1.352 to pyright 1.1.355 makes dagster.Config type checking fail. Pyright reports reportUnknownMemberType for each field of the config. Mypy does not report any error.

Code or Screenshots

import pydantic
import dagster

class ConfigA(pydantic.BaseModel):
    string_pydantic: str = "test"

class ConfigB(dagster.Config):
    string_dagster: str = "test"

def function(a=ConfigA, b=ConfigB):
    a.string_pydantic
    b.string_dagster

dagster, version 1.6.13

  • with pyright 1.1.355:
    error: Type of "string_dagster" is unknown (reportUnknownMemberType)
  • with pyright 1.1.352:
    no errors
  • with mypy:
    no errors

VS Code extension or command-line
pyright 1.1.355 using the command-line tool

@Hunterlige Hunterlige added the bug Something isn't working label Mar 25, 2024
@erictraut
Copy link
Collaborator

Pyright 1.1.353 improved support for instance variables within metaclasses. In this particular case, the dagster.Config class is using a metaclass named BaseConfigMeta. It derives from a class named ModelMetaclass. The dagster library uses a dynamic try/except block to import the ModelMetaclass symbol, which means that pyright cannot determine which class is intended here.

try:
    # Pydantic 2.x
    from pydantic.main import ModelMetaclass
except ImportError:
    # Pydantic 1.x
    from pydantic._internal._model_construction import ModelMetaclass  # type: ignore

The result is that the class hierarchy for the BaseConfigMeta class contains Unknown types in its MRO (from the perspective of static type evaluation). Because pyright is now faithfully honoring the metaclass, it results in an Unknown type evaluation for string_dagster in your example above.

There are three potential fixes for this issue:

  1. The maintainers of dagster fix their dynamic loading of ModelMetaclass such that the intention is unambiguous for a static type checker.
  2. I add some better heuristics to pyright to better resolve the case where a base class is dynamically loaded within a try/except block.
  3. I change pyright's metaclass lookup logic to ignore the case where the metaclass MRO contains an Unknown.

I will start with 3, since that would restore the pre-1.1.353 behavior. I think some combination of 1 or 2 would also be a good idea. Ideally, static type checkers wouldn't need to use heuristics to guess at the intent of a library author, so 1 is a good idea regardless.

erictraut added a commit that referenced this issue Mar 25, 2024
…he lookup if the metaclass MRO contains an unknown class. This addresses #7556.
@erictraut
Copy link
Collaborator

I've implemented 3, which works around the issue. As I mentioned above, it would be advisable for the dagster maintainers to also address 1.

@Hunterlige
Copy link
Author

Thank you for your feedback. Created an issue in the dagster repository.

@erictraut
Copy link
Collaborator

This is addressed in pyright 1.1.356, which I just published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed in next version Issue is fixed and will appear in next published version bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants