-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[BUGFIX] zep - always serialize type field #7056
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
👇 Click on the image for a new way to code review
Legend |
# type should be in dumped str even if not explicitly set | ||
assert f"type: {asset.type}" in source_str |
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.
Note. These 3 test_postgres_datasource.py
tests are the only ones that I could actually get to catch the error.
This only happens when an asset is created in code and no type
field is set.
I couldn't reproduce it with any of the config tests (as expected).
), | ||
], | ||
) | ||
def test_type_field_always_dumped(ds_w_assets: dict): |
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.
This test didn't actually catch the error so it's of questionable value.
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.
Is this because you are using a dictionary with the type
field defined and checking that the type field roundtrips?
If you don't think this adds value then feel free to remove it.
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.
It's because the only way to reproduce the type
field not being serialized error is when adding assets "in-code". Because that's the only place where the default type
field value is used. And because the default is used, pydantic doesn't consider the field as having been "set".
So when we dump the model with exclude_unset=True
it gets left out.
https://docs.pydantic.dev/usage/exporting_models/#modeldict
Note in the above docs it says exclude_unset
defaults to False
, but we've changed this behavior to default to True
When we read from config (dict, yaml or json) everything (including type
) gets loaded into the DataAsset
class constructer and so pydantic considers it set
, that's why the test_config.py
tests didn't catch the issue.
So.... I'll delete these new test_config.py
tests and leave the changes to test_postgres_datasource.py
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.
Thank you! LGTM. One question responding to your comment.
), | ||
], | ||
) | ||
def test_type_field_always_dumped(ds_w_assets: dict): |
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.
Is this because you are using a dictionary with the type
field defined and checking that the type field roundtrips?
If you don't think this adds value then feel free to remove it.
#7009 changed the default behavior of all ZEP class serialization methods to exclude any fields that haven't been explicitly set.
Without this change, we would end up with configs full of default field values.
However, we always need the value of
type
in order to determine the correct asset class/type to use.Changes proposed in this pull request:
Datasource
andDataAsset
type
field