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

Breaking Change: update model.py classes to use proto-plus instead of _pb #319

Closed
tswast opened this issue Oct 12, 2020 · 4 comments
Closed
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. semver: major Hint for users that this is an API breaking change. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@tswast
Copy link
Contributor

tswast commented Oct 12, 2020

Currently the classes in model.py use the underlying protobuf _pb object for read-only properties instead of the proto-plus object.

self._proto = types.Model()._pb

This avoided some possible breaking changes, but was mostly done to expedite the transition to the microgenerator.

There are some benefits to using proto-plus that would be worth taking advantage of. Most importantly, timestamp well-known types are properly translated into a datetime-like object with proto-plus.

Why now?

There are some breaking changes in the latest generated clients due to googleapis/gapic-generator-python#595

If we need to make a breaking change for these changes, we might as well update the classes to support the latest versions.

@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Oct 12, 2020
@tswast tswast added semver: major Hint for users that this is an API breaking change. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Oct 12, 2020
@cguardia
Copy link
Contributor

@tswast After getting a bit more work done here, I saw that routine also uses _pb, so I started to look at getting rid of it there too:

https://github.com/googleapis/python-bigquery/blob/master/google/cloud/bigquery/routine.py#L202

When the line above runs, resource is equal to {'typeKind': 'INT64'}. If I replace that line with e.g. resource = type(value).to_dict(value), resource is {'type_kind': 2}.

This is an example of the kind of under the hood name and type conversion that json_format does. There are more than a few tests that take this for granted, not only in model code, but also in client code. The problem is that some actual code outside of the tests also relies on this on occasion, so when I tried my initial approach to change to snake_case wholesale, I sometimes ended up with half a dozen more failing tests after making a change to correct the case of a single different test.

I'm working on this still, but I wanted to describe this issue that has complicated the work a bit.

@tswast
Copy link
Contributor Author

tswast commented Jul 20, 2021

Alternatively, we could switch to returning dictionaries for these BigQuery ML classes. The proto definitions aren't republished as often as I first anticipated, as it's a manual process.

@tswast
Copy link
Contributor Author

tswast commented Jul 20, 2021

Dictionaries would be a worse experience from an IDE and docs perspective, though.

@tswast
Copy link
Contributor Author

tswast commented Jul 26, 2021

Closing in favor of #814

@tswast tswast closed this as completed Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. semver: major Hint for users that this is an API breaking change. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants