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

BaseSettings - pylance complains for not setting default values to fields #3753

Closed
calvo-jp opened this issue Jan 30, 2022 · 10 comments · Fixed by #3972
Closed

BaseSettings - pylance complains for not setting default values to fields #3753

calvo-jp opened this issue Jan 30, 2022 · 10 comments · Fixed by #3972
Labels
bug V1 Bug related to Pydantic V1.X
Milestone

Comments

@calvo-jp
Copy link

Hi! I am a huge fan. I am just encountering an error at the moment. The code below used to have no issue with pylance using previous pydantic version. But, right now I can no longer create an instance without having to pass values to fields that are supposed to be set only in .env or loaded from environment. It is really nice that it now offers autocomplete (same goes to BaseModel) and it actually still works. It's just that I really do not like the view of squiggly lines 😄 Would there be a quick fix for this?

Screenshot from 2022-01-30 10-12-43

These are my vscode settings, btw.

"python.analysis.typeCheckingMode": "basic",
"python.languageServer": "Pylance",

Thank you in advance for your response.

@calvo-jp calvo-jp added the bug V1 Bug related to Pydantic V1.X label Jan 30, 2022
@marianhlavac
Copy link

marianhlavac commented Jan 31, 2022

Duplicate of #3617 – it's related to the use of Field()

@calvo-jp
Copy link
Author

calvo-jp commented Feb 3, 2022

I see. I never had problems with Field and BaseModel, tho. I always do it like so

class Model(BaseModel):
    optional_field1: Optional[int] = None
    optional_field2: Optional[str] = ""
    optional_field3: Optional[str] = Field(default="") # using positional arg for default here does not work

@marianhlavac
Copy link

Well, apparently not just using Field, as mentioned in #3767

crbaker89 added a commit to lexiq-legal/pydantic_schemaorg that referenced this issue Feb 17, 2022
@squaresmile
Copy link

squaresmile commented Mar 7, 2022

A workaround for this BaseSettings issue is to use = Field(default=...) for required parameter and = "literal value" or = Field(default="default") for optional parameter.

  • The required case will trick pyright into thinking the field is optional. Without the workaround, the field definition is of "required" flavor so pyright can't know that the code is still working.
  • For the optional case, it's necessary to use the keyword argument default if the function Field is used because of how dataclass_transform was implemented in pyright.

@samuelcolvin
Copy link
Member

I've emailed Eric Traut, the developer of pyright to ask about this. Hopefully he provides some way to disable dataclass transforms on subclasses of BaseSettings.

@samuelcolvin samuelcolvin added this to the v1.9.1 milestone Apr 2, 2022
@samuelcolvin
Copy link
Member

Here's the response from @erictraut about this, here's our conversation:

me:

...

I wonder if it's possible to disable dataclass transforms (or at least pyright recognising them) on some subclasses of a class which is decorated with @__dataclass_transform__?

The issue is described in #3753 - basically pydantic's BaseSettings class can be initialised from environment variables and hence "required" fields do not need to be provided when initialising the class. However since BaseSettings inherits from BaseModel, pyright and pylance are complaining that fields are missing.

Is there any way of deactivating dataclass transforms on subclasses of BaseSettings?

...

Here's Eric's response:

No, dataclass_transform doesn’t provide any such facility. That would deviate significantly from the behavior of stdlib dataclass, so I don’t think there would be much appetite for supporting it.

I would recommend to pydantic users that they don’t make use of dynamic behaviors like initialization from .env variables if they want to use pydantic with static type checking. Alternatively, they (or you) could use a static conditional (e.g. if TYPE_CHECKING) to define BaseSettings in a way that it doesn’t derive from BaseModel.

I’ve include Erik de Bonte on the thread. He’s the primary author of PEP 681, and I want to make sure he’s aware of the issue you’re raising.

We’re running a bit short on time to get PEP 681 accepted prior to the deadline for Python 3.11, but the good news is that Erik submitted the PEP to the Python steering council (SC) last Friday. We’re hoping to receive word within the next couple of weeks that it has been accepted. It’s probably too late to amend it at this time unless we identify a problem that is so big that we would be willing to risk slipping beyond 3.11.

In Summary: There's no way to disable the dataclass transform logic on classes which inherit from BaseSettings.

There are therefore two work around:

settings = Settings.parse_obj({})
# or
settings = Settings()  # pyright: ignore

I don't this is is too bad - because of pydantic's lax/coercion approach to data validation, you'll already need to do a fair bit of ignoring pyright, this is no exception.

I've created a PR #3972 to test pyright against pydantic code in CI so we can make sure the behaviour doesn't change (or at least is formalised)

@samuelcolvin
Copy link
Member

feedback welcome on #3972

@florian-sattler
Copy link

florian-sattler commented Apr 4, 2022

I think the PR is good so far, however I don't think the proposed solution

settings = Settings.parse_obj({})

makes it very obvious what is happening. Perhaps adding a dedicated method like one of the following would be handy.

Idea 1:

    @classmethod
    def from_env(cls):
        return cls.parse_obj({})

Idea 2:

    @classmethod
    def parse_env(cls):
        return cls.parse_obj({})

Idea 3:

    @classmethod
    def parse_env(cls, defaults=None):
        obj = {} if defaults is None else defaults
        return cls.parse_obj(obj)

I know adding this might be a stretch and might also introduce some issues later down the line, when the underlying issues get resolved in the future.

EDIT: These issues are not primarily part of pydantic.

@samuelcolvin
Copy link
Member

humm, this was my initial idea, however:

  1. I'm not keen to add another method just to work around an issue in a type checker - not even the most popular python type checker
  2. I think it could be even more confusing for those new to pydantic than the current behaviour "what's so special about parse_env()?"
  3. Adding new public methods to BaseModel and BaseSettings is somewhat dangerous, see this dicussion

I'd be happy to remove the settings = Settings.parse_obj({}) hack from the docs - can you comment on the PR, rather than here so I remember to change it.

@marianhlavac
Copy link

I'm not keen to add another method just to work around an issue in a type checker

Not only this has the chance to resolve the type checking issue, but some users might prefer this, as this will explicitly note that the settings object is constructed from environment variables (which might not be immediately obvious for users new to Pydantic, typing settings = Settings() might leave them thinking it will make an empty model).

Adding new public methods to BaseModel and BaseSettings is somewhat dangerous

How terrible idea it would be to keep the "from_env" method separate from the class, receiving BaseSettings instance just as an argument?

from pydantic.env_settings import build_from_env

settings = build_from_env(Settings)  # returns Settings
# or
settings = Settings()

It's not as elegant as Settings.from_env(), but also it wouldn't be the primary way to build the settings objects, the settings = Settings() would be still the recommended way to go. But of course this might confuse some users, as they might ask "should I use Settings() of build_from_env(Settings)?"

CobaltCause added a commit to team-telnyx/openapi-codegen that referenced this issue May 19, 2022
This is now the recommended way to set default values. Doing it the
previous way causes pyright to assuming you're accidentally omitting
required values. Doing it this way is somehow able to tell pyright that
these fields are actually optional/have defaults and do not need to be
specified. See [here][0] for details. Change seems to have been
introduced by [this][1].

[0]: pydantic/pydantic#3753 (comment)

[1]: pydantic/pydantic#2721
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug V1 Bug related to Pydantic V1.X
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants