-
Notifications
You must be signed in to change notification settings - Fork 64
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
Support ModelCardGenerator in Vertex AI #260
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR! 🚀 Instructions: Approve using |
Thank you for your contribution, @jeongukjae! Does this pull request introduce any breaking changes? @hanneshapke, would this resolve your errors in #249? |
@codesue It's changing the artifact's type, so I think this is a breaking change 🤔 but a small change I think? |
It's been a while since I've worked directly with artifacts, so this gave me pause, but it does seem like a small change. We can point people back to this PR if they run into problems, so let's update the description of this PR to also include a brief summary of the changes. Something like
Otherwise this LGTM from the ModelCardGenerator side with passing tests. |
@codesue Great. I updated the PR body! |
Hi @rcrowe-google or @casassg, would you please take a look at this PR since Hannes is currently unavailable. 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look ok to me, based on my limited knowledge of this code. However I'm not listed as an owner so I don't think I can actually approve this for merging:
Missing approvals for PR. Potential owners: @casassg, @hanneshapke, @codesue, @deutranium, @wihanbooyse, and @BACtaki
/lgtm A suggestion: move artifact to a separate file similar to how we have for pkgs so that it avoids triggering all CI |
Approval received from @casassg! ✅ PR is approved. Missing merge command to auto-merge PR! |
Feel free to comment / merge to merge btw |
Fixes #259
Changes the ModelCard artifact's TYPE_NAME from "ModelCard" to "tfx_addons.ModelCard" to conform with the Kubeflow V2 schema.
For detailed descriptions of the changes, check out this comment.