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

feat: support new protobuf value param types for Pipeline Job client #797

Merged
merged 4 commits into from Oct 28, 2021

Conversation

ji-yaqi
Copy link
Contributor

@ji-yaqi ji-yaqi commented Oct 27, 2021

Note: The test will be uncommented when GAPIC library change for protobuf values is complete.

@product-auto-label product-auto-label bot added the api: aiplatform Issues related to the AI Platform API. label Oct 27, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 27, 2021
@ji-yaqi ji-yaqi requested a review from chensun October 27, 2021 00:26
Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

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

Only two nitpicks, otherwise looking great!

Thank you!

raise TypeError("Got unknown type of value: {}".format(value))

return result
# 'parameters' are deprecated in IR and changed to 'parameterValues'.
Copy link
Member

Choose a reason for hiding this comment

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

nit: same here, try reading parameterValues first, then the old field as a fallback.
That would be more "efficient" going forward.

parameter_types = {k: v["type"] for k, v in parameter_input_definitions.items()}
# 'type' is deprecated in IR and change to 'parameterType'.
parameter_types = {
k: v.get("type") or v.get("parameterType")
Copy link
Member

Choose a reason for hiding this comment

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

nit: usually we read the latest field first, and fallback to the deprecated field if the latest one not found.

Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

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

Great work! Thanks!

@chensun chensun merged commit 2fc05ca into googleapis:main Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: aiplatform Issues related to the AI Platform API. 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

2 participants