model-registry icon indicating copy to clipboard operation
model-registry copied to clipboard

[Ongoing] Capture gaps in MR codebase to make more pragmatic

Open syntaxsdev opened this issue 7 months ago • 8 comments

Ongoing, we should capture gaps or issues in the MR codebase (both backend and clients) to make the eventual tackle codebase simpler, easier to work on, developer friendly and maintainable.

Even if they are not "gaps", but are "gotcha's" we should also capture.

syntaxsdev avatar May 15 '25 16:05 syntaxsdev

To start, suggestion to eventually modularize the client code more and split out larger files. https://github.com/kubeflow/model-registry/pull/1111#issuecomment-2883991681

syntaxsdev avatar May 15 '25 16:05 syntaxsdev

consider implementing config mgt a-la 12 factor apps with Gila (or analogous). https://github.com/kubeflow/model-registry/pull/1031#discussion_r2066732610

tarilabs avatar May 16 '25 07:05 tarilabs

I would consider refactoring the Storage to OCI auth done with:

  • https://github.com/kubeflow/model-registry/pull/1031

to make use of temporary files (with secure usage that NamedTemporaryFile extends).

This can be achieved for both Skopeo and Oras backends, for which Push example is provided below:

# example
skopeo_push("download", "quay.io/mmortari/demo20250519:latest", ["--debug", "--dest-authfile", "<omitted>"])

# example
oras_push("download", "quay.io/mmortari/demo20250519:latest", ["--verbose", "--to-registry-config", "<omitted>"])

this would also simplify the handling of the current function wrapping.

tarilabs avatar May 19 '25 09:05 tarilabs

@tarilabs yes we can do that. We can address in a cleanup for it at a later date?

syntaxsdev avatar May 20 '25 17:05 syntaxsdev

thanks @syntaxsdev in my view https://github.com/kubeflow/model-registry/issues/1115#issuecomment-2890292128 is of higher priority

tarilabs avatar May 20 '25 18:05 tarilabs

I believe we should move as_mlops_engineer_i_ itests the Python client tests and move them to a dedicated file to de-clutter the tests.

re: https://github.com/kubeflow/model-registry/blob/a56dc202495c6a04d3ab90243befecf072cf29f3/clients/python/tests/test_client.py#L1181

and re: https://github.com/kubeflow/model-registry/blob/a56dc202495c6a04d3ab90243befecf072cf29f3/clients/python/tests/regression_test.py#L140

please note they are also in different files.

why not combine into one persona test file?

What do you think? @dbasunag @lugi0

syntaxsdev avatar Aug 04 '25 17:08 syntaxsdev

I like the idea of dedicated test files based on persona or even features. This would make the tests more organized. While we are at it, I would also suggest 1) renaming tests like test_as_mlops_engineer_i_would_like_to_store_a_malformed_registered_model_i_get_a_structured_error_message to something short. We already have nice descriptions for each tests (we can add for any tests that misses descriptions). 2) parameterize tests when appropriate - that would reduce duplicate test code and encourage more code reusability. I totally understand if that happens over time with multiple PRs.

dbasunag avatar Aug 04 '25 20:08 dbasunag