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

fix(automl): fix TablesClient.predict for array and struct #9991

Merged
merged 2 commits into from
Jan 28, 2020

Conversation

helinwang
Copy link
Contributor

@helinwang helinwang commented Dec 17, 2019

The Predict request payload is proto. Previously Python dict is
automatically converted to proto. However, the conversion failed for
list item and dict item. Changing the structure of the Python dict might
fix the problem. However, this PR fixes the problem by generating the
proto message directly. So there is no auto conversion step.

FIXES #9887

@helinwang helinwang requested a review from sirtorry December 17, 2019 23:36
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 17, 2019
@helinwang helinwang force-pushed the prediction_fix branch 2 times, most recently from edb9a2a to f58f6b1 Compare December 17, 2019 23:43
@helinwang helinwang changed the title fix(automl): fix TablesClient.predict for array and struct. fix(automl): fix TablesClient.predict for array and struct Dec 17, 2019
The Predict request payload is proto. Previously Python dict is
automatically converted to proto. However, the conversion failed for
google.protobuf.ListValue and google.protobuf.Struct. Changing the
structure of the Python dict might fix the problem. However, this PR
fixes the problem by generating the proto message directly. So there
is no auto conversion step.

FIXES googleapis#9887
@helinwang
Copy link
Contributor Author

Friendly ping @busunkim96

Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

LGTM with one small nit about dependencies

automl/setup.py Outdated Show resolved Hide resolved
protobuf is already a transitive dependency (see google-api-core's
setup.py)
parthea pushed a commit that referenced this pull request Oct 21, 2023
* fix(automl): fix TablesClient.predict for list and struct

The Predict request payload is proto. Previously Python dict is
automatically converted to proto. However, the conversion failed for
google.protobuf.ListValue and google.protobuf.Struct. Changing the
structure of the Python dict might fix the problem. However, this PR
fixes the problem by generating the proto message directly. So there
is no auto conversion step.

FIXES #9887

* Address comment: remove protobuf dependency requirement.

protobuf is already a transitive dependency (see google-api-core's
setup.py)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TablesClient: predict does not work with Python list or dictionary
4 participants