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: Made display_name parameter optional for most calls #882

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Dec 3, 2021

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #853 🦕

@product-auto-label product-auto-label bot added the api: aiplatform Issues related to the AI Platform API. label Dec 3, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 3, 2021
@Ark-kun Ark-kun force-pushed the feat--Made-display_name-optional-for-most-calls branch from 2508d11 to ae8adcb Compare December 3, 2021 06:49
@morgandu morgandu requested a review from sasha-gitg January 5, 2022 20:17
Copy link
Member

@sasha-gitg sasha-gitg left a comment

Choose a reason for hiding this comment

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

Please add/update unit tests to test this change.

google/cloud/aiplatform/datasets/dataset.py Show resolved Hide resolved
google/cloud/aiplatform/datasets/image_dataset.py Outdated Show resolved Hide resolved
@vinnysenthil vinnysenthil self-assigned this Jan 24, 2022
@Ark-kun Ark-kun force-pushed the feat--Made-display_name-optional-for-most-calls branch from bf9f73f to fabb496 Compare February 3, 2022 03:35
@Ark-kun Ark-kun requested a review from sasha-gitg February 3, 2022 04:07
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Feb 3, 2022

I think I've resolved all feedback.

@Ark-kun Ark-kun force-pushed the feat--Made-display_name-optional-for-most-calls branch from de0b11f to f08c19b Compare February 5, 2022 02:52
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Feb 23, 2022

What are the remaining action items for me here?

@vinnysenthil
Copy link
Contributor

Re-assigning @sasha-gitg for triage. @Ark-kun the change of argument order in public methods is considered a breaking change and cannot be merged. Some Vertex AI resources require display_name (i.e. all the Job subclasses). Please work with the team to address these issues, thanks!

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Feb 23, 2022

@Ark-kun the change of argument order in public methods is considered a breaking change and cannot be merged

Thank you for the link. I've missed this one. I'll restore the ordering. Thank you.

The parameter default values had to be removed due to Python's syntax.
@Ark-kun Ark-kun force-pushed the feat--Made-display_name-optional-for-most-calls branch from 40dac14 to 00977be Compare March 2, 2022 10:31
google/cloud/aiplatform/datasets/dataset.py Outdated Show resolved Hide resolved
google/cloud/aiplatform/datasets/dataset.py Show resolved Hide resolved
google/cloud/aiplatform/jobs.py Outdated Show resolved Hide resolved
google/cloud/aiplatform/jobs.py Outdated Show resolved Hide resolved
google/cloud/aiplatform/jobs.py Outdated Show resolved Hide resolved
google/cloud/aiplatform/training_jobs.py Outdated Show resolved Hide resolved
@Ark-kun Ark-kun requested a review from sasha-gitg March 15, 2022 04:53
@Ark-kun Ark-kun force-pushed the feat--Made-display_name-optional-for-most-calls branch from 45d8e14 to b1b19f7 Compare April 1, 2022 02:16
@Ark-kun Ark-kun force-pushed the feat--Made-display_name-optional-for-most-calls branch from b1b19f7 to fee7b89 Compare April 1, 2022 02:17
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Apr 1, 2022
@sasha-gitg sasha-gitg added the automerge Merge the pull request once unit tests and other checks pass. label Apr 5, 2022
@gcf-merge-on-green gcf-merge-on-green bot merged commit 400b760 into googleapis:main Apr 5, 2022
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 5, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
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. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make display_name parameter optional for all resources
4 participants