fix(automl): pass params to underlying client

[sirtorry]

passes params from tables_client to gapic prediction_service_client

[busunkim96]

LGTM with some nits. Is this something to be watching out for every time there's an update to AutoML?

[sirtorry]

LGTM with some nits. Is this something to be watching out for every time there's an update to AutoML?

Looking at previous PRs, there's certainly a precedence at this point. #9779 #9491 are examples.

[helinwang]

Non-blocking: curious if there is documentation / spec about how to how to write docstring with links.
E.g.,

• why \ in feature\_importance is needed,
• what does (-s) mean, and
• seems different from markdown, [text][[link] instead of [text][link] is used.

[busunkim96]

This formatting is from the proto comments. I believe it's some flavor of markdown that makes the comments render nicely on cloud.google.com. See https://github.com/googleapis/googleapis/tree/master/google/cloud/automl/v1 and https://cloud.google.com/automl/docs/reference/rpc/google.cloud.automl.v1.

When you need to add docstrings, please follow the Google Python Style Guide

[helinwang]

LGTM

[helinwang]

Regarding "Is this something to be watching out for every time there's an update to AutoML?".
Any recommendation of how to keep backend and Python SDK in-sync?
The currently process is manually trying the best to make sure it's in-sync.

[busunkim96]

@helinwang Would it be possible to write some tests to help catch these incompatibilities?

[helinwang]

@busunkim96 I think it's hard to do because:
New backend feature is added in piper first. The test for these features should be added during the feature development, so they have to be in piper too. If we add the test in Github, when we are adding the test, we already know the new feature is coming, so the test does not help catching new features. It only catches regressions - still good to have, but does not solve this problem.
However, Python SDK lives in Github, not piper. piper code can't reference Python SDK thus have no way of testing it.
Does it makes sense?

Do you know if there is plan to mirrow this repo into piper? I think it will solve this problem. We can setup integration test that reference the piper mirror code with TAP.

[sirtorry]

@busunkim96 please merge if it looks good to you

[busunkim96]

You're free to setup a mirror if you'd like, but that is not something the client libraries team is currently staffed to do.

I'll make sure to tag both of you in future PRs so you can manually inspect changes.

[busunkim96]

@sirtorry Could you do nox -s blacken to appease the linter?

[helinwang]

Thanks, @busunkim96 .

[busunkim96]

I missed this earlier, but it looks like the unit tests are failing.

[helinwang]

LGTM. Torry could you update the unit test?
The test is failing with errors like:

value = AssertionError("expected call not found.\nExpected: predict('my_model', {'row'..., {'row': {'values': [{'string_value': '1'}, {'string_value': '2'}]}}, None)",)
from_value = None

    def raise_from(value, from_value):
>       raise value
E       AssertionError: expected call not found.
E       Expected: predict('my_model', {'row': {'values': [{'string_value': '1'}, {'string_value': '2'}]}})
E       Actual: predict('my_model', {'row': {'values': [{'string_value': '1'}, {'string_value': '2'}]}}, None)

[sirtorry]

could also add unit tests for non-empty param passing if y'all want

[helinwang]

I think that's fine, since the method just forward param to predict.