Skip to content
This repository has been archived by the owner on Dec 31, 2023. It is now read-only.

fix: make TablesClient.predict permissive to input data types #13

Merged
merged 4 commits into from Mar 23, 2020

Conversation

helinwang
Copy link
Contributor

@helinwang helinwang commented Feb 18, 2020

The current implementation checks input instance's data type according
to column spec's data type. E.g., if the column spec is float, it
requires the input to be float or int, but not string. However, this
is not the same as tables API contract:

float column data type could be string or number values.

The current code raises exception with error messages like

TypeError: '0' has type str, but expected one of: int, long, float
when passed in a string value for numeric columns, which should be
allowed.

This PR changes the logic so that Python SDK side will be permissive
for the input data type - basically all JSON compatible data types are
allow. And rely on backend for the validation.

The current implementation checks input instance's data type according
to column spec's data type. E.g., if the column spec is float, it
requires the input to be float or int, but not string. However, this
is not the same as tables API contract:

   float column data type could be string or number values.

The current code raises exception with error messages like

    TypeError: '0' has type str, but expected one of: int, long, float
    when passed in a string value for numeric columns, which should be
    allowed.

This PR changes the logic so that Python SDK side will be permissive
for the input data type - basically all JSON compatible data types are
allow. And rely on backend for the validation.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 18, 2020
Copy link
Contributor

@sirtorry sirtorry left a comment

Choose a reason for hiding this comment

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

lgtm

google/cloud/automl_v1beta1/tables/tables_client.py Outdated Show resolved Hide resolved
# This check needs to happen before isinstance(value, int),
# isinstance(value, int) returns True when value is bool.
return struct_pb2.Value(bool_value=value), None
if isinstance(value, six.integer_types) or isinstance(value, float):
Copy link
Contributor

Choose a reason for hiding this comment

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

if or elif should work for all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I don't quite understand, do you mean if these if-elif statements (from line 55 to 85) covers all supported data types? Yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry poor communication. just an observation, on this line, you started a new if clause. this if can be elif OR all of the elif can be if. no change necessary.

@helinwang
Copy link
Contributor Author

@busunkim96 could you take a look? Thanks!

@helinwang helinwang merged commit ddc9f71 into googleapis:master Mar 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants