yocto-gl icon indicating copy to clipboard operation
yocto-gl copied to clipboard

[FR] [Roadmap] Generate Open API / swagger for MLflow APIs

Open BenWilson2 opened this issue 2 years ago • 8 comments

MLflow Roadmap Item

This is an MLflow Roadmap item that has been prioritized by the MLflow maintainers. We’ve identified this feature as a highly requested addition to the MLflow package based on community feedback. We're seeking a community contribution for the implementation of this feature and will enthusiastically support the development and review of a submitted PR for this.

Contribution Note

As with other roadmap items, there may be a desire for multiple contributors to work on an issue. While we don’t discourage collaboration, we strongly encourage that a primary contributor is assigned to roadmap issues to simplify the merging process. The items on the roadmap are of a high priority. Due to the wide-spread demand of roadmap features, we encourage potential contributors to only agree to take on the work of creating a PR, making changes, and ensuring that test coverage is adequately created for the feature if they are willing and able to see the implementation through to a merged state.

Feature scope

This roadmap feature’s complexity is classified as:

  • [ ] good-first-issue: This feature is limited in complexity and effort required to implement.
  • [X] simple: This feature does not require a large amount of effort to implement and / or is clear enough to not need a design discussion with maintainers.
  • [ ] involved: This feature will require a substantial amount of development effort but does not require an agreed-upon design from the maintainers. The feedback given during the PR phase may be involved and necessitate multiple iterations before approval. (Please bear with us as we collaborate with you to make a great contribution)
  • [ ] design-recommended: This is a substantial feature that should have a design document approved prior to working on an implementation (to save your time, not ours). After agreeing to work on this feature, a maintainer will be assigned to support you throughout the development process.

Proposal Summary

Generate Swagger definitions from the grpc protobuf definitions to map to OpenAPI standards. A recommended approach for generating these files is to look into the gnostic library.

The original FR for this: #2948

Motivation

What is the use case for this feature?

Allow for stub creation through swagger codegen for interfacing with Mlflow APIs and (outside of scope for this FR) support swagger ui eventually.

Why is this use case valuable to support for MLflow users in general?

Aids in developer experience and reduces the time and complexity to create interface APIs to MLflow REST endpoints.

What component(s), interfaces, languages, and integrations does this feature affect?

Components

  • [ ] area/artifacts: Artifact stores and artifact logging
  • [ ] area/build: Build and test infrastructure for MLflow
  • [ ] area/docs: MLflow documentation pages
  • [ ] area/examples: Example code
  • [X] area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
  • [ ] area/models: MLmodel format, model serialization/deserialization, flavors
  • [ ] area/projects: MLproject format, project running backends
  • [ ] area/scoring: MLflow Model server, model deployment tools, Spark UDFs
  • [ ] area/server-infra: MLflow Tracking server backend
  • [X] area/tracking: Tracking Service, tracking client APIs, autologging

Interfaces

  • [ ] area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
  • [ ] area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
  • [ ] area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
  • [ ] area/windows: Windows support

Languages

  • [ ] language/r: R APIs and clients
  • [ ] language/java: Java APIs and clients
  • [ ] language/new: Proposals for new client languages

Integrations

  • [ ] integrations/azure: Azure and Azure ML integrations
  • [ ] integrations/sagemaker: SageMaker integrations
  • [ ] integrations/databricks: Databricks integrations

BenWilson2 avatar Jun 17 '22 00:06 BenWilson2

For any questions, concerns, or clarification on implementing this issue, please ping @BenWilson2

BenWilson2 avatar Jun 21 '22 18:06 BenWilson2

@BenWilson2 I'm willing to take this one. Maybe slightly off-topic: would it make sense to also include replacing Flask with FastAPI in the roadmap (Swagger docs would come out of the box and it's more performant than Flask). Let me know what you think!

rafaelvp-db avatar Jun 23 '22 13:06 rafaelvp-db

@dbczumar i can take a stab at it, as per our meeting today ;)

nfx avatar Aug 31 '22 19:08 nfx

