adds model_name option in ConfigDict #6814

romulocollopy · 2023-07-23T20:43:46Z

Change Summary

Adds model_name attribute to ConfigDict. This will set the cls.__name__

Related issue number

Fix #6304

Checklist

The pull request title is a good summary of the changes - it will be used in the changelog
Unit tests for the changes exist
Tests pass on CI
Documentation reflects the changes where applicable
My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

sydney-runkle · 2024-01-12T15:13:09Z

@romulocollopy,

This generally looks good - would you want to rebase + open this PR for review? We're likely going to release 2.6.0b1 next week, so if you'd like, we could get this new feature in that release 🚀.

codspeed-hq · 2024-01-12T15:43:31Z

CodSpeed Performance Report

Merging #6814 will not alter performance

_{Comparing romulocollopy:feature/6304-override-model-name (48e2068) with main (2e459bb)}

Summary

✅ 10 untouched benchmarks

sydney-runkle · 2024-01-12T16:27:34Z

@romulocollopy, is this ready for review now? If so, could you please move it out of draft state?

romulocollopy · 2024-01-12T16:31:17Z

@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

sydney-runkle · 2024-01-12T16:42:20Z

@romulocollopy, I think our contribution guidelines might be able to help with linter config: https://docs.pydantic.dev/latest/contributing/

sydney-runkle

Just a few comments / requests, looking good thus far!

pydantic/config.py

pydantic/v1/schema.py

sydney-runkle · 2024-01-12T21:19:06Z

tests/test_config.py

@@ -492,6 +493,66 @@ def my_function():
        pass


+def test_config_model_name() -> None:


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.

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?

Viicos · 2024-01-13T14:53:14Z

pydantic/_internal/_model_construction.py

@@ -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


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

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?

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

sydney-runkle · 2024-01-15T13:17:47Z

@romulocollopy,

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>

romulocollopy · 2024-01-16T02:00:40Z

pydantic/v1/schema.py

+        try:
+            model_name = model.model_config["model_name"]
+        except AttributeError:
+            model_name = normalize_name(model.__name__)


I could try something like this if it is preferable:

Suggested change

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__)

)

romulocollopy · 2024-01-16T14:20:42Z

pydantic/config.py

+        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'}


I'll revert the formating. I have to turn off my black when saving.

sydney-runkle

@romulocollopy,

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.

Viicos · 2024-01-16T14:34:10Z

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

sydney-runkle · 2024-01-16T14:35:40Z

If the class name gets changed, I'll have to update my PR once this is merged. But as said #6814 (comment), 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 model_name and title, then? https://docs.pydantic.dev/latest/api/config/#pydantic.config.ConfigDict.title

Update: I guess the JSON schema refs differentiate this logic from that of just the simple title setting

Viicos · 2024-01-16T14:49:03Z

Aaah, now I'm a bit confused, what use case having model_name is trying to solve where title is not enough (in v2)?

Looking at the code base, I see it is being used in GenerateSchema._model_schema.

I'm also a bit concerned about having the __name__ attribute changed, e.g. what happens in this function?

pydantic/pydantic/_internal/_typing_extra.py

Lines 195 to 198 in 5de67f7

    
           def get_cls_types_namespace(cls: type[Any], parent_namespace: dict[str, Any] | None = None) -> dict[str, Any]: 
        
               ns = add_module_globals(cls, parent_namespace) 
        
               ns[cls.__name__] = cls 
        
               return ns

Update: I guess the JSON schema refs differentiate this logic from that of just the simple title setting

Ah that might it

sydney-runkle · 2024-01-16T14:52:56Z

@Viicos,

I agree, we shouldn't change the __name__ attribute (as I had originally suggested).

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 get_defs_ref function of the GenerateJsonSchema class will probably be the best approach.

romulocollopy force-pushed the feature/6304-override-model-name branch from c81e78f to 1ef95b6 Compare July 23, 2023 20:51

romulocollopy mentioned this pull request Jul 23, 2023

Allowing overriding of model names in get_model_name_map #6304

Open

13 tasks

romulocollopy force-pushed the feature/6304-override-model-name branch 3 times, most recently from a6b7bc0 to b8e8294 Compare July 23, 2023 21:25

sydney-runkle added awaiting author revision relnotes-feature labels Jan 12, 2024

Rômulo Collopy added 2 commits January 12, 2024 10:39

adds model_name option in ConfigDict

25aea57

allows TypeModelSet to accept subclasses of BaseModel

0fd7fe2

romulocollopy force-pushed the feature/6304-override-model-name branch from b8e8294 to a0740b3 Compare January 12, 2024 15:40

add tests for model_name conflict

090985b

romulocollopy force-pushed the feature/6304-override-model-name branch from a0740b3 to 090985b Compare January 12, 2024 20:48

sydney-runkle requested changes Jan 12, 2024

View reviewed changes

pydantic-hooky bot assigned romulocollopy Jan 12, 2024

Viicos reviewed Jan 13, 2024

View reviewed changes

sydney-runkle mentioned this pull request Jan 15, 2024

🚀 2.6.0b1 checklist ✅ #8549

Closed

10 tasks

romulocollopy and others added 2 commits January 15, 2024 11:00

Update pydantic/config.py

e8af284

Co-authored-by: Sydney Runkle <54324534+sydney-runkle@users.noreply.github.com>

wip

5b32e4f

romulocollopy marked this pull request as ready for review January 15, 2024 18:45

wip: get model_name from model_config

48e2068

romulocollopy commented Jan 16, 2024

View reviewed changes

sydney-runkle requested changes Jan 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds model_name option in ConfigDict #6814

adds model_name option in ConfigDict #6814

romulocollopy commented Jul 23, 2023 •

edited

sydney-runkle commented Jan 12, 2024 •

edited

codspeed-hq bot commented Jan 12, 2024 •

edited

sydney-runkle commented Jan 12, 2024

romulocollopy commented Jan 12, 2024

sydney-runkle commented Jan 12, 2024

sydney-runkle left a comment

sydney-runkle Jan 12, 2024

romulocollopy Jan 15, 2024

Viicos Jan 13, 2024

romulocollopy Jan 15, 2024

Viicos Jan 15, 2024

sydney-runkle commented Jan 15, 2024

romulocollopy Jan 16, 2024

romulocollopy Jan 16, 2024

sydney-runkle left a comment

Viicos commented Jan 16, 2024

sydney-runkle commented Jan 16, 2024 •

edited

Viicos commented Jan 16, 2024 •

edited

sydney-runkle commented Jan 16, 2024

		@@ -492,6 +493,66 @@ def my_function():
		pass


		def test_config_model_name() -> None:

adds model_name option in ConfigDict #6814

Are you sure you want to change the base?

adds model_name option in ConfigDict #6814

Conversation

romulocollopy commented Jul 23, 2023 • edited

Change Summary

Related issue number

Checklist

sydney-runkle commented Jan 12, 2024 • edited

codspeed-hq bot commented Jan 12, 2024 • edited

Merging #6814 will not alter performance

Summary

sydney-runkle commented Jan 12, 2024

romulocollopy commented Jan 12, 2024

sydney-runkle commented Jan 12, 2024

sydney-runkle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sydney-runkle commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sydney-runkle left a comment

Choose a reason for hiding this comment

Viicos commented Jan 16, 2024

sydney-runkle commented Jan 16, 2024 • edited

Viicos commented Jan 16, 2024 • edited

sydney-runkle commented Jan 16, 2024

romulocollopy commented Jul 23, 2023 •

edited

sydney-runkle commented Jan 12, 2024 •

edited

codspeed-hq bot commented Jan 12, 2024 •

edited

sydney-runkle commented Jan 16, 2024 •

edited

Viicos commented Jan 16, 2024 •

edited