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

fix: Incorrect request body for create and patch version #522

Merged
merged 5 commits into from
Jan 26, 2024

Conversation

tiopramayudi
Copy link
Contributor

@tiopramayudi tiopramayudi commented Jan 24, 2024

Description

Previously in this #518 we introduce CRUD API for model schema, also changed the version schema that aim to update model schema during version creation. While CRUID API is working correctly, create model version with model schema info is not working properly, due to the request body type that is used for the controller is not Version but VersionPost. Hence this PR try to fix that issue

Modifications

  • api/api/version_api.go -> Update the model version creation by supplying model schema data from VersionPost to Version
  • api/models/version.go -> Including ModelSchema field for VersionPost and VersionPatch struct to supply model schema info during model version creation or patch
  • python/sdk/merlin/model_schema.py -> Add output_class field to model prediction output
  • python/sdk/test/integration_test.py -> Add new integration test for model schema

Tests

Checklist

  • Added PR label
  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduces API changes

Release Notes


@tiopramayudi tiopramayudi added the bug Something isn't working label Jan 24, 2024
@ghost
Copy link

ghost commented Jan 24, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@khorshuheng
Copy link
Collaborator

khorshuheng commented Jan 25, 2024

Seems like the deserialization failed because fields in RegressionOutput is a subset of the fields in BinaryClassification. Since extra fields are ignored, both passed the validation test and result in more than one match.

@khorshuheng
Copy link
Collaborator

I checked the swagger schema. Are we missing the mapping here?

    ModelPredictionOutput:
      type: object
      discriminator:
        propertyName: output_class
        mapping:
          BinaryClassificationOutput: '#/components/schemas/BinaryClassificationOutput'
          RankingOutput: '#/components/schemas/RankingOutput'

@tiopramayudi
Copy link
Contributor Author

I checked the swagger schema. Are we missing the mapping here?

    ModelPredictionOutput:
      type: object
      discriminator:
        propertyName: output_class
        mapping:
          BinaryClassificationOutput: '#/components/schemas/BinaryClassificationOutput'
          RankingOutput: '#/components/schemas/RankingOutput'

this is due to not using discriminator lookup, hence they try to deserialize to all possible schema

@tiopramayudi tiopramayudi merged commit 9f3b161 into main Jan 26, 2024
32 checks passed
@tiopramayudi tiopramayudi deleted the fix-model-schema branch January 26, 2024 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants