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

Model deployment support for model observability #492

Merged
merged 30 commits into from
Dec 5, 2023

Conversation

tiopramayudi
Copy link
Collaborator

@tiopramayudi tiopramayudi commented Nov 17, 2023

What this PR does / why we need it:

In the previous PR we introduce PyFuncV3Model to support model observability use case. What missing from previous PR is deployment process of PyFuncV3Model, hence this PR is raised.

Modification:

  • api/config/config.go - Adding PyFuncPublisherConfig new configuration for pyfunc publisher
  • api/config/environment.go - Update parsing DeploymentConfig to add relevant config to the deployment config
  • db-migrations/34_version_endpoints_enable_observability.*.sql - Adding model observability enable flag to version endpoint
  • api/models/model.go - Adding pyfunc_v3 model type
  • api/cluster/resource/templater.go
    • All deployment config will be part of deploymentConfig including standard transformer configuration
    • Add required env vars to be passed for PyFuncV3Model
  • api/api/version_endpoints_api.go - Adding PyFuncV3Model as supported model type for UPI
  • python/sdk/test/pyfunc_integration_test.py - Adding integration test for PyFuncV3Model

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:


Checklist

  • 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 introduce API changes

@tiopramayudi tiopramayudi marked this pull request as draft November 17, 2023 08:01
@ghost
Copy link

ghost commented Nov 17, 2023

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

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@tiopramayudi tiopramayudi marked this pull request as ready for review November 22, 2023 05:26
@tiopramayudi tiopramayudi changed the title [WIP] Model deployment support for model observability Model deployment support for model observability Nov 22, 2023
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR and the detailed description, @tiopramayudi! Left some small comments - the rest LGTM.

python/sdk/test/pyfunc_integration_test.py Show resolved Hide resolved
api/config/config.go Outdated Show resolved Hide resolved
api/cluster/resource/templater.go Show resolved Hide resolved
api/cluster/resource/templater_test.go Show resolved Hide resolved
api/cmd/api/setup.go Show resolved Hide resolved
api/models/service.go Outdated Show resolved Hide resolved
@khorshuheng
Copy link
Collaborator

The e2e test failed because of:

Merlin log
  {"level":"panic","ts":1701181719.603602,"caller":"api/main.go:101","msg":"Failed validating config: Key: 'Config.StandardTransformerConfig.Kafka.Acks' Error:Field validation for 'Acks' failed on the 'required' tag\nKey: 'Config.PyFuncPublisherConfig.Kafka.Brokers' Error:Field validation for 'Brokers' failed on the 'required' tag\nKey: 'Config.PyFuncPublisherConfig.Kafka.Acks' Error:Field validation for 'Acks' failed on the 'required' tag","stacktrace":"main.main\n\t/src/api/cmd/api/main.go:101\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:250"}
  panic: Failed validating config: Key: 'Config.StandardTransformerConfig.Kafka.Acks' Error:Field validation for 'Acks' failed on the 'required' tag
  Key: 'Config.PyFuncPublisherConfig.Kafka.Brokers' Error:Field validation for 'Brokers' failed on the 'required' tag
  Key: 'Config.PyFuncPublisherConfig.Kafka.Acks' Error:Field validation for 'Acks' failed on the 'required' tag

@tiopramayudi
Copy link
Collaborator Author

The e2e test failed because of:

Merlin log
  {"level":"panic","ts":1701181719.603602,"caller":"api/main.go:101","msg":"Failed validating config: Key: 'Config.StandardTransformerConfig.Kafka.Acks' Error:Field validation for 'Acks' failed on the 'required' tag\nKey: 'Config.PyFuncPublisherConfig.Kafka.Brokers' Error:Field validation for 'Brokers' failed on the 'required' tag\nKey: 'Config.PyFuncPublisherConfig.Kafka.Acks' Error:Field validation for 'Acks' failed on the 'required' tag","stacktrace":"main.main\n\t/src/api/cmd/api/main.go:101\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:250"}
  panic: Failed validating config: Key: 'Config.StandardTransformerConfig.Kafka.Acks' Error:Field validation for 'Acks' failed on the 'required' tag
  Key: 'Config.PyFuncPublisherConfig.Kafka.Brokers' Error:Field validation for 'Brokers' failed on the 'required' tag
  Key: 'Config.PyFuncPublisherConfig.Kafka.Acks' Error:Field validation for 'Acks' failed on the 'required' tag

Need to merge this PR first

@tiopramayudi tiopramayudi merged commit c5b61e0 into main Dec 5, 2023
36 checks passed
@tiopramayudi tiopramayudi deleted the model_obs_deployment branch December 5, 2023 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants