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

Overriding default value makes __fields__ inconsistent with __annotations__ #757

Closed
kszucs opened this issue Aug 16, 2019 · 13 comments · Fixed by #758
Closed

Overriding default value makes __fields__ inconsistent with __annotations__ #757

kszucs opened this issue Aug 16, 2019 · 13 comments · Fixed by #758

Comments

@kszucs
Copy link

@kszucs kszucs commented Aug 16, 2019

Bug

Please complete:

  • OS: OSX
  • Python version import sys; print(sys.version): 3.7.3 | packaged by conda-forge | (default, Jul 1 2019, 14:38:56)
  • Pydantic version import pydantic; print(pydantic.VERSION): 0.32.1

Please read the docs and search through issues to
confirm your bug hasn't already been reported before.

Where possible please include a self contained code snippet describing your bug:

from pydantic import BaseModel


class A(BaseModel):
    value: int = 1
        
        
class B(A):
    value = 'G'


print(A.__annotations__, A.__fields__)
# ({'value': int}, {'value': <Field(value type=int default=1)>})

print(A.__annotations__, A.__fields__)
# ({'value': int}, {'value': <Field(value type=str default='G')>})

The film type should be int in both cases. __annotations__ is empty in the metaclass so a type will be inferred for the field even without a type hint.

A solution would be to take the parents' annotations into consideration as well, or to introduce another config option to prevent inferring the type of the default value as the type hint for the field. I'm not sure how would the former one affect the rest of the features.

@kszucs kszucs added the bug label Aug 16, 2019
@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Aug 17, 2019

I think I’ve noticed this before with slightly different resulting issues but thought it might just be a limitation of python. Thanks for breaking this down.

I think there doesn’t need to be a config value, parent annotations should probably just be used if they exist; if you want to override you should probably just have to do it explicitly. I’m not sure if this would be hard to implement though.

@dmontagu dmontagu mentioned this issue Aug 17, 2019
4 of 4 tasks complete
@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Aug 17, 2019

This turned out to be a surprisingly easy fix. @kszucs I agree with your interpretation of what the intended behavior should be, but I defer to @samuelcolvin .

@kszucs kszucs mentioned this issue Aug 17, 2019
1 of 4 tasks complete
@kszucs

This comment has been minimized.

Copy link
Author

@kszucs kszucs commented Aug 17, 2019

@dmontagu just submitted a PR with a different approach, yours seem more robust

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Aug 17, 2019

@kszucs yours also seems good to me, and has the benefit of only changing two lines 😄 and I thought mine was short.... I’m not sure which approach is more robust; I could see either way. We can wait for @samuelcolvin’s opinion. (I would be happy to close my PR in favor of @kszucs’s if his approach is preferable.)

@samuelcolvin

This comment has been minimized.

Copy link
Owner

@samuelcolvin samuelcolvin commented Aug 17, 2019

[removed as I misread the issue].

@samuelcolvin

This comment has been minimized.

Copy link
Owner

@samuelcolvin samuelcolvin commented Aug 17, 2019

I don't agree this is a bug, it's what I would expect to happen. However I'd consider raising an exception to make sure it's explicit.

Cancel that, I've just read through your question again and I see what you mean.

I think we should raise an eror if the subtype doesn't have an annotatoin.

@samuelcolvin samuelcolvin added the bug label Aug 17, 2019
@kszucs

This comment has been minimized.

Copy link
Author

@kszucs kszucs commented Aug 17, 2019

@samuelcolvin I think overriding the default value without altering the type hint would be useful, it enables a quite flexible declarative API:

from pydantic import BaseModel
from typing import List, Dict
from pathlib import Path


class Build(BaseModel):
    name: str
    builddir: Path
    env: Dict[str, str] = {}
    tags: List[str] = []
    

class LinuxBuild(Build):
    builddir = Path('/tmp/builddir')
    tags = ['linux']
    env = {
        'LD_LIBRARY_PATH': '/usr/local/lib'
    }
    

class OSXBuild(Build):
    builddir = Path('/var/temp/build')
    tags = ['osx']
    
    
lb = LinuxBuild(name='linux-test')
ob = OSXBuild(
    name='osx-test', 
    tags=['use', 'darwin', 'instead'], 
    env={'MACOSX_DEPLOYMENT_TARGET': '10.14'}
)
lb == <LinuxBuild name='linux-test' builddir=PosixPath('/tmp/builddir') env={'LD_LIBRARY_PATH': '/usr/local/lib'} tags=['linux']>
ob == <OSXBuild name='osx-test' builddir=PosixPath('/var/temp/build') env={'MACOSX_DEPLOYMENT_TARGET': '10.14'} tags=['use', 'darwin', 'instead']>

It works with #758 but fails with the current master.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

@samuelcolvin samuelcolvin commented Aug 17, 2019

I understand and use this myself.

The problem comes when the type on the child-model is different from the type on the parent model, eg. int vs .'G' in your above case - it's in this case where I think we should raise an error. Does that make sense?

@kszucs

This comment has been minimized.

Copy link
Author

@kszucs kszucs commented Aug 17, 2019

@samuelcolvin Dataclass permits to override the annotation in child classes:

from dataclasses import dataclass

@dataclass
class Uni:
    name: str = 'A'
    position: int = 1
        

@dataclass
class Byt(Uni):
    name: bytes = b'A'
    position: int = 1

Uni.__annotations__ == {'name': str, 'position': int}
Byt.__annotations__ == {'name': bytes, 'position': int}

Are you referring at, that the new default value might have different type than the annotation in the parent class?
Because a current behaviour of pydantic allows it:

from pydantic import BaseModel


class A(BaseModel):
    value: int = 1
        
        
class B(A):
    value: int = 'G'


B()
<B value='G'>
@kszucs

This comment has been minimized.

Copy link
Author

@kszucs kszucs commented Aug 17, 2019

Another example with more specific types down in the hierarchy:

class Car:
    pass


class SportCar(Car):
    pass


class CarStore(BaseModel):
    cars: List[Car]
    affordable: bool = True
        
        
class SportCarStore(CarStore):
    cars: List[SportCar]
    affordable: bool = False
@samuelcolvin

This comment has been minimized.

Copy link
Owner

@samuelcolvin samuelcolvin commented Aug 17, 2019

I'm not opposed to overwriting the type or annotation in child classes, I'm just saying:

  • M.__annotations__ and M.__fields__ should be consistent
  • you shouldn't be able to override the type of a field implicitly on a child class

So

from pydantic import BaseModel

class A(BaseModel):
    value: int = 1

class B(A):
    value: str = 'G'

Would be fine,

But

from pydantic import BaseModel

class A(BaseModel):
    value: int = 1

class B(A):
    value = 'G'

Would raise an exception, something like "The type of B.value is implicitly changed by the default value, if you wish to change the type of this field, please use a type annotation: value: str = ...".

@kszucs

This comment has been minimized.

Copy link
Author

@kszucs kszucs commented Aug 17, 2019

@samuelcolvin sure, I agree with that :) - actually it would be great to have a validation for that.

@dmontagu

This comment has been minimized.

Copy link
Collaborator

@dmontagu dmontagu commented Aug 17, 2019

I updated #758 to reflect this implementation, but there are some minor tradeoffs; I'll comment on them there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.