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

Log: MR Python client REST rebase #181

Closed
tarilabs opened this issue Jul 12, 2024 · 1 comment
Closed

Log: MR Python client REST rebase #181

tarilabs opened this issue Jul 12, 2024 · 1 comment

Comments

@tarilabs
Copy link
Member

This is just a convenience GitHub issue to log data from multiple sources, and provide a summary of the Model Registry Python client rebase on REST work, so far.

The challenge

Historically, the Model Registry Python client connected directly to the gRPC MLMD container/service.
Besides this requiring to replicate logic on GoLang REST server and the MR Python client itself (i.e.: implement the Logical Model mapping to/from MLMD both in GoLang and Python) so in pragmatic terms implementing the same logic in 2 places, it posed some additional challenges due to the only distributions available of the MLMD python library/wheel.

For example, but not limiting to:

  • the MLMD python wheel only distributed for x86 (example at this link, only amd64/x86 available)
  • the MLMD python wheel only distributed for CPython 3.9 and 3.10, not available for 3.11 (see, for example, https://github.com/google/ml-metadata/issues/198)

Introducing a workaround

Some of these concerns where partially addressed with:

  • providing a remote-only, grpc-only version of the MLMD whl (see related document)

However, this posed some challenges to end-users, for example but not limiting to:

as can be seen, it was difficult to explain to end-user the constraint mainly came as the requirement from the MLMD whl, not necessarily a choice originating in the MR client itself.

The solution

This was overcome with:

Please notice the code-generated part is not exposed (and is not intended to be exposed in the future) to the end-user of the MR Python client; this is so that, if in the future we will need to change the underlying implementation, that would require a little less effort eventually.

What else was considered

Different REST code generators were evaluated, including Kiota, openapi-codegen, etc.

Each code generators had some limitations; for posterity in the case of Kiota a bunch were actually addressed upstream, for example:

  • https://github.com/microsoft/kiota/issues/4428
  • https://github.com/microsoft/kiota/issues/4325

While other remain (at the time of writing):

  • https://github.com/microsoft/kiota/issues/4178
  • https://github.com/microsoft/kiota/issues/4433
  • https://github.com/microsoft/kiota/issues/4750#issuecomment-2160551178

In the case of openapi-codegen, limitations affected discriminator fields too, for example requiring to "manually" change the generated code anyway:

diff --git a/clients/python/src/mr_openapi/models/doc_artifact.py b/clients/python/src/mr_openapi/models/doc_artifact.py
index 5e6a34a5..4f7c29bd 100644
--- a/clients/python/src/mr_openapi/models/doc_artifact.py
+++ b/clients/python/src/mr_openapi/models/doc_artifact.py
@@ -104,6 +104,7 @@ class DocArtifact(BaseModel):
return cls.model_validate(obj)
_obj = cls.model_validate({
+ "artifactType": "doc-artifact",
"customProperties": dict(
(_k, MetadataValue.from_dict(_v))
for _k, _v in obj["customProperties"].items()
diff --git a/clients/python/src/mr_openapi/models/model_artifact.py b/clients/python/src/mr_openapi/models/model_artifact.py
index 380d1480..306d74d6 100644
--- a/clients/python/src/mr_openapi/models/model_artifact.py
+++ b/clients/python/src/mr_openapi/models/model_artifact.py
@@ -109,6 +109,7 @@ class ModelArtifact(BaseModel):
return cls.model_validate(obj)
_obj = cls.model_validate({
+ "artifactType": "model-artifact",
"customProperties": dict(
(_k, MetadataValue.from_dict(_v))
for _k, _v in obj["customProperties"].items()

Since the openapi-codegen is already used on the Go side to generate the server models, this was picked as the selected code-generator to go forward with at this time.

Additional considerations

This workaround is no longer needed:

https://github.com/kubeflow/model-registry/pull/116/files#diff-6b074bce6a463d7cd6b69e5b1901d4d48c6ff2cd150a40ce849f7a99cb68bce4R123-R124

thanks to the REST rebase.

With KF 1.9, this workaround is no longer needed:

#165 (comment)

thanks to the REST rebase.


I would like to express my gratitude to everyone who contributed directly or indirectly to this work, it benefits a lot the MR python client itself and its end-users 🙏

@tarilabs
Copy link
Member Author

Considered Done.

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

No branches or pull requests

1 participant