Skip to content

[DOM-56006] Regenerated the clients #142

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 24, 2024
Merged

[DOM-56006] Regenerated the clients #142

merged 4 commits into from
Apr 24, 2024

Conversation

@ddl-joyce-zhao
Copy link
Contributor

@ddl-joyce-zhao ddl-joyce-zhao commented Apr 3, 2024 •
edited
Loading

Description

Regenerated the clients with the new version of openapi-python-client. Here is its release history:

https://github.com/openapi-generators/openapi-python-client/releases

To have the compatible pydantic, the openapi-python-client has to be equal to or greater than 0.15.1 where the pydantic was upgraded to V2.
https://github.com/openapi-generators/openapi-python-client/releases/tag/v0.15.1

Noticeable breaking changes that affects our generated clients include
https://github.com/openapi-generators/openapi-python-client/releases/tag/v0.15.0

Useful discussion about the new type of clients:
openapi-generators/openapi-python-client#775 (comment)

Related Issue

https://dominodatalab.atlassian.net/browse/DOM-56006

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

Checklist

  • I've read the CONTRIBUTING.md guide.
  • I've updated the code style using make codestyle.
  • I've written tests for all new methods and classes that I created.
  • I've written the docstring in Google format for all the methods and classes that I used.

@github-actions
Copy link

github-actions bot commented Apr 3, 2024 •
edited
Loading

✅ Result of Pytest Coverage

---------- coverage: platform linux, python 3.10.14-final-0 ----------

Name Stmts Miss Cover
domino_data/init.py 7 2 71%
domino_data/_feature_store/init.py 0 0 100%
domino_data/_feature_store/client.py 43 3 93%
domino_data/_feature_store/exceptions.py 9 0 100%
domino_data/_feature_store/git.py 43 1 98%
domino_data/_feature_store/logging.py 7 0 100%
domino_data/_feature_store/run.py 16 16 0%
domino_data/_feature_store/sync.py 98 9 91%
domino_data/auth.py 100 14 86%
domino_data/configuration_gen.py 232 0 100%
domino_data/data_sources.py 290 24 92%
domino_data/logging.py 10 0 100%
domino_data/meta.py 22 0 100%
domino_data/training_sets/init.py 0 0 100%
domino_data/training_sets/client.py 135 10 93%
domino_data/training_sets/model.py 42 0 100%
domino_data/transfer.py 37 0 100%
domino_data/vectordb.py 23 4 83%
TOTAL 1114 83 93%

~ 67 passed in 13.33s ~

@ddl-joyce-zhao ddl-joyce-zhao requested review from a team and ddl-gabrielhaim April 3, 2024 16:58
@ddl-joyce-zhao ddl-joyce-zhao marked this pull request as ready for review April 10, 2024 14:21
@ddl-mcetin ddl-mcetin changed the title Regenerated the clients DOM-56006 Regenerated the clients Apr 17, 2024
@ddl-mcetin ddl-mcetin changed the title DOM-56006 Regenerated the clients [DOM-56006] Regenerated the clients Apr 17, 2024
ddl-eric-jin
ddl-eric-jin approved these changes Apr 22, 2024
View reviewed changes
Copy link
Collaborator

@ddl-eric-jin ddl-eric-jin left a comment

Choose a reason for hiding this comment

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

Given that most of the changes are from auto-generated code, it looks good to me! I had a few comments/questions about the changes. It also would be good to run system tests with this client if you haven't already.

datasource_api_client/models/datasource_dto_auth_type.py
AWSIAMBASICNOOVERRIDE = "AWSIAMBasicNoOverride"
AWSIAMROLE = "AWSIAMRole"
AWSIAMROLEWITHUSERNAME = "AWSIAMRoleWithUsername"
AZUREBASIC = "AzureBasic"
Copy link
Collaborator

@ddl-eric-jin ddl-eric-jin Apr 22, 2024

Choose a reason for hiding this comment

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

Nice alphabetization!

datasource_api_client/models/list_request.py

@classmethod
def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T:
from ..models.datasource_config import DatasourceConfig
Copy link
Collaborator

@ddl-eric-jin ddl-eric-jin Apr 22, 2024

Choose a reason for hiding this comment

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

I know this is auto-generated but just curious, why would they add the import inside the method?

Copy link
Contributor Author

@ddl-joyce-zhao ddl-joyce-zhao Apr 24, 2024

Choose a reason for hiding this comment

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

This is under the class method. And the data type is only used for converting from dictionary. I think the rational here is for lazy imports.

feature_store_api_client/api/default/get.py
client: Client,
) -> Dict[str, Any]:
url = "{}/".format(client.base_url)
def _get_kwargs() -> Dict[str, Any]:
Copy link
Collaborator

@ddl-eric-jin ddl-eric-jin Apr 22, 2024

Choose a reason for hiding this comment

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

Will not returning things like the headers, cookies, etc. like before cause any issues?

Copy link
Contributor Author

@ddl-joyce-zhao ddl-joyce-zhao Apr 24, 2024

Choose a reason for hiding this comment

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

Nice catch! I regenerated the client again and seems this is brought back.

datasource_api_client/models/datasource_dto.py
added_by: "DatasourceDtoAddedBy"
auth_type: DatasourceDtoAuthType
config: DatasourceConfig
config: "DatasourceConfig"
Copy link
Collaborator

@ddl-eric-jin ddl-eric-jin Apr 22, 2024

Choose a reason for hiding this comment

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

Why is there a mix of Strings and variables for the type definitions?

Copy link
Contributor Author

@ddl-joyce-zhao ddl-joyce-zhao Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Very good point. I haven't found out why it's doing so. But seeing it's widely used in open api python generator and generated code. https://github.com/openapi-generators/openapi-python-client/blob/main/openapi_python_client/parser/openapi.py#L192

Will continue checking the rational behind this.

@ddl-joyce-zhao ddl-joyce-zhao force-pushed the ddl-joyce-zhao.DOM-56006.Regenerate-clients branch from 9abdfe0 to b3fe693 Compare April 24, 2024 19:04
@ddl-joyce-zhao ddl-joyce-zhao merged commit d5e57c5 into main Apr 24, 2024
@ddl-joyce-zhao ddl-joyce-zhao deleted the ddl-joyce-zhao.DOM-56006.Regenerate-clients branch April 24, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ddl-eric-jin ddl-eric-jin ddl-eric-jin approved these changes

@ddl-gabrielhaim ddl-gabrielhaim Awaiting requested review from ddl-gabrielhaim

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ddl-joyce-zhao @ddl-eric-jin