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

Add generic functionality #595

Merged
merged 24 commits into from Jun 19, 2019
Merged

Add generic functionality #595

merged 24 commits into from Jun 19, 2019

Conversation

dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Jun 14, 2019

Change Summary

Adds some functionality around generics.

There are definitely some edge cases not perfectly handled yet, but I wanted to get your take on this before I invest more effort. A number of people from the FastAPI community seem interested in this functionality, but if you think it's going to be too hard to maintain then we can let it go.

For what it's worth, I think the create_model function is actually doing 90% of the work necessary to get this working.

Related issue number

#556

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>

@dmontagu
Copy link
Collaborator Author

@dmontagu dmontagu commented Jun 14, 2019

I think it would be hard to get this working in python 3.6 due to differences in the way generics work. But I could look into it if necessary.

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 14, 2019

I'll look at this further, but overall looks cool.

I don't think 3.6 matters, just 3.7 support for this is fine.

Please use single quotes, I'll do my best to add linting for single quotes somehow soon.

@codecov
Copy link

@codecov codecov bot commented Jun 14, 2019

Codecov Report

Merging #595 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #595   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2582   2544   -38     
  Branches      510    504    -6     
=====================================
- Hits         2582   2544   -38

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Good work, basically this looks good to me.

pydantic/generics.py Outdated Show resolved Hide resolved
pydantic/generics.py Outdated Show resolved Hide resolved
pydantic/generics.py Outdated Show resolved Hide resolved
tests/test_generics.py Outdated Show resolved Hide resolved
tests/test_generics.py Outdated Show resolved Hide resolved
tests/test_generics.py Show resolved Hide resolved
pydantic/generics.py Outdated Show resolved Hide resolved
from pydantic.class_validators import gather_validators


class GenericModel:
Copy link
Collaborator Author

@dmontagu dmontagu Jun 14, 2019

Choose a reason for hiding this comment

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

I figured GenericModel was an appropriate name but I can change it back if desired.

pydantic/generics.py Outdated Show resolved Hide resolved
pydantic/generics.py Outdated Show resolved Hide resolved
pydantic/generics.py Outdated Show resolved Hide resolved
tests/test_generics.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@drunkwcodes
Copy link

@drunkwcodes drunkwcodes commented Jun 15, 2019

Can it be some constraints on Generics, e.g. "not iterable"?
It is less expressive without type constraints and losts the purpose with type constraints.

@dmontagu
Copy link
Collaborator Author

@dmontagu dmontagu commented Jun 15, 2019

Can it be some constraints on Generics, e.g. "not iterable"?
It is less expressive without type constraints and losts the purpose with type constraints.

@drunkwcodes I'm not sure exactly what you mean. The primary (intended) value here is that you can more easily generate lots of models with a different BaseModel (or not, but seems more likely to be useful with many BaseModels) as a specific attribute, without having to declare a separate model for each attribute-class you want to contain.

Think:

class Response(GenericModel, Generic[T])
    data: T
    error: ErrorModel

then all you have to do is write

Response[int]
Response[str]
Response[MyModel]

and you get the corresponding type without having to declare it.

@drunkwcodes
Copy link

@drunkwcodes drunkwcodes commented Jun 15, 2019

@dmontagu It is useful when you want to construct a list with *args and ensure there is no iterable in them.

@dmontagu
Copy link
Collaborator Author

@dmontagu dmontagu commented Jun 15, 2019

Hmm, I didn't notice this because mypy doesn't run over the tests by default (which is where most of my usages were), but it looks like mypy doesn't like type variables if you don't explicitly inherit from Generic (or another typing class). So it looks like it may be necessary to change back to

class Model(GenericModel, Generic[T, S]):
    ...

I'll get that version working again, with all of these improvements.

@dmontagu
Copy link
Collaborator Author

@dmontagu dmontagu commented Jun 15, 2019

Okay, everything worked out nicer than expected:

  • I was able to inherit from BaseModel without any problems, which helps mypy with all the built-in BaseModel methods.
  • If we want to support differing numbers of type variables (which I think we should), I think it's necessary that generic classes also inherit from Generic[T] or similar. This appears to be imposed by typing since you can't create your own custom variadic generic type yet. (This is an open issue in python/typing.)

Overall I'm very pleased with how neatly this is all fitting together, I expected typing to get in the way a lot more.

pydantic/generics.py Outdated Show resolved Hide resolved
@drunkwcodes
Copy link

@drunkwcodes drunkwcodes commented Jun 15, 2019

@dmontagu typing works sometimes. It depends.

Copy link

@drunkwcodes drunkwcodes left a comment

What if introducing Not as another _SpecialForm and make GenericModel recognize it?

__slots__ = ()

def __new__(cls, *args: Any, **kwargs: Any) -> NoReturn:
if cls is GenericModel:
Copy link

@drunkwcodes drunkwcodes Jun 15, 2019

Choose a reason for hiding this comment

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

Does it mean it is not abstract and still can not be initialized without subclassing and not useful without implementing the function by users themselves?

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 15, 2019

Looks good to me, please can you:

  • update history
  • add a section to docs
  • "Hmm, I didn't notice this because mypy doesn't run over the tests by default..." add to the mypy external tests (tests/mypy_test_success.py) to check that Generic works with mypy

@drunkwcodes I'm afraid I don't understand what you're asking/concerned about. Probably best to give a code example to demonstrate your point. If you're not sure how something will work, it would be good if you checked out this PR and ran code yourself to check, then reported back here if you've found a problem.

@drunkwcodes
Copy link

@drunkwcodes drunkwcodes commented Jun 15, 2019

@samuelcolvin The test_generic() itself is expressive enough to show how a type checking function can be faked for other implementors.

@dmontagu dmontagu changed the title Added generic functionality Add generic functionality Jun 15, 2019
pydantic/generics.py Show resolved Hide resolved
@tiangolo
Copy link
Sponsor Collaborator

@tiangolo tiangolo commented Jun 15, 2019

Great job @dmontagu! Impressive 👏👏🎉

Even the docs are great! 📝🚀

@dmontagu
Copy link
Collaborator Author

@dmontagu dmontagu commented Jun 16, 2019

