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
allow for shallow copies #4093
allow for shallow copies #4093
Conversation
please review |
Thanks so much. I'm just moving house at the moment, but hopefully will find time to review this in the next few days. |
@samuelcolvin no worries! pinning 1.9.0 is a workaround for now (or setting |
You need to change the base for this PR to |
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.
Also need to add something to the documentation.
At this stage, I think most people have worked around this, I think best we leave behaviour as it is. |
We have not worked around it in Home Assistant. Instead, we blacklisted this release since it is a breaking change. Introducing breaking changes in a bugfix revision and then not fixing them is not a good way to handle versioning. |
I understand this isn't ideal @AngellusMortis, the breaking change wasn't intentional. As far as I can tell, this change wasn't a simple fix for the original problem, it introduced new settings, also as you mentioned it introduced a new breaking change. If you (or anyone else) can create a fix for the issue, I'll happily review it and try to get this fixed. Otherwise I'll try to get to it soon. |
Even if this PR is canceled, as long as it is fixed is the important part. Introducing a breaking change in a bugfix versions and then not fixing it erodes the trust in the package. It would be good enough reason for me to consider an alternative solution as I do not want to use a package whose response to semver is "oh there was a breaking change, oh well, I am sure people will work around it". |
This PR was a compromise solution that kept the breaking change and added in extra functionality to allow users to revert to the old behavior. Simply changing the functionality back will itself be a breaking change (no doubt released in a patch version), so I recommend not doing this and going with this PR instead. |
Again, I'm really sorry about this. Finding time to work on pydantic when I'm not paid by a big company to do so has been hard, hence I've had to prioritise. Since I've been working on it full time, I've been mostly concentrating on preparing for V2. One of the big reason to rewrite pydantic from the ground up in V2 is to make the codebase easier to work with so we can reduce the chance of breaking changes in minor or patch releases. Again, we try to avoid breaking changes as much as possible. |
Ok, please comment if there's anything you think needs changing and I'll
try to merge tomorrow and get a new release out.
Is there anything else which should go in v1.9.2?
…On Mon, 8 Aug 2022, 22:01 Eric Jolibois, ***@***.***> wrote:
FWIW @samuelcolvin <https://github.com/samuelcolvin> I agree with
@timkpaine <https://github.com/timkpaine> that this PR has the right
approach now (it is what I would have done #4093 (comment)
<#4093 (comment)>)
In v2 we'll go with explicit calls of copy or deepcopy but in the meantime
having this literal is probably the best to be 1.9.0 compliant
—
Reply to this email directly, view it on GitHub
<#4093 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA62GGNINKQ273N6OKWTRFLVYFYRZANCNFSM5WNCJPPA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
One for your list - starlite is an API framework that heavily leverages pydantic. |
Okay I think this is fixed and ready to be deployed. @AngellusMortis @PrettyWood please can you review and confirm you're happy? |
see #4359, moving discussion to there. |
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.
Except the default value and the note looks good! Thank you!
changes/4093-timkpaine.md
Outdated
@@ -0,0 +1,3 @@ | |||
Allow for shallow copies of attributes, adjusting the behavior of #3642 | |||
`Config.copy_on_model_validation` is now a str enum of `["none", "deep", "shallow"]` corresponding to | |||
not copying, deep copy, shallow copy. |
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.
My main concern is on the default value
With the test example
class User(BaseModel):
hobbies: list[str]
my_user = User(hobbies=['scuba diving'])
class Transaction(BaseModel):
user: User
t = Transaction(user=my_user)
print(t.user is my_user)
print(t.user.hobbies is my_user.hobbies)
we used to have in v1.9.0
copy_on_model_validation = True (default): False, True
copy_on_model_validation = False: True, True
which got changed (the famous breaking change) in 1.9.1
copy_on_model_validation = True (default): False, False <-💥
copy_on_model_validation = False: True, True
We now have with this patch
copy_on_model_validation = 'deep' : False, False
copy_on_model_validation = 'shallow': False, True
copy_on_model_validation = 'none': True, True
I wonder if we should write a note saying a breaking change has been introduced by mistake in a release version and that the old 1.9.0 behaviour has been fixed. In which case, the default value should be 'shallow'
.
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 agree, we should make the default shallow
.
I'll try to fix this today.
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.
default changed.
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.
LGTM! Thanks a lot
@samuelcolvin we'll need to add a note at the start of the release note for this issue and patch
Thanks for the review, will do. I'll do the release tomorrow. |
* allow for shallow copies * Add changes file * tweak change * update for comments * rename attr * use single quotes * bump ci * add warning if not a string, switch to string literals * fix linting, prompt ci * fix ci * extend and fix tests * change default to "shallow" Co-authored-by: Samuel Colvin <s@muelcolvin.com>
* generate history from changes, uprev * Pydantic V2 blog (#4218) * first draft of pydantic V2 blog * more blog * blog rendering and formatting * more section * completing conversion table * prompt build * reviewing blog post * more reviewing and extending * recommendations from @Rabscuttler and @PrettyWood * add implementation details and more suggestions * comment about breaking changes * convert namespae to table, more removals * Apply suggestions from code review by @tiangolo Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com> * feedback from @tiangolo's review * changes from @adriangb's review * Apply suggestions from code review Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com> * convert namespace info to psuedo-code * rename property, remove schema_json() * adding validation context * remove 'model_schema_json', take 2 * more tweaks while reviewing * comment about pypy and tagged unions * add thanks :prey:, prepare for release * suggestions from @PrettyWood * suggestions from @PrettyWood, model_dump_json comment Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com> Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com> * comments mostly from @PrettyWood (#4226) * comments mostly from @PrettyWood * add updated comment * fix pre-commit * allow for shallow copies (#4093) * allow for shallow copies * Add changes file * tweak change * update for comments * rename attr * use single quotes * bump ci * add warning if not a string, switch to string literals * fix linting, prompt ci * fix ci * extend and fix tests * change default to "shallow" Co-authored-by: Samuel Colvin <s@muelcolvin.com> * uprev and prepare for release * linting Co-authored-by: Sebastián Ramírez <tiangolo@gmail.com> Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com> Co-authored-by: Tim Paine <t.paine154@gmail.com>
Looks like this PR reproduce the issue #3641 from pydantic import BaseModel
from typing import List
class A(BaseModel):
name: str
class B(BaseModel):
list_a: List[A]
a = A(name="a")
b = B(list_a=[a])
print(a)
print(b)
a.name = "b" # make a change to variable a's field
print(a)
print(b) Output in 1.6.2, 1.7.4, 1.8.1, 1.8.2, and 1.9.1,
Output in 1.9.0 and 1.9.2,
Is this the expected change of behaviour from previous version to v1.9.x? |
@NunchakusLei yes, this PR restores state to some combination of 1.9.0 and 1.9.1 behavior, but lets you pick between them. from the docs:
So you'll want |
See #4369, I need to update the docs. |
Do you know if the 1.8.2 and previous are equivalent to |
pre 1.9.1 was default shallow or "none", 1.9.1 was default deep or none, 1.9.2 is none/deep/shallow |
@timkpaine Base on my test #4093 (comment), pre 1.9.0 was default deep or none (tested in 1.6.2, 1.7.4, 1.8.1, and 1.8.2). The test result conflicting the statement of,
I got very confuse now so want to clarify. |
On my phone but I think:
|
This doesn't matched my test result in 1.6.2, 1.7.4, 1.8.1, and 1.8.2. #4093 (comment) |
@NunchakusLei your tests results might be wrong, but also...it doesnt really matter at this point, all behaviors are supported now. |
See: https://github.com/pydantic/pydantic/blob/v1.8.2/pydantic/main.py#L731 pretty sure I'm correct. |
class M(BaseModel):
class Config:
copy_on_model_validation = "!! Some non sens str !!" Does not raise any error. This is probably due to the fact that deprecation warning is handled. if copy_on_model_validation not in {'deep', 'shallow', 'none'}:
# Warn about deprecated behavior
warnings.warn(
"`copy_on_model_validation` should be a string: 'deep', 'shallow' or 'none'", DeprecationWarning
)
if copy_on_model_validation:
deep_copy = False |
Change Summary
Related issue number
Fix #4092.
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)