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

feat: support default_factory with BaseModel.construct #1755

Merged
merged 4 commits into from Oct 25, 2020

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Jul 22, 2020

Change Summary

Some changes have been made on default_factory to avoid calling it for nothing and avoid some side effects in #1504 and #1712. And this line changed the default value that is used in BaseModel.construct to None.
Instead of changing __field_defaults__, which would be a mess, let's just use ModelField.get_default in construct.
The feature is hence scoped to BaseModel.construct and has the exact same behaviour as BaseModel.__init__

Related issue number

closes #1732

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #1755 into master will increase coverage by 0.15%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #1755      +/-   ##
===========================================
+ Coverage   99.84%   100.00%   +0.15%     
===========================================
  Files          42        21      -21     
  Lines        7850      3925    -3925     
  Branches     1576       788     -788     
===========================================
- Hits         7838      3925    -3913     
+ Misses         10         0      -10     
+ Partials        2         0       -2     
Impacted Files Coverage Δ
D:/a/pydantic/pydantic/pydantic/env_settings.py
D:/a/pydantic/pydantic/pydantic/networks.py
D:/a/pydantic/pydantic/pydantic/datetime_parse.py
D:/a/pydantic/pydantic/pydantic/errors.py
...:/a/pydantic/pydantic/pydantic/class_validators.py
D:/a/pydantic/pydantic/pydantic/fields.py
D:/a/pydantic/pydantic/pydantic/tools.py
D:/a/pydantic/pydantic/pydantic/mypy.py
D:/a/pydantic/pydantic/pydantic/error_wrappers.py
D:/a/pydantic/pydantic/pydantic/dataclasses.py
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8326f8...20190a0. Read the comment docs.

Copy link

@zoopp zoopp left a comment

Choose a reason for hiding this comment

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

To me this seems fine however I have a couple of questions:

I see that the only other place where __field_defaults__ is used is in BaseModel._iter() when excluding default values. Would it make sense to remove it altogether and rely on __fields__ for that?

Apart from that, was __field_defaults__ used as an optimisation to keep the number of deepcopys down? If yes and if the impact is measurable, would it be better to use get_default only for fields with default factory?

@PrettyWood
Copy link
Member Author

PrettyWood commented Jul 27, 2020

Hello @zoopp
To be honest I checked the __field_defaults__ while doing the PR and I agree it could be removed.
Right now we don't support dict(exclude_defaults) with default_factory and I wanted to remove __field_defaults__ while working on it. But maybe we could remove __field_defaults__ in this PR by just calling BaseField.default

EDIT: I added a basic commit to remove it. We can discuss whether or not we want to go even further but I'm afraid it would take us away from the issue

@zoopp
Copy link

zoopp commented Jul 27, 2020

Sure, I was only inquiring about it out of curiosity.

@zoopp
Copy link

zoopp commented Aug 24, 2020

I'm wondering if there anything I can do expedite a potential merge of this pull request. Is there any specific feedback I can provide to help with it?

@PrettyWood
Copy link
Member Author

@zoopp As Samuel is busy working, I would just fork this repo and merge the PRs you need if you can't wait an extra month

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

This looks like it could be fine, but questions:

  • could this be considered a breaking change, after all people use __fields__ a lot, they probably use __field_defaults__ too? at least I think we need a note in the change description
  • this is currently conflicting with master, could you fix
  • does this have a measurable effect on performance.

@PrettyWood
Copy link
Member Author

PrettyWood commented Oct 10, 2020

@samuelcolvin I'm not sure __field_defaults__ is really used compared to __fields__ but it's just my 2 cents.
But you are right: if we go with this approach, it needs to be added.

As for the performance question it slows down things for sure when no default_factory is set

with default_factory
# 3 loops on `master` branch with 10 ** 6 loops
5704059017ns
5680524864ns
5950163218ns

# 3 loops on this branch with 10 ** 6 loops
5811149041ns
6014411367ns
5877212718ns

without default_factory (was not supported)

# 3 loops on `master` branch with 10 ** 6 loops
1517463041ns
1480426577ns
1481139567ns

# 3 loops on this branch with 10 ** 6 loops
1648662578ns
1656794397ns
1632753169ns

Don't know if it's a big deal.

Or maybe we could change the approach by keeping __field_defaults__ as a custom dict. Something like

class FieldDefaultsDict(Dict[str, Any]):
    def add_field_default(self, key: str, field: ModelField) -> None:
        if field.default_factory:
            default_value = field.default_factory
            default_value.__default_factory__ = True  # this would need some hack as `default_value` can be a builtin
            self[key] = default_value
        else:
            self[key] = field.default

    def __getitem__(self, item: str) -> Any:
        default_value = super().__getitem__(item)
        return default_value() if getattr(default_value, '__default_factory__', False) else default_value

@samuelcolvin samuelcolvin merged commit 4bc4230 into pydantic:master Oct 25, 2020
@samuelcolvin
Copy link
Member

thanks so much. I think this is definitely better than what went before.

PrettyWood added a commit to PrettyWood/pydantic that referenced this pull request Dec 2, 2020
PrettyWood added a commit to PrettyWood/pydantic that referenced this pull request Dec 2, 2020
@PrettyWood PrettyWood deleted the construct-default-factory branch December 17, 2020 12:34
samuelcolvin pushed a commit that referenced this pull request Jan 1, 2021
… models (#2167)

* revert logic of #2143

* rewrite logic removed in #1755

* test: add test to ensure dynamic and static behaviours are the same
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.

BaseModel.construct returns instances with None assigned to fields with a default_factory
3 participants