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.

Sign up for GitHub

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

Jump to bottom

Fix metadata OpenAPI spec #17

Merged
merged 6 commits into from
Feb 26, 2024
Merged

Fix metadata OpenAPI spec #17

merged 6 commits into from
Feb 26, 2024

Conversation

lampajr
Copy link
Member

@lampajr lampajr commented Feb 22, 2024 •
edited
Loading

Description

This PR aims to improve the openapi spec for MetadataValue by properly using the discriminator.

How Has This Been Tested?

Checking the generated models and server are correctly bootstrapped

make FORCE_SERVER_GENERATION=true clean build

Then by running both unit and robot tests:

make test

and

# Start MLMD server
...

# Start model registry
make proxy

# In another terminal
robot test/robot/ && TEST_MODE=python robot test/robot/MRandLogicalModel.robot

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@lampajr lampajr force-pushed the lampajr-metadatadiscriminator branch 2 times, most recently from 45ba485 to 96c9f30 Compare February 22, 2024 15:02
tarilabs and others added 4 commits February 22, 2024 16:05
@tarilabs
fix: OpenAPI metadata discriminator
154645c 
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
@tarilabs
wiring factories and default values missed in codegen
6be24ba 
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
@tarilabs
introduce openapi defaults
2cca86b 
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
@lampajr @tarilabs
fix TestMetadataValue*
adad89a 
Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
@tarilabs tarilabs force-pushed the lampajr-metadatadiscriminator branch from 96c9f30 to adad89a Compare February 22, 2024 15:06
@tarilabs
Copy link
Member

tarilabs commented Feb 22, 2024 •
edited
Loading

TBD:

  • generation of internal/server/openapi/type_asserts.go still need some fixing
  • clarify if we really need to bump "version" to v1alpha2

@dhirajsb
Copy link
Contributor

Given it's a breaking change in the API, we have to bump api version.
The discriminator change lgtm.

andreaTP
andreaTP reviewed Feb 26, 2024
View reviewed changes
api/openapi/model-registry.yaml
@@ -1102,50 +1102,102 @@ components:
- $ref: "#/components/schemas/MetadataStructValue"
- $ref: "#/components/schemas/MetadataProtoValue"
- $ref: "#/components/schemas/MetadataBoolValue"
discriminator:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making the change!
This is going to improve a lot, the generation of code out of the spec 👍

@lampajr
fix: type assert generation
627fa3d 
Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
@lampajr
upgrade openapi spec version to v1alpha2
8416435 
Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
@lampajr lampajr force-pushed the lampajr-metadatadiscriminator branch from 1f714b6 to 8416435 Compare February 26, 2024 11:00
@lampajr
Copy link
Member Author

lampajr commented Feb 26, 2024

TBD:

  • generation of internal/server/openapi/type_asserts.go still need some fixing
  • clarify if we really need to bump "version" to v1alpha2

Addressed both items:

  • Updated the type_asserts patch to fix the autogenerated code
  • Bumped openapi spec version to v1alpha2

@lampajr lampajr marked this pull request as ready for review February 26, 2024 11:04
@lampajr
Copy link
Member Author

lampajr commented Feb 26, 2024

If the change looks good, I would suggest to merge with squash!

@tarilabs
Copy link
Member

/lgtm

Thanks a lot @lampajr for the teamwork and the contributions!

@google-oss-prow google-oss-prow bot added the lgtm label Feb 26, 2024
@tarilabs
Copy link
Member

/approve

@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lampajr, tarilabs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit ddbd6ae into kubeflow:main Feb 26, 2024
11 checks passed
@isinyaaa isinyaaa mentioned this pull request Feb 26, 2024
Merged
3 tasks
@lampajr lampajr deleted the lampajr-metadatadiscriminator branch February 26, 2024 15:24
openshift-merge-bot bot referenced this pull request in opendatahub-io/model-registry Mar 4, 2024
@tarilabs @lampajr
@dependabot @isinyaaa
sync upstream KF into midstream ODH (#15)
6ae59f5 
* kubeflow: fix go module and odh. debranding (#15)

* kubeflow: change go module name and references

Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>

* kubeflow: rename odh. into kfmr.

Signed-off-by: tarilabs <matteo.mortari@gmail.com>

* kubeflow: py: pyproject description

Signed-off-by: tarilabs <matteo.mortari@gmail.com>

* kubeflow: nit picks in text documents

Signed-off-by: tarilabs <matteo.mortari@gmail.com>

---------

Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
Co-authored-by: tarilabs <matteo.mortari@gmail.com>

* Fix metadata OpenAPI spec (#17)

* fix: OpenAPI metadata discriminator

Signed-off-by: tarilabs <matteo.mortari@gmail.com>

* wiring factories and default values missed in codegen

Signed-off-by: tarilabs <matteo.mortari@gmail.com>

* introduce openapi defaults

Signed-off-by: tarilabs <matteo.mortari@gmail.com>

* fix TestMetadataValue*

Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>

* fix: type assert generation

Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>

* upgrade openapi spec version to v1alpha2

Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>

---------

Signed-off-by: tarilabs <matteo.mortari@gmail.com>
Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
Co-authored-by: tarilabs <matteo.mortari@gmail.com>

* kubeflow: make MLMD type names (and prefix) pluggable (#19)

Signed-off-by: Matteo Mortari <matteo.mortari@gmail.com>

* build(deps): bump google.golang.org/grpc from 1.61.1 to 1.62.0 (#20)

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.61.1 to 1.62.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.61.1...v1.62.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* OAS: Fix discriminator field definition for Artifact (#22)

* gitignore: ignore all coverage files

Signed-off-by: Isabella Basso do Amaral <idoamara@redhat.com>

* OAS: fix discriminator field for Artifact

Signed-off-by: Isabella Basso do Amaral <idoamara@redhat.com>

---------

Signed-off-by: Isabella Basso do Amaral <idoamara@redhat.com>

---------

Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
Signed-off-by: tarilabs <matteo.mortari@gmail.com>
Signed-off-by: Matteo Mortari <matteo.mortari@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Isabella Basso do Amaral <idoamara@redhat.com>
Co-authored-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Isabella Basso <idoamara@redhat.com>
@isinyaaa isinyaaa mentioned this pull request Mar 5, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreaTP andreaTP andreaTP left review comments

@andreyvelich andreyvelich Awaiting requested review from andreyvelich

@ckadner ckadner Awaiting requested review from ckadner

@rareddy rareddy Awaiting requested review from rareddy

@zijianjoy zijianjoy Awaiting requested review from zijianjoy

Assignees

@tarilabs tarilabs

Labels
approved lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lampajr @tarilabs @dhirajsb @andreaTP