Thanks @nfx ! :D

dbczumar avatar Aug 31 '22 19:08 dbczumar

Are there any plans to use move model serving to FastAPI or to provide automated Open API spec generation for mflow models serve component?

marrrcin avatar Sep 09 '22 14:09 marrrcin

@marrrcin Currently, the generation of this spec is semi-manual. I'm going to add model registry to the PR and we could consider it done, for now at least. Later in the future there's a possibility of a more automated generator, but it still has to be defined more in a scope of a bigger initiative at Databricks.

Could you please share your intended usages of OpenAPI spec, by the way? It'll help us prioritize. Usually the spec is not the end goal, but rather means to get there.

nfx avatar Sep 11 '22 12:09 nfx

It would be great to have an OpenAPI spec generated for the /invocations path, so that API users (developers) will be able to quickly play with served models by sending example requests to them e.g. from Swagger UI. The spec could be generated (optionally) from the logged model's signature and input_example.

marrrcin avatar Sep 12 '22 08:09 marrrcin

@marrrcin Ah I get what you mean now. OpenAPI for /invocations is a separate issue, not that much related to this one. We need to create a separate issue to track that one

nfx avatar Sep 12 '22 08:09 nfx

@nfx Hey hi, is this FR is fully implemented or anything pending that I could contribute!?

shaikmoeed avatar Oct 24 '22 14:10 shaikmoeed

Is there anything I can do to help on this?

phatlast96 avatar Nov 15 '22 22:11 phatlast96

Hi, I would like to contribute to the implementation of this road map. Is there anything that I could take up as my first task.

AdityaThakur1 avatar Dec 19 '22 21:12 AdityaThakur1

Hi @AdityaThakur1 @shaikmoeed @phatlast96 apologies for the delay here. It would be super helpful to integrate a utility into MLflow that can generate OpenAPI specs for the MLflow Tracking and Model Registry APIs from proto. For example, https://github.com/google/gnostic would be a very useful tool for this. We could add a script for this generation in https://github.com/mlflow/mlflow/tree/master/dev, which is where proto generation scripts (https://github.com/mlflow/mlflow/blob/master/dev/generate-protos.sh) live.

dbczumar avatar Jan 31 '23 07:01 dbczumar

FYI - I tinkered with this a little bit today and couldn't get any useful output from gnostic (more specifically protoc-gen-openapi. I've noticed a couple of things:

  • Syntax is proto2 which might have issues. I did also try porting over to proto3 but couldn't get that working correctly either.
  • At the very least, running the tool requires the .proto files to have something like option go_package = "./";. I got all the import tests to pass, but running protoc model_registry.proto service.proto -I=. --openapi_out=. produce a stub openapi.yml without anything in it. Some discussion at https://stackoverflow.com/questions/70586511/protoc-gen-go-unable-to-determine-go-import-path-for-simple-proto (and a few other ideas in https://stackoverflow.com/questions/61666805/correct-format-of-protoc-go-package)

Gave up for now, but would love to see some real Swagger produced as well

ddl-ebrown avatar Mar 30 '23 21:03 ddl-ebrown

FYI - I tinkered with this a little bit today and couldn't get any useful output from gnostic (more specifically protoc-gen-openapi. I've noticed a couple of things:

  • Syntax is proto2 which might have issues. I did also try porting over to proto3 but couldn't get that working correctly either.
  • At the very least, running the tool requires the .proto files to have something like option go_package = "./";. I got all the import tests to pass, but running protoc model_registry.proto service.proto -I=. --openapi_out=. produce a stub openapi.yml without anything in it. Some discussion at https://stackoverflow.com/questions/70586511/protoc-gen-go-unable-to-determine-go-import-path-for-simple-proto (and a few other ideas in https://stackoverflow.com/questions/61666805/correct-format-of-protoc-go-package)

Gave up for now, but would love to see some real Swagger produced as well

Thanks @ddl-ebrown , this is super helpful!

dbczumar avatar Mar 30 '23 21:03 dbczumar