One thing that might be problematic is that I am placing [ and ] in the name of the created model. This makes all the associated validation messages cleaner / easier to parse, but I could see it causing unforeseen issues with the schema-related features due to having special characters (since you won't get special characters in the name of any model declared e.g. by inheriting from BaseModel).

In particular, @tiangolo I could see this causing issues with the OpenAPI spec generated by FastAPI. Is there anything to worry about here? (Maybe there's a config setting that let's you override the name of the model used in the spec? I'm a little out of my depth here.)

@dmontagu
Copy link
Collaborator Author

@dmontagu dmontagu commented Jun 18, 2019

Okay, I tested it out, and there don't seem to be any problems with FastAPI. However, the generated names (which currently may include [, ], , ., and , violate the openapi spec, and prevent the use of openapi-generator because they don't meet the regex ^[a-zA-Z0-9\.\-_]+$.

@@ -346,6 +347,7 @@ def get_model_name_map(unique_models: Set[Type['BaseModel']]) -> Dict[Type['Base
conflicting_names: Set[str] = set()
for model in unique_models:
model_name = model.__name__
model_name = re.sub(r"[^a-zA-Z0-9.\-_]", "_", model_name)
Copy link
Collaborator Author

@dmontagu dmontagu Jun 18, 2019

Choose a reason for hiding this comment

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

@samuelcolvin I expect this line may need to be removed / changed before merging, but I tested it, and adding this regex substitution fixes all problems with generic class names for FastAPI+openapi-generator (and I imagine for most json-schema-based tools). As far as I can tell, it isn't possible to generate a name that would be modified by this regex substitution in the normal course of pydantic usage (i.e., inheriting from BaseModel), including would-be-substituted characters is a violation of the open api spec (and presumably others), and I am having a hard time imagining a scenario where you would want those characters in your model name anyway.

From what I can tell, this substitution would also have little-to-no impact on performance given it's only involved in schema generation and is a fairly cheap regex.

I'm reluctant to change the naming scheme for generic classes because the repr and error messages are so clear. At the same time, I think json schemas are one of the most natural contexts for the use of generics (and my intended use is specifically with openapi / openapi-generator), so I think it would be unfortunate if they were slightly broken for this use case.

I'd be interested in your thoughts.

Copy link
Sponsor Collaborator

@tiangolo tiangolo Jun 18, 2019

Choose a reason for hiding this comment

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

LGTM

This function is mainly/only used to generate the JSON Schemas (and it's reused by FastAPI).

I think it would be safe/ideal to apply this. And I see you are using the same regex in the OpenAPI spec ✔️

@@ -56,6 +56,8 @@ def __class_getitem__( # type: ignore
created_model.Config = cls.Config
created_model.__concrete = True # type: ignore
_generic_types_cache[(cls, params)] = created_model
if len(params) == 1:
_generic_types_cache[(cls, params[0])] = created_model
return created_model
Copy link
Collaborator Author

@dmontagu dmontagu Jun 18, 2019

Choose a reason for hiding this comment

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

You need to have the non-tuple version in the cache so cache-access can be the first line of the function (rather than tuple-making), ensuring minimum overhead. I think it makes sense to also add the tuple version to the cache here to minimize the amount of special cases (and after all it is possible to pass a single-element tuple to the __class_getitem__ function).

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

sorry, few more small things. Otherwise I think this looks great.


class GenericModel(BaseModel):
__slots__ = ()
__concrete: ClassVar[bool] = False
Copy link
Owner

@samuelcolvin samuelcolvin Jun 18, 2019

Choose a reason for hiding this comment

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

Suggested change
__concrete: ClassVar[bool] = False
__concrete__: ClassVar[bool] = False

pydantic/generics.py Outdated Show resolved Hide resolved
pydantic/generics.py Show resolved Hide resolved
@@ -131,7 +143,7 @@ class Config:
def test_generic_instantiation_error():
with pytest.raises(TypeError) as exc_info:
GenericModel()
assert str(exc_info.value) == 'Type GenericModel cannot be instantiated; it can be used only as a base class'
assert str(exc_info.value) == 'Type GenericModel cannot be used without generic parameters, e.g. GenericModel[T]'
Copy link
Collaborator Author

@dmontagu dmontagu Jun 18, 2019

Choose a reason for hiding this comment

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

This might be slightly confusing because you also have to inherit from Generic before this will work, but if you try (e.g. by doing GenericModel[int]) it will then tell you to inherit from typing.Generic. So I think it's okay.

@dmontagu
Copy link
Collaborator Author

@dmontagu dmontagu commented Jun 18, 2019

@samuelcolvin Okay, I think I addressed the points you raised. I slightly reworked where which exceptions arise in order to make the messages a little more clear. Let me know if you think it needs more work.

params = (params,)
if any(isinstance(param, TypeVar) for param in params): # type: ignore
raise TypeError(f'Type parameters should be placed on typing.Generic, not GenericModel')
if Generic not in cls.__bases__:
Copy link
Collaborator Author

@dmontagu dmontagu Jun 18, 2019

Choose a reason for hiding this comment

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

It's important to put this check for inheriting from Generic after the check for TypeVar parameterization, or the error messages end up being more confusing if you try to put type vars directly on the GenericModel (which I would expect to be a relatively common mistake).

@intrepidOlivia
Copy link

@intrepidOlivia intrepidOlivia commented Jun 18, 2019

Just curious, since I'm interested in using this feature, do we have an ETA on when this might be merged?

@drunkwcodes
Copy link

@drunkwcodes drunkwcodes commented Jun 19, 2019

I'm also curious about it but I still stand for @dmontagu's optimistic development cycle.

@samuelcolvin samuelcolvin merged commit b84df07 into samuelcolvin:master Jun 19, 2019
5 of 7 checks passed
@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 19, 2019

this is amazing, thank you very much.

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 19, 2019

@intrepidOlivia @drunkwcodes I've released a new version now with this included, should be available in pypi in a few minutes.

@intrepidOlivia
Copy link

@intrepidOlivia intrepidOlivia commented Jun 19, 2019

Awesome! Thank you!

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.

None yet

5 participants