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

195 migrate to pydantic v2 #199

Merged
merged 50 commits into from
Nov 29, 2023
Merged

195 migrate to pydantic v2 #199

merged 50 commits into from
Nov 29, 2023

Conversation

djs0109
Copy link
Contributor

@djs0109 djs0109 commented Jul 13, 2023

@tstorek I met the first difficulty while doing the migration. Previously, you defined the QueryStatement and QueryStatement in simple_ql.py, and now they cause the issues: pydantic.errors.PydanticSchemaGenerationError: Unable to generate pydantic-core schema for <class 'filip.utils.simple_ql.QueryString'>. Do you have an Idea about how to fix that?

@tstorek
Copy link
Collaborator

tstorek commented Jul 13, 2023

@djs0109 I have to look into it.

@tstorek
Copy link
Collaborator

tstorek commented Jul 13, 2023

@djs0109 did you run the migration tool on the tests as well? Otherwise I will add it very quickly

@djs0109
Copy link
Contributor Author

djs0109 commented Jul 13, 2023

Yes, I run it already. These parts are not automatically migrated. I guess the reason is that they are not inherited from the pydantic.BaseModel

@tstorek
Copy link
Collaborator

tstorek commented Jul 13, 2023

@djs0109 I will need a little more time. I will go ahead but will require a little for it. The trouble is probably in pydantic-settings but pydantiv itself.

@tstorek
Copy link
Collaborator

tstorek commented Jul 14, 2023

@djs0109 Hi, i looked into it bit. I think that regex expressions are still an issue. There is an open issue in bump-pydantic pydantic/bump-pydantic#80. Lets see, what will happen within the next days :)

@tstorek tstorek mentioned this pull request Jul 14, 2023
@djs0109
Copy link
Contributor Author

djs0109 commented Sep 29, 2023

The migration is mostly done. All tests pass also in CI-runner. @tstorek do you have time to review it?

Copy link
Collaborator

@tstorek tstorek left a comment

Choose a reason for hiding this comment

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

Hi, thanks for that big effort. I found some stuff. Please double check on them

@@ -385,9 +386,11 @@ def get_entity_list(self,
params=params,
headers=headers)
if AttrsFormat.NORMALIZED in response_format:
return parse_obj_as(List[ContextEntity], items)
ta = TypeAdapter(List[ContextEntity])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please follow the notation in the migration guide: https://docs.pydantic.dev/2.0/migration/#introduction-of-typeadapter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change the variable name from ta to adapter just as in the documentation.

filip/clients/ngsi_v2/cb.py Outdated Show resolved Hide resolved
@@ -92,7 +93,7 @@ def post_groups(self,

url = urljoin(self.base_url, 'iot/services')
headers = self.headers
data = {'services': [group.dict(exclude={'service', 'subservice'},
data = {'services': [group.model_dump(exclude={'service', 'subservice'},
exclude_none=True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

adapt indentation

filip/config.py Outdated
env_file_encoding = 'utf-8'
case_sensitive = False
validation_alias=AliasChoices('QUANTUMLEAP_URL', 'QL_URL'))
model_config = SettingsConfigDict(env_file='.env.filip', env_file_encoding='utf-8',
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider to move to the top of the class. To make it easier to access

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you describe more about this point? I could not get your suggestion

Copy link
Collaborator

Choose a reason for hiding this comment

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

Model config should be directly below the class doc-string to have a better overview about whats going on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already fixed

@@ -25,7 +27,8 @@ class Http(BaseModel):
"also be supported."
)

@validator('url', allow_reuse=True)
@field_validator('url')
@classmethod
def check_url(cls, value):
return validate_http_url(url=value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this stil work with the new Url-pattern?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use the new validation pattern for reusing the validator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What new URL-pattern do you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check on here: This seems far easier:) https://docs.pydantic.dev/latest/api/networks/

Copy link
Contributor Author

@djs0109 djs0109 Nov 16, 2023

Choose a reason for hiding this comment

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

If I replace L23 with url: HttpUrl or url: AnyHttpUrl, the http data can not be passed into the data model with the syntax Subscription(**sub_dict)
That is a good hint 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hence, to change or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already updated, please check the latest commit

filip/models/ngsi_v2/base.py Outdated Show resolved Hide resolved
filip/semantics/semantics_models.py Show resolved Hide resolved
filip/semantics/semantics_models.py Outdated Show resolved Hide resolved
filip/semantics/semantics_models.py Outdated Show resolved Hide resolved
filip/semantics/semantics_models.py Show resolved Hide resolved
Copy link
Contributor Author

@djs0109 djs0109 left a comment

Choose a reason for hiding this comment

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

@tstorek Thanks for the reviewing effort! I went through your comments. Please check again, if it looks ok for you

Copy link
Collaborator

@tstorek tstorek left a comment

Choose a reason for hiding this comment

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

@djs0109 There are jsut minor things. I prefer to have the model config as first entry a pydantic model. That's what I orginally meant with my comment

@@ -25,7 +27,8 @@ class Http(BaseModel):
"also be supported."
)

@validator('url', allow_reuse=True)
@field_validator('url')
@classmethod
def check_url(cls, value):
return validate_http_url(url=value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check on here: This seems far easier:) https://docs.pydantic.dev/latest/api/networks/

filip/semantics/semantics_models.py Show resolved Hide resolved
@djs0109
Copy link
Contributor Author

djs0109 commented Nov 16, 2023

@tstorek sure! I have now adjusted the location of model_config in each class

Copy link
Collaborator

@tstorek tstorek left a comment

Choose a reason for hiding this comment

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

only minor things. Max 1h of work

filip/config.py Outdated
env_file_encoding = 'utf-8'
case_sensitive = False
validation_alias=AliasChoices('QUANTUMLEAP_URL', 'QL_URL'))
model_config = SettingsConfigDict(env_file='.env.filip', env_file_encoding='utf-8',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Model config should be directly below the class doc-string to have a better overview about whats going on

@@ -25,7 +27,8 @@ class Http(BaseModel):
"also be supported."
)

@validator('url', allow_reuse=True)
@field_validator('url')
@classmethod
def check_url(cls, value):
return validate_http_url(url=value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hence, to change or not?

filip/models/ngsi_v2/context.py Outdated Show resolved Hide resolved
filip/models/ngsi_v2/context.py Outdated Show resolved Hide resolved
default=None,
description="Endpoint where the device is going to receive commands, "
"if any."
)
@field_validator('endpoint')
@classmethod
def validate_endpoint(cls, value):
Copy link
Collaborator

Choose a reason for hiding this comment

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

filip/models/ngsi_v2/subscriptions.py Outdated Show resolved Hide resolved
filip/models/ngsi_v2/timeseries.py Outdated Show resolved Hide resolved
filip/models/ngsi_v2/timeseries.py Outdated Show resolved Hide resolved
@@ -22,10 +25,10 @@ def validate_http_url(url: AnyHttpUrl) -> str:
Returns:
validated url
"""
return url
return str(url) if url else url
Copy link
Collaborator

Choose a reason for hiding this comment

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

tests/clients/test_mqtt_client.py Outdated Show resolved Hide resolved
@tstorek
Copy link
Collaborator

tstorek commented Nov 28, 2023

@djs0109 i'm sorry somehow the latest commit did not show up

@tstorek tstorek self-requested a review November 29, 2023 15:21
Copy link
Collaborator

@tstorek tstorek left a comment

Choose a reason for hiding this comment

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

Whoop Whoop. Thanks a lot! Let#s go for it. Don#t forget the Changelog ;)

@djs0109 djs0109 merged commit f5f1d9f into master Nov 29, 2023
1 check passed
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.

3 participants