-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
adds model_name option in ConfigDict #6814
adds model_name option in ConfigDict #6814
Conversation
c81e78f
to
1ef95b6
Compare
a6b7bc0
to
b8e8294
Compare
This generally looks good - would you want to rebase + open this PR for review? We're likely going to release |
b8e8294
to
a0740b3
Compare
CodSpeed Performance ReportMerging #6814 will not alter performanceComparing Summary
|
@romulocollopy, is this ready for review now? If so, could you please move it out of draft state? |
@sydney-runkle I changed some formatting accidentally, I will check the linters configuration and push again to open the PR |
@romulocollopy, I think our contribution guidelines might be able to help with linter config: https://docs.pydantic.dev/latest/contributing/ |
a0740b3
to
090985b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments / requests, looking good thus far!
@@ -492,6 +493,66 @@ def my_function(): | |||
pass | |||
|
|||
|
|||
def test_config_model_name() -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm missing something here, but isn't the only thing that we really need to focus on testing just that the name of the model when assigned via model_name
is correct? Why even bring in get_model_name_map
?
You'll definitely want to run a test or two with model names not equivalent to what they would by default be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal of this PR was to address #6304, but considering this comment, maybe it is better to not touch the cls.__name__
and change the behaviour of get_model_name_map
by making it check a new attribute with the model_name
.
I would rework a bit the implementation in this direction and keep this test. What do you think?
@@ -88,6 +88,7 @@ def __new__( | |||
base_field_names, class_vars, base_private_attributes = mcs._collect_bases_data(bases) | |||
|
|||
config_wrapper = ConfigWrapper.for_model(bases, namespace, kwargs) | |||
cls_name = config_wrapper.config_dict.get('model_name') or cls_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to have some kind of "backup" of the original class name if overridden by config.model_name
? I'm making use of cls.__name__
here, and comparing it to AST-parsed file.
In fact it would be great to have this PR merged before #6563, I will then be able to rebase on top of it and add this "backup" field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if cls.__name__
is used somewhere else, maybe it would be better to keep its original behaviour and create an extra attribute for the model_name that will explicitly interact with the v1.schema
functions. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this seems better: __name__
is a Python implementation detail and people should expect it to be the same as defined in the source code (it might also be used by documentation tools). Perhaps you can define a specific attribute that will be used for schema generation
Feel free to move this out of draft when you have a chance 👍 |
Co-authored-by: Sydney Runkle <54324534+sydney-runkle@users.noreply.github.com>
try: | ||
model_name = model.model_config["model_name"] | ||
except AttributeError: | ||
model_name = normalize_name(model.__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could try something like this if it is preferable:
try: | |
model_name = model.model_config["model_name"] | |
except AttributeError: | |
model_name = normalize_name(model.__name__) | |
model_name = ( | |
model.model_config["model_name"] if not isinstance(model, Enum) | |
else normalize_name(model.__name__) | |
) |
another_enum: Optional[SomeEnum] = Field( | ||
default=SomeEnum.FOO, validate_default=True | ||
) | ||
|
||
model1 = SomeModel(some_enum=SomeEnum.BAR) | ||
print(model1.model_dump()) | ||
#> {'some_enum': 'bar', 'another_enum': 'foo'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll revert the formating. I have to turn off my black when saving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this. I know that the initial issue was filed with V1 in mind, but I think we primarily need to focus on V2 here. We're no longer making changes to V1 unless they're security oriented - we're not adding new features.
I think the changes you had to the _model_construction.py
file with this addition were on the right track:
cls_name = config_wrapper.config_dict.get('model_name') or cls_name
@Viicos, in order for this to be helpful for your #6563 PR, do you need any changes to V1 made?
Let me know if you have any follow up questions.
If the class name gets changed, I'll have to update my PR once this is merged. But as said here, it might be better to not set the actual class name to something else, and instead define a custom attribute that will be used when generating the JSON schema. If we go with the later, then no changes will be required in my PR |
Hmm, what is the difference between Update: I guess the JSON schema refs differentiate this logic from that of just the simple |
Aaah, now I'm a bit confused, what use case having Looking at the code base, I see it is being used in I'm also a bit concerned about having the pydantic/pydantic/_internal/_typing_extra.py Lines 195 to 198 in 5de67f7
Ah that might it |
I agree, we shouldn't change the I think a better solution here will be supporting a hook for changing the defs ref for a class. @alexmojaki seemed to be on the right track here: #6304 (comment). More specifically, having some sort of dunder value that specifies what to use in the |
Tidying up PRs -- closing this as stale for now, but we can certainly reopen if it makes sense down the line! |
Change Summary
Adds
model_name
attribute toConfigDict
. This will set thecls.__name__
Related issue number
Fix #6304
Checklist