DO NOT MERGE. Used to trigger coderabbit. #289
Conversation
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
…rvice tests Signed-off-by: Dhiraj Bokde <[email protected]>
…ype properties only support int32, not int64 Signed-off-by: Dhiraj Bokde <[email protected]>
…es in artifact queries Signed-off-by: Dhiraj Bokde <[email protected]>
…riment run metrics Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
…nflicts Signed-off-by: Dhiraj Bokde <[email protected]>
…ies and custom properties Signed-off-by: Dhiraj Bokde <[email protected]>
…rebased on main Signed-off-by: Dhiraj Bokde <[email protected]>
…oints for embedmd datastore Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
…ation to reduce code duplication Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
…tore Signed-off-by: Dhiraj Bokde <[email protected]>
…etric history when retrieving all artifacts for runs and versions Signed-off-by: Dhiraj Bokde <[email protected]>
…action check Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
…pe property migraiton Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dipanshu Gupta <[email protected]>
Signed-off-by: ppadti <[email protected]>
Signed-off-by: Matteo Mortari <[email protected]>
* add rhec source & pull rhec models using graphql Signed-off-by: Adysen Rothman <[email protected]> * replace graphql client & update source.yaml Signed-off-by: Adysen Rothman <[email protected]> * add testing Signed-off-by: Adysen Rothman <[email protected]> * cleanup logs and todo Signed-off-by: Adysen Rothman <[email protected]> * rm temp placeholder Signed-off-by: Adysen Rothman <[email protected]> * add graphql gen code to makefile Signed-off-by: Adysen Rothman <[email protected]> * add makefule targets Signed-off-by: Adysen Rothman <[email protected]> * resolve deps Signed-off-by: Adysen Rothman <[email protected]> * add context to genqlient and rhec sources Signed-off-by: Adysen Rothman <[email protected]> * work vendor Signed-off-by: Adysen Rothman <[email protected]> --------- Signed-off-by: Adysen Rothman <[email protected]>
Signed-off-by: Alessio Pragliola <[email protected]>
Signed-off-by: Alessio Pragliola <[email protected]>
|
Skipping CI for Draft Pull Request. |
WalkthroughThis update introduces comprehensive experiment and experiment run tracking to the model registry system, including new artifact types (DataSet, Metric, Parameter), advanced filtering capabilities, and new REST and GraphQL interfaces. It adds database migrations, core service logic, OpenAPI and Python client support, extensive test coverage, and refactors artifact handling for greater generality and extensibility. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Service
participant DB
Client->>API: CreateExperiment/Run/Artifact/Metric/Parameter
API->>Service: Validate & Map Request
Service->>DB: Upsert entity (experiment/run/artifact/metric/parameter)
DB-->>Service: Save or update record
Service->>API: Map & Return entity
API-->>Client: Response
Client->>API: ListExperiments/ExperimentRuns/Artifacts (with filterQuery)
API->>Service: Parse & Translate filterQuery
Service->>DB: Query with translated filter
DB-->>Service: Return filtered list
Service->>API: Map & Return list
API-->>Client: Response
Estimated code review effort5 (~180+ minutes) Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tarilabs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@coderabbitai review |
github-actions
bot
added
Area/UI
Area/MR Python client
Area/Go REST server
Area/Manifests
labels
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (1)
clients/python/src/mr_openapi/README.md (1)
139-194: Fix markdown list indentation for better readability.The static analysis tool detected incorrect indentation for the model documentation list. All list items should start at column 0 without leading spaces.
Apply this formatting fix pattern to all affected lines:
- - [ArtifactTypeQueryParam](mr_openapi/docs/ArtifactTypeQueryParam.md) + - [ArtifactTypeQueryParam](mr_openapi/docs/ArtifactTypeQueryParam.md)
🧹 Nitpick comments (27)
clients/ui/frontend/src/app/api/k8s.ts (1)
251-281: ConsolidateaddOwnerReferenceinto the shared utility to avoid duplicationThe helper is already present in
mod-arch-shared. Maintaining a local copy risks behaviour drift and extra maintenance. Prefer importing it (or moving it there and deleting this block) once the TODO is addressed.clients/python/pyproject.toml (1)
3-3: Bumped to 0.2.22 – remember to document the changePlease ensure the CHANGELOG / release notes capture the new experiment-related additions now carried by this release.
catalog/internal/catalog/genqlient/genqlient.yaml (1)
1-10: Fix YAML formatting issues.The static analysis tools have identified formatting issues that should be addressed:
- Missing newline character at the end of file
- Trailing spaces on line 10
Apply this diff to fix the formatting:
bindings: DateTime: - type: time.Time + type: time.Time +catalog/internal/catalog/genqlient/README.md (1)
11-12: Fix markdown list indentation.The nested list items use 4-space indentation instead of the expected 2-space indentation per markdown standards.
- - `schema.graphql`: The GraphQL schema for the RHEC API. - - `*.graphql`: Files containing the GraphQL queries. + - `schema.graphql`: The GraphQL schema for the RHEC API. + - `*.graphql`: Files containing the GraphQL queries.clients/ui/bff/internal/api/model_registry_settings_handler.go (1)
131-154: Consider REST semantics for DELETE operationThe implementation correctly extracts parameters and returns a JSON response to satisfy frontend expectations. However, returning a
200 OKwith the deleted resource data is unconventional for DELETE operations.Consider these alternatives:
- Return
204 No Contentwith empty body (RESTful standard)- Return
200 OKwith a confirmation message instead of the full resource- Use
202 Acceptedif the deletion is asynchronousThe current approach works as a temporary solution, but for production consider:
- err := app.WriteJSON(w, http.StatusOK, modelRegistryRes, nil) + // Return 204 No Content for successful deletion (RESTful standard) + w.WriteHeader(http.StatusNoContent)Or maintain current structure but with confirmation message:
- modelRegistryRes := ModelRegistrySettingsEnvelope{ - Data: registry, - } + type DeleteResponse struct { + Message string `json:"message"` + Id string `json:"id"` + } + deleteRes := Envelope[DeleteResponse, None]{ + Data: DeleteResponse{ + Message: "Model registry deleted successfully", + Id: modelId, + }, + }clients/python/src/mr_openapi/models/experiment.py (1)
87-116: Fix duplicate comment in documentation.The implementation correctly handles readonly field exclusion and custom_properties serialization, but there's a duplicate documentation line.
* OpenAPI `readOnly` fields are excluded. - * OpenAPI `readOnly` fields are excluded.internal/core/experiment.go (1)
123-129: Avoid variable shadowing in the loop.The loop variable
experimentshadows the function parameter name, which could lead to confusion.for _, experiment := range experiments.Items { - experiment, err := b.mapper.MapToExperiment(experiment) + mappedExperiment, err := b.mapper.MapToExperiment(experiment) if err != nil { return nil, fmt.Errorf("%v: %w", err, api.ErrBadRequest) } - experimentList.Items = append(experimentList.Items, *experiment) + experimentList.Items = append(experimentList.Items, *mappedExperiment) }clients/python/src/mr_openapi/models/metric_create.py (1)
73-76: Consider tracking the Pydantic v2 migration TODO.The TODO comment indicates a future migration to Pydantic v2's native JSON serialization. This should be tracked to ensure consistency across all generated models.
Would you like me to create an issue to track the Pydantic v2 migration across all model files?
internal/converter/mlmd_openapi_converter.go (1)
107-107: Consider using a more formal comment style.The comment "ADD EXPERIMENT CONVERTER FUNCTIONS" appears informal. Consider using a comment that describes the section more professionally, such as "Experiment and ExperimentRun conversion methods" or simply removing it as the method names are self-documenting.
- // ADD EXPERIMENT CONVERTER FUNCTIONS + // Experiment and ExperimentRun conversion methodsclients/python/src/mr_openapi/models/experiment_list.py (1)
47-48: Track the Pydantic v2 migration TODO.The TODO comment indicates that the serialization method should be updated when migrating to Pydantic v2. This technical debt should be tracked to ensure it's addressed during the migration.
Would you like me to create an issue to track this Pydantic v2 migration task across all model files?
clients/python/src/mr_openapi/models/parameter.py (2)
102-103: Remove duplicate comment.There's a duplicated comment about excluding OpenAPI
readOnlyfields.- * OpenAPI `readOnly` fields are excluded. - * OpenAPI `readOnly` fields are excluded. + * OpenAPI `readOnly` fields are excluded.
84-84: Consider updating to Pydantic v2 syntax.The TODO comment suggests using the newer Pydantic v2 method for JSON serialization. Consider updating this if the project supports Pydantic v2.
- # TODO: pydantic v2: use .model_dump_json(by_alias=True, exclude_unset=True) instead - return json.dumps(self.to_dict()) + return self.model_dump_json(by_alias=True, exclude_unset=True)clients/python/src/mr_openapi/models/parameter_update.py (1)
66-66: Consider updating to Pydantic v2 syntax.Same TODO comment as in the Parameter model. Consider updating this if the project supports Pydantic v2.
- # TODO: pydantic v2: use .model_dump_json(by_alias=True, exclude_unset=True) instead - return json.dumps(self.to_dict()) + return self.model_dump_json(by_alias=True, exclude_unset=True)internal/core/experiment_test.go (1)
267-267: Consider making the sleep duration configurable or more robust.The fixed 10ms sleep might be insufficient in slower CI environments or could cause flaky tests. Consider using a larger duration or making it configurable.
- time.Sleep(10 * time.Millisecond) + time.Sleep(100 * time.Millisecond) // Ensure timestamp difference in CI environmentsinternal/converter/openapi_mlmd_converter.go (1)
100-101: Remove unnecessary comment.The comment "ADD EXPERIMENT CONVERTER FUNCTIONS" appears to be a development artifact and should be removed.
- // ADD EXPERIMENT CONVERTER FUNCTIONS -clients/python/src/mr_openapi/models/experiment_run_update.py (1)
70-70: Consider updating to Pydantic v2 syntax.Same TODO comment pattern as other models. Consider updating consistently across all generated models if the project supports Pydantic v2.
- # TODO: pydantic v2: use .model_dump_json(by_alias=True, exclude_unset=True) instead - return json.dumps(self.to_dict()) + return self.model_dump_json(by_alias=True, exclude_unset=True)clients/python/src/mr_openapi/models/data_set.py (2)
120-122: Remove duplicate comment in docstring.There's a duplicate line in the
to_dict()method docstring.* `None` is only added to the output dict for nullable fields that were set at model initialization. Other fields with value `None` are ignored. * OpenAPI `readOnly` fields are excluded. - * OpenAPI `readOnly` fields are excluded.
25-26: Consider enhancing the class docstring.The current docstring is quite brief. Consider adding more detail about what constitutes a dataset artifact in this context.
-class DataSet(BaseModel): - """A dataset artifact representing training or test data.""" # noqa: E501 +class DataSet(BaseModel): + """A dataset artifact representing training, validation, or test data used in ML experiments.""" # noqa: E501clients/python/src/mr_openapi/models/metric.py (1)
106-108: Remove duplicate comment in docstring.Same issue as in the DataSet model - duplicate comment line.
* `None` is only added to the output dict for nullable fields that were set at model initialization. Other fields with value `None` are ignored. * OpenAPI `readOnly` fields are excluded. - * OpenAPI `readOnly` fields are excluded.clients/python/src/mr_openapi/models/experiment_run.py (1)
117-117: Remove duplicate commentThis line duplicates the comment from line 116.
- * OpenAPI `readOnly` fields are excluded.catalog/internal/catalog/rhec_catalog.go (1)
172-172: Consider making the GraphQL endpoint configurableThe GraphQL endpoint URL is hardcoded. Consider making it configurable through the catalog source configuration or environment variables for better flexibility and testability.
api/openapi/src/model-registry.yaml (1)
1883-1896: Possible type mismatch forParameter.value
ParameterTypeallowsnumber,boolean,object, yetParameterUpdate.valueis always a string. If numeric/boolean parameters are expected, consider makingvalueaoneOfunion, or store everything as string but document the expectation clearly.internal/core/artifact.go (3)
113-152: Fix error message capitalization for consistency.The error message uses "dataset artifact" while other artifact types use "model artifact", "doc artifact". Consider using "DataSet artifact" for consistency.
- return nil, fmt.Errorf("dataset artifact with name %s already exists: %w", *ds.Name, api.ErrConflict) + return nil, fmt.Errorf("DataSet artifact with name %s already exists: %w", *ds.Name, api.ErrConflict)
211-268: Remove the TODO-style comment.The comment "ADD PARAMETER SUPPORT" on line 212 appears to be a leftover TODO. Since the parameter support is fully implemented, this comment should be updated or removed.
- // ADD PARAMETER SUPPORT using embedmd converters + // Handle Parameter artifacts using embedmd converters
282-327: Consider consistent mapper method naming.The method
MapToMetricFromMetricon line 313 has a redundant naming pattern compared toMapToDataSetandMapToParameter. Consider renaming it toMapToMetricfor consistency.api/openapi/model-registry.yaml (1)
157-170: Missing “400 BadRequest” response for invalid query parametersUnlike most other list/search operations,
findExperimentlacks a400mapping.
Add it for consistency and to document how the server behaves when an invalidname/externalIdis supplied."400": $ref: "#/components/responses/BadRequest"internal/apiutils/query_translator.go (1)
43-99: Consider caching field mappings for better performance.The
getFieldMappingsfunction creates a new map on each call. If this function is called frequently during query translation, consider initializing the mappings once as a package-level variable to avoid repeated allocations.+var fieldMappings = initFieldMappings() + +func initFieldMappings() map[EntityType]map[string]FieldMapping { + return map[EntityType]map[string]FieldMapping{ + RegisteredModelEntity: { + // ... existing mappings + }, + // ... other entity mappings + } +} + -func getFieldMappings() map[EntityType]map[string]FieldMapping { - return map[EntityType]map[string]FieldMapping{ - // ... existing implementation - } +func getFieldMappings() map[EntityType]map[string]FieldMapping { + return fieldMappings +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
clients/ui/frontend/package-lock.jsonis excluded by!**/package-lock.jsongo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.suminternal/converter/generated/embedmd_openapi_converter.gen.gois excluded by!**/generated/**internal/converter/generated/mlmd_openapi_converter.gen.gois excluded by!**/generated/**internal/converter/generated/openapi_converter.gen.gois excluded by!**/generated/**internal/converter/generated/openapi_embedmd_converter.gen.gois excluded by!**/generated/**internal/converter/generated/openapi_mlmd_converter.gen.gois excluded by!**/generated/**internal/converter/generated/openapi_reconciler.gen.gois excluded by!**/generated/**
📒 Files selected for processing (107)
Makefile(3 hunks)api/openapi/catalog.yaml(3 hunks)api/openapi/model-registry.yaml(23 hunks)api/openapi/src/lib/common.yaml(5 hunks)api/openapi/src/model-registry.yaml(17 hunks)catalog/Makefile(2 hunks)catalog/internal/catalog/genqlient/README.md(1 hunks)catalog/internal/catalog/genqlient/generated.go(1 hunks)catalog/internal/catalog/genqlient/genqlient.yaml(1 hunks)catalog/internal/catalog/genqlient/queries/find_repository_images.graphql(1 hunks)catalog/internal/catalog/genqlient/queries/get_repository.graphql(1 hunks)catalog/internal/catalog/rhec_catalog.go(1 hunks)catalog/internal/catalog/rhec_catalog_test.go(1 hunks)catalog/internal/server/openapi/.openapi-generator/FILES(1 hunks)catalog/internal/server/openapi/type_asserts.go(1 hunks)catalog/pkg/openapi/.openapi-generator/FILES(1 hunks)catalog/pkg/openapi/model_artifact_type_query_param.go(1 hunks)clients/python/pyproject.toml(1 hunks)clients/python/src/.openapi-generator/FILES(3 hunks)clients/python/src/model_registry/__init__.py(1 hunks)clients/python/src/mr_openapi/README.md(7 hunks)clients/python/src/mr_openapi/__init__.py(3 hunks)clients/python/src/mr_openapi/models/__init__.py(3 hunks)clients/python/src/mr_openapi/models/artifact.py(6 hunks)clients/python/src/mr_openapi/models/artifact_create.py(6 hunks)clients/python/src/mr_openapi/models/artifact_type_query_param.py(1 hunks)clients/python/src/mr_openapi/models/artifact_update.py(6 hunks)clients/python/src/mr_openapi/models/base_resource.py(1 hunks)clients/python/src/mr_openapi/models/data_set.py(1 hunks)clients/python/src/mr_openapi/models/data_set_create.py(1 hunks)clients/python/src/mr_openapi/models/data_set_update.py(1 hunks)clients/python/src/mr_openapi/models/experiment.py(1 hunks)clients/python/src/mr_openapi/models/experiment_create.py(1 hunks)clients/python/src/mr_openapi/models/experiment_list.py(1 hunks)clients/python/src/mr_openapi/models/experiment_run.py(1 hunks)clients/python/src/mr_openapi/models/experiment_run_create.py(1 hunks)clients/python/src/mr_openapi/models/experiment_run_list.py(1 hunks)clients/python/src/mr_openapi/models/experiment_run_state.py(1 hunks)clients/python/src/mr_openapi/models/experiment_run_status.py(1 hunks)clients/python/src/mr_openapi/models/experiment_run_update.py(1 hunks)clients/python/src/mr_openapi/models/experiment_state.py(1 hunks)clients/python/src/mr_openapi/models/experiment_update.py(1 hunks)clients/python/src/mr_openapi/models/metric.py(1 hunks)clients/python/src/mr_openapi/models/metric_create.py(1 hunks)clients/python/src/mr_openapi/models/metric_update.py(1 hunks)clients/python/src/mr_openapi/models/model_version.py(1 hunks)clients/python/src/mr_openapi/models/order_by_field.py(1 hunks)clients/python/src/mr_openapi/models/parameter.py(1 hunks)clients/python/src/mr_openapi/models/parameter_create.py(1 hunks)clients/python/src/mr_openapi/models/parameter_type.py(1 hunks)clients/python/src/mr_openapi/models/parameter_update.py(1 hunks)clients/ui/bff/internal/api/model_registry_settings_handler.go(2 hunks)clients/ui/frontend/src/app/api/k8s.ts(1 hunks)clients/ui/frontend/src/app/pages/settings/DeleteModelRegistryModal.tsx(2 hunks)go.mod(2 hunks)internal/apiutils/api_utils.go(3 hunks)internal/apiutils/query_translator.go(1 hunks)internal/apiutils/query_translator_test.go(1 hunks)internal/converter/embdemd_openapi_converter.go(1 hunks)internal/converter/embdemd_openapi_converter_util.go(1 hunks)internal/converter/mlmd_converter_util_test.go(4 hunks)internal/converter/mlmd_openapi_converter.go(2 hunks)internal/converter/mlmd_openapi_converter_util.go(2 hunks)internal/converter/openapi_converter.go(6 hunks)internal/converter/openapi_converter_test.go(2 hunks)internal/converter/openapi_converter_util.go(1 hunks)internal/converter/openapi_embedmd_converter.go(2 hunks)internal/converter/openapi_embedmd_converter_util.go(7 hunks)internal/converter/openapi_embedmd_converter_util_test.go(9 hunks)internal/converter/openapi_mlmd_converter.go(2 hunks)internal/converter/openapi_mlmd_converter_util.go(4 hunks)internal/converter/openapi_reconciler.go(2 hunks)internal/converter/openapi_reconciler_util.go(1 hunks)internal/core/artifact.go(11 hunks)internal/core/artifact_test.go(27 hunks)internal/core/core_test.go(1 hunks)internal/core/experiment.go(1 hunks)internal/core/experiment_run.go(1 hunks)internal/core/experiment_run_test.go(1 hunks)internal/core/experiment_test.go(1 hunks)internal/core/inference_service.go(1 hunks)internal/core/inference_service_test.go(5 hunks)internal/core/metric_history_test.go(1 hunks)internal/core/model_version.go(2 hunks)internal/core/model_version_test.go(6 hunks)internal/core/modelregistry_service.go(1 hunks)internal/core/non_editable_fields_test.go(1 hunks)internal/core/partial_update_test.go(1 hunks)internal/core/registered_model.go(1 hunks)internal/core/registered_model_test.go(6 hunks)internal/core/serve_model.go(1 hunks)internal/core/serve_model_test.go(4 hunks)internal/core/serving_environment.go(1 hunks)internal/core/serving_environment_test.go(5 hunks)internal/core/utils_test.go(0 hunks)internal/datastore/embedmd/mysql/connect_test.go(6 hunks)internal/datastore/embedmd/mysql/migrate_test.go(3 hunks)internal/datastore/embedmd/mysql/migrations/000020_seed_experiment_types.down.sql(1 hunks)internal/datastore/embedmd/mysql/migrations/000020_seed_experiment_types.up.sql(1 hunks)internal/datastore/embedmd/mysql/migrations/000021_add_experiment_type_properties.down.sql(1 hunks)internal/datastore/embedmd/mysql/migrations/000021_add_experiment_type_properties.up.sql(1 hunks)internal/datastore/embedmd/postgres/migrate.go(1 hunks)internal/datastore/embedmd/postgres/migrations/000020_seed_experiment_types.down.sql(1 hunks)internal/datastore/embedmd/postgres/migrations/000020_seed_experiment_types.up.sql(1 hunks)internal/datastore/embedmd/postgres/migrations/000021_add_experiment_type_properties.down.sql(1 hunks)internal/datastore/embedmd/postgres/migrations/000021_add_experiment_type_properties.up.sql(1 hunks)internal/datastore/embedmd/service.go(2 hunks)
⛔ Files not processed due to max files limit (20)
- internal/db/filter/cross_database_test.go
- internal/db/filter/parser.go
- internal/db/filter/parser_test.go
- internal/db/filter/property_mapping.go
- internal/db/filter/query_builder.go
- internal/db/filter/query_builder_test.go
- internal/db/filter/rest_entity_mapping.go
- internal/db/filter/rest_entity_property_test.go
- internal/db/models/artifact.go
- internal/db/models/dataset.go
- internal/db/models/doc_artifact.go
- internal/db/models/experiment.go
- internal/db/models/experiment_run.go
- internal/db/models/metric.go
- internal/db/models/metric_history.go
- internal/db/models/model_artifact.go
- internal/db/models/model_version.go
- internal/db/models/pagination.go
- internal/db/models/parameter.go
- internal/db/models/serve_model.go
🧬 Code Graph Analysis (30)
internal/core/serving_environment.go (1)
internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)
internal/converter/openapi_converter_test.go (3)
pkg/openapi/model_data_set.go (1)
DataSet(21-50)pkg/openapi/model_metric.go (1)
Metric(21-44)pkg/openapi/model_parameter.go (1)
Parameter(21-41)
internal/core/registered_model.go (1)
internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)
clients/ui/frontend/src/app/pages/settings/DeleteModelRegistryModal.tsx (1)
clients/ui/frontend/src/app/api/k8s.ts (1)
deleteModelRegistrySettings(153-173)
internal/core/inference_service.go (1)
internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)
internal/converter/openapi_converter_util.go (5)
pkg/openapi/model_data_set.go (1)
DataSet(21-50)pkg/openapi/model_metric.go (1)
Metric(21-44)pkg/openapi/model_parameter.go (1)
Parameter(21-41)pkg/openapi/model_experiment.go (1)
Experiment(21-38)pkg/openapi/model_experiment_run.go (1)
ExperimentRun(21-46)
internal/core/serve_model_test.go (1)
internal/core/core_test.go (1)
SetupModelRegistryService(109-116)
internal/core/registered_model_test.go (1)
internal/core/core_test.go (1)
SetupModelRegistryService(109-116)
internal/core/serving_environment_test.go (1)
internal/core/core_test.go (1)
SetupModelRegistryService(109-116)
internal/core/inference_service_test.go (1)
internal/core/core_test.go (1)
SetupModelRegistryService(109-116)
internal/core/serve_model.go (2)
internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)pkg/api/error.go (1)
ErrBadRequest(12-12)
internal/core/model_version_test.go (1)
internal/core/core_test.go (1)
SetupModelRegistryService(109-116)
internal/datastore/embedmd/mysql/connect_test.go (3)
internal/testutils/mysql_test_utils.go (2)
GetSharedMySQLDB(57-78)GetSharedMySQLDSN(81-87)internal/datastore/embedmd/mysql/connect.go (1)
NewMySQLDBConnector(29-38)internal/tls/config.go (1)
TLSConfig(11-18)
clients/python/src/mr_openapi/models/parameter_type.py (6)
clients/python/src/mr_openapi/models/artifact.py (1)
from_json(118-223)clients/python/src/mr_openapi/models/artifact_type_query_param.py (1)
from_json(32-34)clients/python/src/mr_openapi/models/artifact_create.py (1)
from_json(132-237)clients/python/src/mr_openapi/models/artifact_update.py (1)
from_json(132-237)clients/python/src/mr_openapi/models/experiment_run_state.py (1)
from_json(29-31)clients/python/src/mr_openapi/models/experiment_run_status.py (1)
from_json(32-34)
internal/converter/openapi_reconciler_util.go (4)
pkg/openapi/model_data_set.go (1)
DataSet(21-50)internal/converter/openapi_converter_util.go (1)
OpenapiUpdateWrapper(21-26)pkg/openapi/model_metric.go (1)
Metric(21-44)pkg/openapi/model_parameter.go (1)
Parameter(21-41)
clients/ui/bff/internal/api/model_registry_settings_handler.go (1)
clients/ui/bff/internal/api/app.go (2)
App(60-67)ModelRegistryId(29-29)
catalog/internal/catalog/rhec_catalog_test.go (3)
catalog/pkg/openapi/model_catalog_model.go (1)
CatalogModel(21-51)catalog/pkg/openapi/model_catalog_model_artifact.go (1)
CatalogModelArtifact(21-30)catalog/pkg/openapi/model_catalog_model_artifact_list.go (1)
CatalogModelArtifactList(21-30)
internal/datastore/embedmd/mysql/migrate_test.go (2)
internal/testutils/mysql_test_utils.go (2)
TestMainHelper(147-162)GetSharedMySQLDB(57-78)internal/datastore/embedmd/postgres/migrate_test.go (1)
TestMigrations(171-198)
clients/python/src/mr_openapi/models/parameter_create.py (1)
clients/python/src/mr_openapi/models/parameter_type.py (1)
ParameterType(19-33)
internal/core/experiment.go (7)
internal/core/modelregistry_service.go (1)
ModelRegistryService(12-29)internal/db/models/experiment.go (2)
Experiment(16-18)ExperimentListOptions(3-7)pkg/api/error.go (3)
ErrBadRequest(12-12)ErrConflict(14-14)ErrNotFound(13-13)internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)internal/apiutils/api_utils.go (1)
ZeroIfNil(46-51)pkg/api/api.go (1)
ListOptions(7-13)internal/db/models/pagination.go (1)
Pagination(15-21)
internal/apiutils/api_utils.go (6)
pkg/openapi/model_order_by_field.go (1)
OrderByField(19-19)pkg/openapi/model_sort_order.go (1)
SortOrder(19-19)pkg/api/api.go (1)
ListOptions(7-13)internal/db/filter/query_builder.go (1)
EntityType(10-10)internal/apiutils/query_translator.go (2)
EntityType(18-18)TranslateFilterQuery(185-257)pkg/api/error.go (1)
ErrBadRequest(12-12)
internal/converter/embdemd_openapi_converter.go (5)
internal/db/models/experiment.go (2)
ExperimentImpl(20-20)Experiment(16-18)internal/db/models/experiment_run.go (2)
ExperimentRunImpl(28-28)ExperimentRun(24-26)internal/db/models/dataset.go (2)
DataSetImpl(26-26)DataSet(22-24)internal/db/models/metric.go (2)
MetricImpl(26-26)Metric(22-24)internal/db/models/parameter.go (2)
ParameterImpl(26-26)Parameter(22-24)
internal/converter/openapi_embedmd_converter.go (6)
internal/converter/openapi_mlmd_converter.go (1)
OpenAPIModelWrapper(8-15)internal/db/models/experiment.go (2)
Experiment(16-18)ExperimentImpl(20-20)internal/db/models/experiment_run.go (2)
ExperimentRun(24-26)ExperimentRunImpl(28-28)internal/db/models/dataset.go (2)
DataSet(22-24)DataSetImpl(26-26)internal/db/models/metric.go (2)
Metric(22-24)MetricImpl(26-26)internal/db/models/parameter.go (2)
Parameter(22-24)ParameterImpl(26-26)
clients/python/src/mr_openapi/models/artifact_create.py (6)
clients/python/src/mr_openapi/models/data_set_create.py (3)
DataSetCreate(25-151)from_json(93-95)to_dict(97-121)clients/python/src/mr_openapi/models/doc_artifact_create.py (1)
DocArtifactCreate(25-128)clients/python/src/mr_openapi/models/metric_create.py (2)
MetricCreate(25-134)to_dict(83-107)clients/python/src/mr_openapi/models/model_artifact_create.py (1)
ModelArtifactCreate(25-186)clients/python/src/mr_openapi/models/parameter_create.py (1)
ParameterCreate(26-128)clients/python/src/mr_openapi/models/artifact.py (2)
from_json(118-223)to_dict(234-242)
clients/python/src/mr_openapi/models/data_set.py (1)
clients/python/src/mr_openapi/models/artifact.py (5)
to_str(244-246)to_json(225-232)to_dict(234-242)from_json(118-223)from_dict(114-115)
internal/converter/openapi_embedmd_converter_util_test.go (6)
internal/converter/openapi_mlmd_converter.go (1)
OpenAPIModelWrapper(8-15)internal/db/models/experiment_run.go (1)
ExperimentRunAttributes(17-22)internal/converter/openapi_embedmd_converter_util.go (3)
MapExperimentRunAttributesEmbedMD(883-907)MapMetricAttributesEmbedMD(1055-1091)MapParameterAttributesEmbedMD(1132-1168)pkg/openapi/model_artifact_state.go (1)
ARTIFACTSTATE_LIVE(25-25)internal/db/models/metric.go (1)
MetricAttributes(12-20)internal/db/models/parameter.go (1)
ParameterAttributes(12-20)
clients/python/src/mr_openapi/models/artifact.py (3)
clients/python/src/mr_openapi/models/data_set.py (3)
DataSet(25-173)from_json(107-109)to_dict(111-140)clients/python/src/mr_openapi/models/metric.py (1)
Metric(25-156)clients/python/src/mr_openapi/models/parameter.py (1)
Parameter(26-150)
internal/core/experiment_run.go (9)
internal/core/modelregistry_service.go (1)
ModelRegistryService(12-29)internal/db/models/experiment_run.go (2)
ExperimentRun(24-26)ExperimentRunListOptions(5-10)pkg/api/error.go (3)
ErrBadRequest(12-12)ErrConflict(14-14)ErrNotFound(13-13)internal/converter/openapi_mlmd_converter_util.go (1)
PrefixWhenOwned(107-120)internal/apiutils/api_utils.go (3)
ZeroIfNil(46-51)StrPtr(58-63)Of(54-56)internal/db/models/artifact.go (1)
Artifact(32-38)internal/db/models/metric_history.go (4)
MetricHistoryListOptions(5-11)MetricHistoryImpl(27-27)MetricHistoryAttributes(13-21)MetricHistoryType(3-3)internal/defaults/defaults.go (1)
MetricHistoryTypeName(16-16)internal/converter/openapi_embedmd_converter_util.go (2)
MapMetricPropertiesEmbedMD(1014-1052)MapOpenAPICustomPropertiesEmbedMD(41-94)
internal/converter/openapi_mlmd_converter_util.go (9)
pkg/openapi/model_data_set.go (1)
DataSet(21-50)internal/defaults/defaults.go (5)
DataSetTypeName(14-14)MetricTypeName(15-15)ParameterTypeName(17-17)ExperimentTypeName(12-12)ExperimentRunTypeName(13-13)internal/converter/openapi_mlmd_converter.go (1)
OpenAPIModelWrapper(8-15)internal/ml_metadata/proto/metadata_store.pb.go (9)
Value(796-810)Value(825-825)Value(840-842)Value_StringValue(905-907)Value_StringValue(925-925)Value_DoubleValue(901-903)Value_DoubleValue(923-923)Value_IntValue(897-899)Value_IntValue(921-921)pkg/openapi/model_metric.go (1)
Metric(21-44)pkg/openapi/model_parameter.go (1)
Parameter(21-41)pkg/openapi/model_parameter_type.go (1)
ParameterType(19-19)pkg/openapi/model_experiment.go (1)
Experiment(21-38)pkg/openapi/model_experiment_run.go (1)
ExperimentRun(21-46)
internal/core/artifact.go (14)
internal/core/modelregistry_service.go (1)
ModelRegistryService(12-29)internal/db/models/artifact.go (1)
Artifact(32-38)internal/db/schema/artifact.gen.go (2)
Artifact(10-19)Artifact(22-24)pkg/api/error.go (4)
ErrBadRequest(12-12)ErrConflict(14-14)IgnoreNotFound(45-52)ErrNotFound(13-13)internal/db/models/model_artifact.go (1)
ModelArtifact(29-31)internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)internal/db/models/doc_artifact.go (1)
DocArtifact(22-24)internal/db/models/dataset.go (1)
DataSet(22-24)internal/db/models/metric.go (1)
Metric(22-24)catalog/pkg/openapi/model_artifact_type_query_param.go (3)
ARTIFACTTYPEQUERYPARAM_METRIC(26-26)ArtifactTypeQueryParam(19-19)ARTIFACTTYPEQUERYPARAM_MODEL_ARTIFACT(23-23)internal/db/models/parameter.go (1)
Parameter(22-24)internal/converter/openapi_mlmd_converter_util.go (1)
PrefixWhenOwned(107-120)internal/apiutils/api_utils.go (1)
ZeroIfNil(46-51)pkg/api/api.go (1)
ListOptions(7-13)
🪛 markdownlint-cli2 (0.17.2)
catalog/internal/catalog/genqlient/README.md
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
clients/python/src/mr_openapi/README.md
139-139: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
140-140: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
141-141: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
146-146: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
147-147: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
149-149: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
150-150: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
151-151: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
152-152: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
153-153: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
154-154: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
155-155: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
156-156: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
157-157: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
158-158: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
159-159: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
160-160: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
161-161: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
162-162: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
163-163: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
164-164: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
165-165: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
178-178: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
179-179: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
180-180: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
191-191: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
192-192: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
193-193: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
194-194: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🪛 YAMLlint (1.37.1)
catalog/internal/catalog/genqlient/genqlient.yaml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
[error] 10-10: trailing spaces
(trailing-spaces)
💤 Files with no reviewable changes (1)
- internal/core/utils_test.go
🧰 Additional context used
🧬 Code Graph Analysis (30)
internal/core/serving_environment.go (1)
internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)
internal/converter/openapi_converter_test.go (3)
pkg/openapi/model_data_set.go (1)
DataSet(21-50)pkg/openapi/model_metric.go (1)
Metric(21-44)pkg/openapi/model_parameter.go (1)
Parameter(21-41)
internal/core/registered_model.go (1)
internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)
clients/ui/frontend/src/app/pages/settings/DeleteModelRegistryModal.tsx (1)
clients/ui/frontend/src/app/api/k8s.ts (1)
deleteModelRegistrySettings(153-173)
internal/core/inference_service.go (1)
internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)
internal/converter/openapi_converter_util.go (5)
pkg/openapi/model_data_set.go (1)
DataSet(21-50)pkg/openapi/model_metric.go (1)
Metric(21-44)pkg/openapi/model_parameter.go (1)
Parameter(21-41)pkg/openapi/model_experiment.go (1)
Experiment(21-38)pkg/openapi/model_experiment_run.go (1)
ExperimentRun(21-46)
internal/core/serve_model_test.go (1)
internal/core/core_test.go (1)
SetupModelRegistryService(109-116)
internal/core/registered_model_test.go (1)
internal/core/core_test.go (1)
SetupModelRegistryService(109-116)
internal/core/serving_environment_test.go (1)
internal/core/core_test.go (1)
SetupModelRegistryService(109-116)
internal/core/inference_service_test.go (1)
internal/core/core_test.go (1)
SetupModelRegistryService(109-116)
internal/core/serve_model.go (2)
internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)pkg/api/error.go (1)
ErrBadRequest(12-12)
internal/core/model_version_test.go (1)
internal/core/core_test.go (1)
SetupModelRegistryService(109-116)
internal/datastore/embedmd/mysql/connect_test.go (3)
internal/testutils/mysql_test_utils.go (2)
GetSharedMySQLDB(57-78)GetSharedMySQLDSN(81-87)internal/datastore/embedmd/mysql/connect.go (1)
NewMySQLDBConnector(29-38)internal/tls/config.go (1)
TLSConfig(11-18)
clients/python/src/mr_openapi/models/parameter_type.py (6)
clients/python/src/mr_openapi/models/artifact.py (1)
from_json(118-223)clients/python/src/mr_openapi/models/artifact_type_query_param.py (1)
from_json(32-34)clients/python/src/mr_openapi/models/artifact_create.py (1)
from_json(132-237)clients/python/src/mr_openapi/models/artifact_update.py (1)
from_json(132-237)clients/python/src/mr_openapi/models/experiment_run_state.py (1)
from_json(29-31)clients/python/src/mr_openapi/models/experiment_run_status.py (1)
from_json(32-34)
internal/converter/openapi_reconciler_util.go (4)
pkg/openapi/model_data_set.go (1)
DataSet(21-50)internal/converter/openapi_converter_util.go (1)
OpenapiUpdateWrapper(21-26)pkg/openapi/model_metric.go (1)
Metric(21-44)pkg/openapi/model_parameter.go (1)
Parameter(21-41)
clients/ui/bff/internal/api/model_registry_settings_handler.go (1)
clients/ui/bff/internal/api/app.go (2)
App(60-67)ModelRegistryId(29-29)
catalog/internal/catalog/rhec_catalog_test.go (3)
catalog/pkg/openapi/model_catalog_model.go (1)
CatalogModel(21-51)catalog/pkg/openapi/model_catalog_model_artifact.go (1)
CatalogModelArtifact(21-30)catalog/pkg/openapi/model_catalog_model_artifact_list.go (1)
CatalogModelArtifactList(21-30)
internal/datastore/embedmd/mysql/migrate_test.go (2)
internal/testutils/mysql_test_utils.go (2)
TestMainHelper(147-162)GetSharedMySQLDB(57-78)internal/datastore/embedmd/postgres/migrate_test.go (1)
TestMigrations(171-198)
clients/python/src/mr_openapi/models/parameter_create.py (1)
clients/python/src/mr_openapi/models/parameter_type.py (1)
ParameterType(19-33)
internal/core/experiment.go (7)
internal/core/modelregistry_service.go (1)
ModelRegistryService(12-29)internal/db/models/experiment.go (2)
Experiment(16-18)ExperimentListOptions(3-7)pkg/api/error.go (3)
ErrBadRequest(12-12)ErrConflict(14-14)ErrNotFound(13-13)internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)internal/apiutils/api_utils.go (1)
ZeroIfNil(46-51)pkg/api/api.go (1)
ListOptions(7-13)internal/db/models/pagination.go (1)
Pagination(15-21)
internal/apiutils/api_utils.go (6)
pkg/openapi/model_order_by_field.go (1)
OrderByField(19-19)pkg/openapi/model_sort_order.go (1)
SortOrder(19-19)pkg/api/api.go (1)
ListOptions(7-13)internal/db/filter/query_builder.go (1)
EntityType(10-10)internal/apiutils/query_translator.go (2)
EntityType(18-18)TranslateFilterQuery(185-257)pkg/api/error.go (1)
ErrBadRequest(12-12)
internal/converter/embdemd_openapi_converter.go (5)
internal/db/models/experiment.go (2)
ExperimentImpl(20-20)Experiment(16-18)internal/db/models/experiment_run.go (2)
ExperimentRunImpl(28-28)ExperimentRun(24-26)internal/db/models/dataset.go (2)
DataSetImpl(26-26)DataSet(22-24)internal/db/models/metric.go (2)
MetricImpl(26-26)Metric(22-24)internal/db/models/parameter.go (2)
ParameterImpl(26-26)Parameter(22-24)
internal/converter/openapi_embedmd_converter.go (6)
internal/converter/openapi_mlmd_converter.go (1)
OpenAPIModelWrapper(8-15)internal/db/models/experiment.go (2)
Experiment(16-18)ExperimentImpl(20-20)internal/db/models/experiment_run.go (2)
ExperimentRun(24-26)ExperimentRunImpl(28-28)internal/db/models/dataset.go (2)
DataSet(22-24)DataSetImpl(26-26)internal/db/models/metric.go (2)
Metric(22-24)MetricImpl(26-26)internal/db/models/parameter.go (2)
Parameter(22-24)ParameterImpl(26-26)
clients/python/src/mr_openapi/models/artifact_create.py (6)
clients/python/src/mr_openapi/models/data_set_create.py (3)
DataSetCreate(25-151)from_json(93-95)to_dict(97-121)clients/python/src/mr_openapi/models/doc_artifact_create.py (1)
DocArtifactCreate(25-128)clients/python/src/mr_openapi/models/metric_create.py (2)
MetricCreate(25-134)to_dict(83-107)clients/python/src/mr_openapi/models/model_artifact_create.py (1)
ModelArtifactCreate(25-186)clients/python/src/mr_openapi/models/parameter_create.py (1)
ParameterCreate(26-128)clients/python/src/mr_openapi/models/artifact.py (2)
from_json(118-223)to_dict(234-242)
clients/python/src/mr_openapi/models/data_set.py (1)
clients/python/src/mr_openapi/models/artifact.py (5)
to_str(244-246)to_json(225-232)to_dict(234-242)from_json(118-223)from_dict(114-115)
internal/converter/openapi_embedmd_converter_util_test.go (6)
internal/converter/openapi_mlmd_converter.go (1)
OpenAPIModelWrapper(8-15)internal/db/models/experiment_run.go (1)
ExperimentRunAttributes(17-22)internal/converter/openapi_embedmd_converter_util.go (3)
MapExperimentRunAttributesEmbedMD(883-907)MapMetricAttributesEmbedMD(1055-1091)MapParameterAttributesEmbedMD(1132-1168)pkg/openapi/model_artifact_state.go (1)
ARTIFACTSTATE_LIVE(25-25)internal/db/models/metric.go (1)
MetricAttributes(12-20)internal/db/models/parameter.go (1)
ParameterAttributes(12-20)
clients/python/src/mr_openapi/models/artifact.py (3)
clients/python/src/mr_openapi/models/data_set.py (3)
DataSet(25-173)from_json(107-109)to_dict(111-140)clients/python/src/mr_openapi/models/metric.py (1)
Metric(25-156)clients/python/src/mr_openapi/models/parameter.py (1)
Parameter(26-150)
internal/core/experiment_run.go (9)
internal/core/modelregistry_service.go (1)
ModelRegistryService(12-29)internal/db/models/experiment_run.go (2)
ExperimentRun(24-26)ExperimentRunListOptions(5-10)pkg/api/error.go (3)
ErrBadRequest(12-12)ErrConflict(14-14)ErrNotFound(13-13)internal/converter/openapi_mlmd_converter_util.go (1)
PrefixWhenOwned(107-120)internal/apiutils/api_utils.go (3)
ZeroIfNil(46-51)StrPtr(58-63)Of(54-56)internal/db/models/artifact.go (1)
Artifact(32-38)internal/db/models/metric_history.go (4)
MetricHistoryListOptions(5-11)MetricHistoryImpl(27-27)MetricHistoryAttributes(13-21)MetricHistoryType(3-3)internal/defaults/defaults.go (1)
MetricHistoryTypeName(16-16)internal/converter/openapi_embedmd_converter_util.go (2)
MapMetricPropertiesEmbedMD(1014-1052)MapOpenAPICustomPropertiesEmbedMD(41-94)
internal/converter/openapi_mlmd_converter_util.go (9)
pkg/openapi/model_data_set.go (1)
DataSet(21-50)internal/defaults/defaults.go (5)
DataSetTypeName(14-14)MetricTypeName(15-15)ParameterTypeName(17-17)ExperimentTypeName(12-12)ExperimentRunTypeName(13-13)internal/converter/openapi_mlmd_converter.go (1)
OpenAPIModelWrapper(8-15)internal/ml_metadata/proto/metadata_store.pb.go (9)
Value(796-810)Value(825-825)Value(840-842)Value_StringValue(905-907)Value_StringValue(925-925)Value_DoubleValue(901-903)Value_DoubleValue(923-923)Value_IntValue(897-899)Value_IntValue(921-921)pkg/openapi/model_metric.go (1)
Metric(21-44)pkg/openapi/model_parameter.go (1)
Parameter(21-41)pkg/openapi/model_parameter_type.go (1)
ParameterType(19-19)pkg/openapi/model_experiment.go (1)
Experiment(21-38)pkg/openapi/model_experiment_run.go (1)
ExperimentRun(21-46)
internal/core/artifact.go (14)
internal/core/modelregistry_service.go (1)
ModelRegistryService(12-29)internal/db/models/artifact.go (1)
Artifact(32-38)internal/db/schema/artifact.gen.go (2)
Artifact(10-19)Artifact(22-24)pkg/api/error.go (4)
ErrBadRequest(12-12)ErrConflict(14-14)IgnoreNotFound(45-52)ErrNotFound(13-13)internal/db/models/model_artifact.go (1)
ModelArtifact(29-31)internal/converter/openapi_converter_util.go (1)
NewOpenapiUpdateWrapper(28-35)internal/db/models/doc_artifact.go (1)
DocArtifact(22-24)internal/db/models/dataset.go (1)
DataSet(22-24)internal/db/models/metric.go (1)
Metric(22-24)catalog/pkg/openapi/model_artifact_type_query_param.go (3)
ARTIFACTTYPEQUERYPARAM_METRIC(26-26)ArtifactTypeQueryParam(19-19)ARTIFACTTYPEQUERYPARAM_MODEL_ARTIFACT(23-23)internal/db/models/parameter.go (1)
Parameter(22-24)internal/converter/openapi_mlmd_converter_util.go (1)
PrefixWhenOwned(107-120)internal/apiutils/api_utils.go (1)
ZeroIfNil(46-51)pkg/api/api.go (1)
ListOptions(7-13)
🪛 markdownlint-cli2 (0.17.2)
catalog/internal/catalog/genqlient/README.md
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
clients/python/src/mr_openapi/README.md
139-139: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
140-140: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
141-141: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
146-146: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
147-147: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
149-149: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
150-150: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
151-151: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
152-152: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
153-153: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
154-154: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
155-155: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
156-156: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
157-157: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
158-158: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
159-159: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
160-160: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
161-161: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
162-162: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
163-163: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
164-164: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
165-165: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
178-178: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
179-179: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
180-180: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
191-191: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
192-192: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
193-193: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
194-194: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🪛 YAMLlint (1.37.1)
catalog/internal/catalog/genqlient/genqlient.yaml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
[error] 10-10: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: prepare / prepare
- GitHub Check: build
🔇 Additional comments (244)
clients/python/src/model_registry/__init__.py (1)
3-3: Version constant update matchespyproject.toml– LGTMNo further action needed.
catalog/internal/server/openapi/.openapi-generator/FILES (1)
7-7: New file entry added – re-run code-gen & vetAfter adding
model_artifact_type_query_param.go, ensurego generate/make genhas been executed so that generated stubs compile cleanly with the rest of the server.internal/datastore/embedmd/postgres/migrate.go (1)
76-86: Whitespace cleanup looks goodCosmetic-only change; no impact on migration logic.
clients/python/src/mr_openapi/models/base_resource.py (1)
45-45: Approve typographical correction, but ensure consistency at source level.The apostrophe correction improves documentation quality. However, since this is a generated file (as noted in the header), ensure this fix is also applied to the source OpenAPI specification to maintain consistency across regenerations.
catalog/pkg/openapi/.openapi-generator/FILES (1)
4-4: LGTM - Necessary addition for artifact type query parameter.The addition of
model_artifact_type_query_param.gocorrectly supports the new artifact type querying functionality introduced in this PR.internal/datastore/embedmd/mysql/migrations/000020_seed_experiment_types.down.sql (1)
1-8: Well-structured rollback migration with data safety consideration.The migration correctly removes the experiment-related types using an efficient IN clause. The syntax is correct and aligns with the new experiment tracking features.
Consider the impact on existing data: if experiments or artifacts using these types exist, they may become orphaned after this rollback. Ensure appropriate warnings are provided to administrators about data loss implications.
clients/python/src/mr_openapi/models/order_by_field.py (1)
28-28: LGTM - Logical addition for enhanced sorting capabilities.Adding
NAMEto theOrderByFieldenum is a sensible enhancement that supports better user experience with name-based sorting. The implementation follows the established pattern and is backward compatible.internal/datastore/embedmd/postgres/migrations/000020_seed_experiment_types.down.sql (1)
1-8: LGTM! Clean rollback migration.The down migration properly removes the experiment-related types using a targeted DELETE with IN clause, providing a clean rollback path for the up migration.
internal/core/registered_model.go (1)
27-27: LGTM! Improved method naming for update operations.The method name change from
OverrideNotEditableForRegisteredModeltoUpdateExistingRegisteredModelprovides better clarity about the operation's intent and aligns with the standardized update handling pattern across entity types.clients/python/src/mr_openapi/models/model_version.py (1)
36-36: Minor typographical improvement.Good catch on using the correct apostrophe character in the field description.
go.mod (4)
6-6: Good addition for query parsing support.The
alecthomas/participle/v2dependency supports the new query translator functionality for converting REST API filter queries into ML Metadata filter queries.
7-7: GraphQL client generation dependency added.The
Khan/genqlientdependency enables type-safe Go client generation for GraphQL APIs, supporting the new catalog functionality.
14-14: Promoted to direct dependency for testing.Moving
google/go-cmpfrom indirect to direct dependency makes sense given its use in new test files for deep comparison of complex data structures.
122-122: GraphQL parser indirect dependency.The
vektah/gqlparser/v2indirect dependency supports the GraphQL client generation functionality.internal/converter/openapi_converter_util.go (1)
11-18: LGTM! Interface extended to support new entity types.The addition of
DataSet,Metric,Parameter,Experiment, andExperimentRunto theOpenAPIModelinterface properly enables these new entity types to be handled by the conversion utilities, supporting the comprehensive experiment tracking functionality.clients/ui/frontend/src/app/pages/settings/DeleteModelRegistryModal.tsx (2)
13-13: Import addition looks good.The import for
deleteModelRegistrySettingsis correctly added to replace the placeholder deletion logic.
42-45: Deletion implementation correctly matches API signature.The API call properly follows the expected signature from the
deleteModelRegistrySettingsfunction:
- Empty string for
hostPath- Namespace passed in
queryParams- Empty options object, model registry data, and name as the final parameters
The implementation maintains existing error handling and state management patterns.
internal/converter/openapi_converter_test.go (2)
39-47: New artifact type entities correctly added to test visitor.The addition of
DataSet,Metric, andParameterentities to the test visitor's entity map properly extends test coverage for the new artifact types. Each entity is correctly initialized with its corresponding empty OpenAPI struct.
60-65: Experiment-related entities properly integrated into test framework.The
ExperimentandExperimentRunentities are correctly added to support testing of the new experiment functionality. The initialization pattern matches existing entities in the map.internal/core/inference_service.go (1)
28-32: Update method change aligns with consistent refactoring pattern.The change from
OverrideNotEditableForInferenceServicetoUpdateExistingInferenceServicewithNewOpenapiUpdateWrapperfollows the consistent pattern being applied across the codebase for update operations. This approach properly bundles the existing and updated models together for more coherent update handling.internal/datastore/embedmd/mysql/migrations/000021_add_experiment_type_properties.down.sql (1)
1-30: Down migration properly reverses experiment type properties.The SQL correctly deletes type properties for all experiment-related types using appropriate tuple matching with subqueries to resolve type IDs. The structure properly reverses the corresponding up migration by removing the properties that were added for experiment functionality.
catalog/internal/catalog/genqlient/queries/get_repository.graphql (1)
1-21: Well-structured GraphQL query following best practices.The
GetRepositoryquery is properly structured with:
- Correct parameter typing using required String types
- Logical organization with error and data sections
- Comprehensive field selection for repository metadata
- Proper error handling structure
The query follows GraphQL conventions and integrates well with the catalog functionality.
internal/core/serve_model_test.go (4)
15-16: LGTM! Consistent test setup refactoring.The change from direct cleanup to using
SetupModelRegistryService(t)standardizes test initialization and ensures proper resource management through deferred cleanup.
598-599: LGTM! Standardized test setup.Consistent with the refactoring pattern across core service tests for better resource management.
673-674: LGTM! Standardized test setup.Consistent with the refactoring pattern across core service tests for better resource management.
909-910: LGTM! Standardized test setup.Consistent with the refactoring pattern across core service tests for better resource management.
internal/datastore/embedmd/postgres/migrations/000020_seed_experiment_types.up.sql (1)
1-14: LGTM! Well-structured migration for experiment types.The migration correctly:
- Uses NOT EXISTS to prevent duplicate entries
- Efficiently inserts all required types with UNION ALL
- Assigns appropriate type_kind values (1 for artifacts, 2 for contexts)
- Supports the new experiment tracking functionality
The duplicate prevention logic ensures safe re-execution of the migration.
catalog/internal/server/openapi/type_asserts.go (1)
19-27: LGTM! Appropriate validation stubs for new artifact type query parameters.These functions follow the established pattern in the codebase and provide placeholders for future constraint and required field validation for
ArtifactTypeQueryParam. The stub implementation is consistent with the generated code pattern.internal/core/inference_service_test.go (5)
16-17: LGTM! Consistent test setup standardization.The change to use
SetupModelRegistryService(t)aligns with the refactoring pattern across core service tests, ensuring proper resource management and consistent test initialization.
489-490: LGTM! Standardized test setup.Consistent with the broader refactoring for better resource management across core service tests.
550-551: LGTM! Standardized test setup.Consistent with the broader refactoring for better resource management across core service tests.
639-640: LGTM! Standardized test setup.Consistent with the broader refactoring for better resource management across core service tests.
888-889: LGTM! Standardized test setup.Consistent with the broader refactoring for better resource management across core service tests.
catalog/internal/catalog/genqlient/README.md (1)
1-32: Well-structured documentation.The README provides comprehensive guidance for using Genqlient with clear sections covering file structure, client generation, and schema management. The instructions are clear and actionable.
internal/datastore/embedmd/mysql/migrations/000020_seed_experiment_types.up.sql (1)
1-13: Well-implemented migration with proper safeguards.The migration script correctly uses conditional insertion with
NOT EXISTSto prevent duplicate entries, ensuring idempotency. The bulk insert approach withUNION ALLis efficient, and the type categorization (type_kind 1 for artifacts, type_kind 2 for contexts) aligns with the ML metadata model.internal/core/registered_model_test.go (2)
15-16: Excellent test infrastructure improvement.The adoption of
SetupModelRegistryService(t)standardizes test setup and ensures proper resource cleanup through the deferred cleanup function. This pattern improves test isolation and maintainability.
325-326: Consistent application of the new test pattern.All test functions correctly adopt the standardized setup pattern, ensuring consistent resource management across the test suite.
Also applies to: 371-372, 424-425, 498-499, 573-574
internal/core/model_version.go (2)
28-28: Improved semantic clarity in update handling.The change from
OverrideNotEditableForModelVersiontoUpdateExistingModelVersionprovides clearer intent and aligns with the broader refactoring of update methods across core services.
39-39: Centralized name handling in mapper layer.Passing
registeredModelIddirectly to the mapper and removing explicit name prefixing centralizes naming logic, improving code organization and reducing duplication.Makefile (2)
229-232: Well-integrated tool addition.The genqlient installation follows the established pattern for other tools in the Makefile, with proper version pinning and consistent naming conventions.
256-256: Proper dependency management.Adding
bin/genqlientto the deps target ensures the GraphQL client generation tool is available during the build process.internal/core/model_version_test.go (1)
16-17: LGTM! Consistent test setup refactoring.All test functions have been consistently updated to use the new
SetupModelRegistryService(t)pattern, which improves test isolation and resource management by providing a proper cleanup mechanism. This aligns with the broader refactoring across the core test suite.Also applies to: 432-433, 487-488, 599-600, 678-679, 805-806
catalog/internal/catalog/genqlient/queries/find_repository_images.graphql (1)
1-29: Well-structured GraphQL query for repository image retrieval.The query is properly formed with:
- Required parameters with correct typing (
registryandrepository)- Comprehensive error handling structure
- Sensible default sorting by creation date (descending for newest first)
- Complete data retrieval including metadata, repository info, and parsed labels
- Clean, readable formatting
This integrates well with the Red Hat Ecosystem Catalog (RHEC) GraphQL API client functionality.
clients/python/src/mr_openapi/models/parameter_type.py (1)
19-33: Well-implemented enum class following established patterns.The
ParameterTypeenum is correctly implemented with:
- Proper inheritance from
strandEnum- Comprehensive parameter types (string, number, boolean, object)
- Consistent
from_jsonclass method matching other model classes in the codebase- Clear documentation and formatting
This integrates well with the new parameter artifact support in the model registry.
internal/core/serve_model.go (2)
27-27: Updated mapper method call aligns with refactoring pattern.The change from
OverrideNotEditableForServeModeltoUpdateExistingServeModelaligns with the broader refactoring of update operations across core services, providing a more consistent update pattern.
34-34: Enhanced mapper call with inference service relationship.Adding the
inferenceServiceIdparameter toMapFromServeModelimproves ownership and relationship tracking, supporting more flexible artifact management in the updated architecture.internal/core/serving_environment_test.go (1)
15-16: LGTM! Consistent test infrastructure refactoring.All serving environment tests have been updated to use the standardized
SetupModelRegistryService(t)pattern, matching the refactoring applied across other core test files. This provides better test isolation and proper resource cleanup.Also applies to: 298-299, 342-343, 400-401, 475-476
internal/converter/mlmd_converter_util_test.go (2)
349-349: LGTM: Test expectations correctly updated for nil name handlingThese test changes properly reflect the updated converter logic that now returns
nilwhen the input model wrapper'sNamefield isnil, supporting patch requests without names. This is a sensible design improvement for partial updates.Also applies to: 407-407
842-842: LGTM: Error message expectations updated for ParseInt usageThe test expectations have been correctly updated to reflect the change from
strconv.Atoitostrconv.ParseIntin the underlying converter utilities. UsingParseIntis generally preferred as it provides more control over parsing behavior.Also applies to: 873-873
clients/python/src/mr_openapi/models/experiment_run_status.py (1)
1-35: LGTM: Well-structured generated enum for experiment run statusThis auto-generated Python enum properly defines the experiment run lifecycle states with clear documentation and a reasonable state transition model. The implementation follows Python conventions correctly.
Note: As indicated in the header, this is generated code and should not be manually edited.
internal/datastore/embedmd/mysql/migrations/000021_add_experiment_type_properties.up.sql (1)
1-28: Data type codes are consistent between MySQL and PostgreSQL migrationsWe’ve confirmed both migrations use 5 entries with code 1, 20 entries with code 3, and 2 entries with code 5. No discrepancies were found—formats differ due to quoting and dialect but the numeric codes match exactly.
No further changes required; migration can be approved as is.
internal/datastore/embedmd/postgres/migrations/000021_add_experiment_type_properties.up.sql (1)
1-29: Approve: PostgreSQL migration matches MySQL versionAll 27 property-name and data-type pairs in
internal/datastore/embedmd/postgres/migrations/000021_add_experiment_type_properties.up.sqlhave been verified against the MySQL counterpart and are identical. LGTM.internal/converter/openapi_reconciler_util.go (1)
32-54: LGTM! Consistent implementation pattern maintained.The new artifact type handling follows the exact same pattern as the existing ModelArtifact and DocArtifact implementations. The error handling, null checks, and update logic are all consistent and correct.
internal/datastore/embedmd/postgres/migrations/000021_add_experiment_type_properties.down.sql (1)
1-31: All up migrations present with matching data types
LGTM! I’ve verified that both MySQL and PostgreSQL up migrations exist (000021_add_experiment_type_properties.up.sql) and define the exact same property names and data_type values as this down migration. No further changes needed.internal/core/modelregistry_service.go (3)
9-10: Good practice: Compile-time interface assertion.The compile-time assertion ensures that ModelRegistryService properly implements the ModelRegistryApi interface, catching any interface compliance issues at compile time.
12-29: Well-structured service with comprehensive repository coverage.The service struct cleanly organizes all necessary repository dependencies for the expanded model registry functionality, including the new experiment-related repositories.
31-65: Constructor follows proper dependency injection pattern.The constructor properly initializes all repository dependencies and creates the EmbedMDMapper with the types map. While the parameter list is extensive, it's appropriate given the comprehensive nature of the service.
clients/python/src/mr_openapi/models/__init__.py (5)
21-21: New artifact type query parameter import added.The ArtifactTypeQueryParam import supports the new artifact filtering capabilities introduced in the API.
29-31: DataSet model imports added.The DataSet model and its create/update variants are properly imported to support the new dataset artifact type functionality.
37-47: Comprehensive experiment model imports.All experiment and experiment run related models are properly imported, including state/status enums and CRUD variants, supporting the full experiment lifecycle management functionality.
60-62: Metric model imports added.The Metric model and its create/update variants support the new metric artifact type functionality.
73-76: Parameter model imports with type support.The Parameter model imports include the ParameterType enum, properly supporting typed parameter artifacts.
api/openapi/catalog.yaml (4)
140-148: Well-defined artifact type enumeration.The ArtifactTypeQueryParam schema properly defines all supported artifact types with consistent kebab-case naming. This enables type-safe artifact filtering in the API.
577-613: Excellent documentation for advanced filtering.The filterQuery parameter documentation is comprehensive and well-structured, covering operators, data types, property access patterns, and practical examples. This will greatly help API users understand the rich filtering capabilities.
643-654: Proper artifact type filtering parameter.The artifactType parameter correctly references the ArtifactTypeQueryParam schema and includes helpful examples for API consumers.
655-666: Simple and clear step IDs parameter.The stepIds parameter provides a straightforward way to filter metrics by specific step IDs with clear documentation and examples.
clients/python/src/.openapi-generator/FILES (1)
13-68: Auto-generated file additions look comprehensive and well-structured.The new model files correctly correspond to the experiment tracking features being added, including experiment entities, experiment runs, datasets, metrics, parameters, and artifact type query parameters. The file structure follows consistent naming conventions.
internal/datastore/embedmd/mysql/connect_test.go (4)
18-24: Good addition of shared test utilities.The import of
testutilspackage and additional testcontainer modules supports the improved test infrastructure approach.
27-41: Excellent use of shared MySQL utilities for basic connection tests.Replacing manual container setup with
testutils.GetSharedMySQLDBandtestutils.GetSharedMySQLDSNimproves maintainability and resource efficiency. The cleanup function properly handles database connection closure.
135-150: Well-designed approach for SSL testing with dedicated container.Creating a dedicated SSL-enabled MySQL container for secure connection tests is the right approach since SSL tests require specific configuration that differs from the shared container setup. The container lifecycle management with proper cleanup is correctly implemented.
284-339: Good cleanup of error case tests.Removing redundant retry configurations from error case tests makes them more focused on testing the actual error conditions rather than retry behavior.
clients/python/src/mr_openapi/__init__.py (1)
38-93: New model imports are well-organized and complete.The additions properly expose all new experiment-related model classes following the existing import pattern and alphabetical ordering. The imports cover the full scope of experiment tracking functionality including:
- Artifact type query parameters for filtering
- DataSet models (create, update variants)
- Experiment models (create, list, update, state variants)
- ExperimentRun models (create, list, update, state, status variants)
- Metric models (create, update variants)
- Parameter models (create, update, type variants)
clients/python/src/mr_openapi/models/experiment.py (4)
1-26: Well-structured Pydantic model class for Experiment entity.The imports, class definition, and docstring follow OpenAPI Generator conventions properly. The model represents an experiment with appropriate field types and relationships.
28-65: Comprehensive field definitions with proper Pydantic configuration.The field definitions are well-structured with:
- Appropriate type hints using strict types
- Proper JSON field aliases (camelCase conversion)
- Clear descriptions matching OpenAPI specifications
- Correct handling of optional and required fields
67-86: Standard and correct Pydantic model configuration and basic serialization methods.The model configuration enables proper alias handling and validation. The
to_str()andto_json()methods follow the expected OpenAPI Generator pattern.
118-144: Excellent implementation of custom deserialization logic.The
from_dict()method properly handles:
- Null safety checks
- MetadataValue object reconstruction for custom_properties
- All field mapping with correct alias handling
This ensures proper round-trip serialization/deserialization for complex nested objects.
catalog/internal/catalog/rhec_catalog_test.go (3)
1-35: Well-structured test file setup with comprehensive imports and mock data.The test file follows Go testing best practices with:
- Appropriate imports including
cmpfor detailed comparisons- Realistic mock data setup with proper timestamp handling
- Clean separation of test data from test logic
36-73: Excellent table-driven test implementation for GetModel.The test cases comprehensively cover:
- Successful retrieval of existing models
- Proper handling of non-existent models (returns nil without error)
- Detailed result comparison using
cmp.Diff- Clear test case naming and structure
75-168: Comprehensive GetArtifacts test coverage with proper pagination handling.The test implementation effectively covers multiple scenarios:
- Models with artifacts (proper artifact data and metadata)
- Models without artifacts (empty list with correct pagination info)
- Non-existent models (nil result)
- Correct pagination metadata (PageSize and Size fields)
The test data includes realistic artifact information and proper timestamp handling.
internal/core/experiment.go (2)
16-56: Well-implemented upsert logic with proper error handling.The method correctly handles both create and update scenarios, with appropriate nil checks, duplicate key handling, and consistent error wrapping.
77-104: Clean implementation with proper parameter validation.Good use of
apiutils.ZeroIfNilfor safely displaying parameter values in error messages, and appropriate handling of both empty and multiple result cases.internal/apiutils/api_utils.go (2)
20-22: Clean integration of FilterQuery support.The FilterQuery field is consistently handled across both functions following the established patterns for optional fields.
Also applies to: 65-69, 91-91
99-117: Well-structured filter translation function.Good separation of concerns by reusing
BuildListOptionand adding translation as a separate step. The error handling provides clear context for debugging.clients/python/src/mr_openapi/models/metric_create.py (1)
25-68: Well-structured model following established patterns.The model correctly implements all required fields with appropriate types and follows the standard Pydantic patterns used throughout the codebase.
api/openapi/src/lib/common.yaml (3)
3-11: Well-defined enum schemas for query parameters.The new schemas provide clear constraints for artifact types and ordering fields, improving API usability and validation.
Also applies to: 226-233
315-351: Excellent documentation for the filterQuery parameter.The comprehensive documentation with examples, supported operators, and property access patterns will greatly help API users construct effective queries. The SQL-like syntax is intuitive and the escaping mechanism for special characters is well-explained.
369-381: Consistent parameter definitions with good examples.All new query parameters follow the established patterns with clear descriptions and helpful examples.
Also applies to: 393-404, 405-416
clients/python/src/mr_openapi/models/experiment_run_list.py (1)
24-101: Standard list model implementation following established patterns.The model correctly implements pagination fields and properly handles serialization of the nested ExperimentRun items.
internal/core/core_test.go (5)
16-18: LGTM: Standard TestMain pattern.The TestMain function properly delegates to testutils.TestMainHelper for consistent test lifecycle management.
20-34: Robust test database setup with proper cleanup.The setupTestDB function properly handles both setup and cleanup of test data, ensuring test isolation by cleaning up before and after each test. The combined cleanup function pattern ensures resources are properly released.
36-64: Comprehensive type ID mapping with proper error handling.The getTypeIDs function systematically retrieves all required type IDs from the database. The inclusion of new artifact types (DataSet, Metric, Parameter, etc.) aligns with the experiment tracking feature expansion.
66-105: Well-structured service creation with all repositories.The createModelRegistryService function properly instantiates all required repositories with correct type IDs and creates a fully configured ModelRegistryService. The repository initialization follows a consistent pattern and includes all the new experiment-related repositories.
107-116: Clean public API for test setup.The SetupModelRegistryService function provides a clean interface for tests, returning both the service and cleanup function. This pattern promotes proper resource management in tests.
clients/python/src/mr_openapi/models/experiment_create.py (5)
1-24: Standard OpenAPI generated model header.The file header correctly identifies this as generated OpenAPI code with appropriate warnings against manual editing.
25-44: Well-defined experiment model with proper validation.The ExperimentCreate model properly defines all required fields with appropriate types and validation. The
namefield is correctly marked as required while others are optional, which aligns with typical experiment creation workflows.
52-64: Standard serialization methods.The to_str, to_json, and from_json methods follow the established pattern for OpenAPI generated models.
66-90: Proper custom properties handling in to_dict.The to_dict method correctly handles the special case of custom_properties by recursively calling to_dict() on MetadataValue objects, ensuring proper nested serialization.
92-114: Correct nested deserialization in from_dict.The from_dict method properly handles the deserialization of custom_properties by converting dictionary values back to MetadataValue objects using MetadataValue.from_dict().
clients/python/src/mr_openapi/models/metric_update.py (3)
25-57: Well-structured metric update model with appropriate field types.The MetricUpdate model properly defines metric-specific fields with correct types:
valueaccepts both float and int for numeric metricstimestampuses string for Unix millisecond timestampsstepis properly typed as int for training step trackingartifact_typedefaults to "metric" as expected
79-103: Consistent custom properties handling.The to_dict method follows the same pattern as other models for handling custom_properties serialization.
114-129: Proper default value handling in from_dict.The from_dict method correctly handles the artifact_type default value by explicitly setting it to "metric" when not provided, ensuring consistency with the field definition.
clients/python/src/mr_openapi/models/experiment_update.py (3)
25-47: Appropriate field definitions for update operations.The ExperimentUpdate model correctly makes all fields optional, which is proper for update operations where clients should be able to update only specific fields without providing all data.
63-87: Consistent serialization implementation.The to_dict method maintains the same custom_properties handling pattern as other models in the codebase.
89-110: Standard deserialization pattern.The from_dict method follows the established pattern for handling optional fields and nested custom_properties deserialization.
clients/python/src/mr_openapi/models/parameter_create.py (3)
26-56: Well-designed parameter model with proper type constraints.The ParameterCreate model appropriately defines parameter-specific fields:
parameter_typeuses the ParameterType enum for validationvalueis typed as string for flexibility in parameter valuesartifact_typedefaults to "parameter" as expectednamefield is optional, allowing for unnamed parameters if needed
78-102: Consistent custom properties serialization.The to_dict method maintains the established pattern for handling nested MetadataValue serialization in custom_properties.
113-128: Proper default handling and deserialization.The from_dict method correctly handles the artifact_type default value and maintains consistency with the field definition by setting "parameter" when not provided.
internal/converter/openapi_reconciler.go (1)
39-85: LGTM! Consistent interface extension following established patterns.The new reconciler methods for DataSet, Metric, Parameter, Experiment, and ExperimentRun follow the exact same pattern as existing methods with appropriate goverter annotations. The ignored fields are correctly identified as immutable (Id, timestamps, Name) and entity-specific constraints (ArtifactType for artifacts, ExperimentId for runs).
internal/datastore/embedmd/mysql/migrate_test.go (4)
8-8: Good refactoring to use centralized test utilities.
40-40: LGTM! Simplified test lifecycle management.Using
testutils.TestMainHelper(m)centralizes MySQL container setup and teardown, improving maintainability and consistency across test files.
44-45: Excellent use of shared database utilities.The refactoring replaces manual database setup with
testutils.GetSharedMySQLDB(t), which provides both the database connection and a proper cleanup function. This approach is more reliable and consistent.
75-76: Consistent pattern application.Same beneficial pattern applied to the second test function, ensuring consistent resource management.
clients/python/src/mr_openapi/models/experiment_run_create.py (2)
26-85: Well-structured Pydantic model with proper field definitions.The ExperimentRunCreate model is correctly implemented with:
- Appropriate field types and aliases for API compatibility
- Required
experiment_idfield properly defined- Consistent field descriptions and validation
- Standard Pydantic configuration for alias handling
93-145: Correct custom properties handling in serialization methods.The dictionary conversion methods properly handle the nested
MetadataValueobjects incustom_properties, ensuring proper serialization/deserialization while maintaining type safety.clients/python/src/mr_openapi/models/data_set_create.py (2)
25-61: Well-designed dataset artifact model with appropriate defaults.The DataSetCreate model properly defines dataset-specific fields with:
- Correct default
artifact_typeof "dataset-artifact"- Proper aliasing for
var_schema→"schema"to avoid Python keyword conflicts- Dataset-specific fields like
digest,source_type,profilewith clear descriptions- Consistent field typing and validation
132-151: Correct deserialization with proper default handling.The
from_dictmethod correctly applies the default artifact_type when not provided (line 142), ensuring consistency with the field definition.internal/core/partial_update_test.go (3)
14-33: Excellent comprehensive test structure for partial update validation.The test suite properly covers all major entity types with partial update scenarios, ensuring consistent behavior across the API.
87-115: Important test case for custom properties replacement behavior.This test correctly validates and documents that
CustomPropertiesare completely replaced rather than merged when provided in an update. The comments clearly explain this expected behavior, which is important for API consumers to understand.
268-311: Critical test for nil vs empty string semantics.This test validates the important distinction between nil pointers (ignored) and empty strings (update to empty), which is crucial for proper partial update behavior. The test cases clearly demonstrate both scenarios.
internal/converter/mlmd_openapi_converter.go (1)
60-122: Well-structured interface extensions.The new conversion methods follow the established patterns consistently:
- Proper use of goverter annotations for property mappings
- Clear separation between artifact conversions (DataSet, Metric, Parameter) and context conversions (Experiment, ExperimentRun)
- Comprehensive property mappings including timestamps, states, and domain-specific fields
clients/python/src/mr_openapi/models/experiment_list.py (1)
24-99: LGTM! Standard OpenAPI-generated model implementation.The
ExperimentListmodel follows the established patterns for paginated list responses with proper:
- Field definitions with appropriate types and aliases
- Pydantic configuration for validation
- Comprehensive serialization/deserialization methods
- Proper handling of nested Experiment objects
clients/python/src/mr_openapi/README.md (1)
78-130: Documentation accurately reflects the new experiment APIs.The README updates comprehensively document all new experiment-related endpoints and their corresponding operations. The additions maintain consistency with the existing documentation format.
internal/converter/embdemd_openapi_converter.go (1)
104-162: Excellent interface consistency with comprehensive property mappings.The new conversion methods maintain perfect symmetry with the MLMD converter interface and follow established patterns:
- Consistent use of mapping functions for properties and attributes
- Proper handling of timestamps, states, and domain-specific fields
- Clear separation between context types (Experiment, ExperimentRun) and artifact types (DataSet, Metric, Parameter)
clients/python/src/mr_openapi/models/artifact_update.py (1)
25-258: Well-implemented polymorphic artifact update support.The extension to support new artifact types (DataSet, Metric, Parameter) is implemented correctly:
- Proper oneOf schema validation ensuring exactly one type matches
- Discriminator-based deserialization with appropriate fallbacks
- Consistent error handling and validation messages
- Type annotations properly updated to include all supported types
The implementation maintains backward compatibility while adding the new functionality.
clients/python/src/mr_openapi/models/parameter.py (1)
26-150: LGTM! Well-structured Pydantic model implementation.The Parameter model is well-implemented with proper field definitions, aliasing, validation, and custom serialization handling for nested MetadataValue objects. The read-only field exclusion and custom properties handling are correctly implemented.
clients/python/src/mr_openapi/models/parameter_update.py (1)
26-123: LGTM! Appropriate update model design.The ParameterUpdate model correctly includes only the fields that should be updatable (excluding id, name, and timestamps). The implementation follows the same patterns as other update models in the codebase.
internal/core/experiment_test.go (1)
15-342: Excellent comprehensive test coverage!This test suite provides thorough coverage of experiment operations including:
- CRUD operations with proper validation
- Error handling for invalid inputs
- Pagination and ordering functionality
- Critical non-editable field protection tests
- Partial update behavior validation
The test structure follows Go best practices with proper setup/teardown and clear test organization.
internal/converter/openapi_mlmd_converter.go (2)
55-113: LGTM! Consistent converter interface extensions.The new converter methods follow established patterns with proper goverter annotations, field mappings, and ignore statements. The addition of Uri field ignoring for Metric and Parameter converters is appropriate for these artifact types.
109-109: Name mapping for ExperimentRun is intentionally customThe ExperimentRun converter uses a bespoke MapExperimentRunName to apply the necessary prefix/UUID logic. All other resources—including RegisteredModel—leverage the
goverter:autoMap Modeldirective, which by default maps theNamefield fromsource.Model.NameintoprotoContext.Name. Verified in:
- internal/converter/openapi_mlmd_converter.go:
// goverter:autoMap Modelfor RegisteredModel- internal/converter/openapi_mlmd_converter_util.go & generated code: default mapping of
NameNo changes required.
clients/python/src/mr_openapi/models/experiment_run_update.py (1)
26-127: LGTM! Proper experiment run update model design.The ExperimentRunUpdate model appropriately includes experiment run-specific updatable fields with proper validation and serialization handling. The implementation follows established patterns and includes proper field aliasing.
clients/python/src/mr_openapi/models/data_set.py (1)
107-109: Verify null handling consistency.The
from_jsonmethod returnsSelf | Nonebut the implementation callsfrom_dictwhich also returnsSelf | None. Ensure this is consistent with other artifact models in the system.internal/core/experiment_run_test.go (4)
14-126: Excellent test coverage for experiment run upsert operations.The test comprehensively covers creation, updates, and various error scenarios. The use of subtests makes the test structure clear and maintainable.
128-176: Well-structured tests for experiment run retrieval by ID.Good coverage of both success and error cases, with proper error message validation.
178-235: Comprehensive parameter-based retrieval testing.The tests properly validate different parameter combinations and error conditions, ensuring the API contracts are enforced.
237-332: Thorough testing of experiment run listing functionality.Excellent coverage of filtering, pagination, and ordering scenarios. The ordering validation logic correctly handles descending sort order.
clients/python/src/mr_openapi/models/metric.py (2)
54-54: Good use of union types for numeric value field.The use of
StrictFloat | StrictIntappropriately handles both integer and floating-point metric values while maintaining type safety.
25-26: Well-defined metric artifact model.The class provides a comprehensive representation of metric artifacts with appropriate fields for ML training and evaluation metrics.
internal/converter/openapi_embedmd_converter.go (1)
59-87: Well-structured addition of new converter methods.The five new conversion methods (ConvertExperiment, ConvertExperimentRun, ConvertDataSet, ConvertMetric, ConvertParameter) follow the established patterns and include proper goverter annotations for Properties, Attributes, and TypeID mapping.
clients/python/src/mr_openapi/models/data_set_update.py (3)
25-26: Clear and appropriate model for dataset updates.The DataSetUpdate model appropriately excludes read-only fields while including all updatable dataset attributes.
102-103: Correct exclusion handling for update model.The empty
excluded_fieldsset is appropriate since this update model doesn't include read-only fields like timestamps that would need to be excluded during serialization.
58-70: DataSetUpdate model fields are consistent with updatable DataSet fields
All immutable/read-only fields (name,id,createTimeSinceEpoch,lastUpdateTimeSinceEpoch) have been excluded, and the update model includes exactly the remaining updatable properties:
- customProperties
- description
- externalId
- artifactType
- digest
- sourceType
- source
- schema
- profile
- uri
- state
No changes required.
clients/python/src/mr_openapi/models/artifact_create.py (7)
25-29: LGTM! Imports for new artifact types added correctly.The new imports for
DataSetCreate,MetricCreate, andParameterCreateare properly organized and follow the existing import pattern.
31-37: LGTM! OneOf schemas list properly updated.The
ARTIFACTCREATE_ONE_OF_SCHEMASlist correctly includes all five artifact types in alphabetical order.
47-62: LGTM! Class attributes updated consistently.The new validator fields,
actual_instancetype annotation, andone_of_schemasset are properly updated to include all five artifact types.
98-124: LGTM! Validation logic correctly extended.The
actual_instance_must_validate_oneofvalidator properly validates all five artifact types and the error messages correctly list all supported types.
144-193: LGTM! Discriminator handling properly implemented.The
from_jsonmethod correctly handles the new artifact types using their discriminator values:
"dataset-artifact"for DataSetCreate"metric"for MetricCreate"parameter"for ParameterCreateThe fallback checks using class names are also properly implemented.
206-237: LGTM! Deserialization fallback and error handling updated correctly.The deserialization attempts for all five types are properly implemented with appropriate error collection. The error messages correctly list all supported oneOf schemas.
248-258: LGTM! Return type annotation properly updated.The
to_dictmethod's return type correctly includes all five artifact types in the union.internal/core/metric_history_test.go (5)
1-13: LGTM! Well-structured test file with proper imports.The package declaration and imports are correctly organized for a comprehensive test suite.
14-86: LGTM! Comprehensive basic retrieval test.The test properly validates metric history retrieval with good coverage of all metric fields. The use of boolean flags to verify both metrics are found is a good practice.
139-208: Excellent error handling test coverage!The
TestInsertMetricHistoryfunction thoroughly tests:
- Successful insertion with custom properties
- Nil metric error
- Empty experiment run ID error
- Non-existent experiment run ID error
All error messages are properly validated.
389-441: Critical test for API compliance - well implemented!The
TestGetExperimentRunMetricHistoryReturnsCorrectArtifactTypetest is crucial for ensuring that the internal "metric-history" type is properly converted to "metric" for REST API compliance. This prevents breaking changes for API consumers.
443-551: LGTM! Comprehensive step ID filtering test.The test thoroughly validates:
- Single step ID filtering
- Multiple step IDs (comma-separated)
- Non-existent step ID
- Combination of name and step ID filters
Good coverage of the filtering functionality.
internal/apiutils/query_translator_test.go (4)
1-6: LGTM! Clean package declaration and imports.
7-160: Excellent comprehensive test coverage for TranslateFilterQuery!The table-driven test approach is well-implemented with:
- Clear test case names
- Good coverage of edge cases including escaped dots, special characters, and complex queries
- Proper error case testing
- Debug output for specific test cases (lines 141-147)
327-351: Minor issue in test expected values.The test cases have comments indicating "corrected" values, suggesting the expected values were updated. Please verify these are the correct expected values:
- Line 335:
"model\\.version.string_value", 14- Comment says "escaped dot, then real dot (corrected)"- Line 339:
"model\\\\\\..version", 9- Comment says "escaped backslash followed by dot (corrected)"- Line 340:
"model\\\\.version", 7- Comment says "double escaped backslash, then dot (corrected)"
375-414: LGTM! Comprehensive property name quoting test.The test thoroughly covers:
- Valid characters that don't require quoting (alphanumeric and underscore)
- All special characters that require backtick quoting
- Edge cases like empty string
Good test coverage!
internal/converter/openapi_embedmd_converter_util_test.go (8)
454-500: LGTM! Test properly updated for wrapper usage.The
TestMapModelVersionAttributesEmbedMDtest correctly usesOpenAPIModelWrapperand validates that the name is prefixed with the parent resource ID.
736-782: LGTM! InferenceService test updated consistently.The test properly validates name prefixing with parent resource ID.
925-1000: LGTM! ModelArtifact tests cover both normal and nil state cases.Good test coverage including:
- Name prefixing with parent resource ID
- Proper state handling
- Nil state preservation
1073-1148: LGTM! DocArtifact tests properly updated.The tests correctly validate name prefixing and nil state handling.
1236-1306: LGTM! ServeModel tests comprehensively updated.Good coverage of both normal and nil last known state scenarios.
1308-1354: LGTM! New ExperimentRun attribute mapping test.The test properly validates:
- Name prefixing with parent resource ID
- Timestamp conversion from string to int64
- External ID mapping
1356-1428: LGTM! Comprehensive Metric attribute mapping test.Excellent test coverage including:
- Name prefixing
- URI field correctly set to nil (metrics don't have URI)
- State handling for both non-nil and nil cases
1430-1502: LGTM! Parameter attribute mapping test mirrors Metric test structure.Good consistency with the Metric test, including:
- Name prefixing
- URI field correctly set to nil
- State handling for both cases
clients/python/src/mr_openapi/models/artifact.py (9)
25-29: LGTM!The new imports for DataSet, Metric, and Parameter are correctly added and follow the existing import pattern.
31-31: LGTM!The ARTIFACT_ONE_OF_SCHEMAS list is correctly updated to include all artifact types in alphabetical order.
41-48: LGTM!The validator attributes and type annotations are correctly added following the existing pattern. The actual_instance union type and one_of_schemas set properly include all artifact types.
84-98: LGTM!The validation logic for the new artifact types follows the established pattern and correctly validates each type.
102-103: LGTM!The error messages are correctly updated to include all artifact types.
Also applies to: 108-109
130-179: LGTM!The discriminator checks correctly handle both the artifact type values (e.g., "dataset-artifact", "metric", "parameter") and class names as fallbacks, following the existing pattern.
192-209: LGTM!The deserialization fallback logic for the new artifact types is correctly implemented following the existing pattern.
214-215: LGTM!The error messages in the deserialization section are correctly updated to include all artifact types.
Also applies to: 220-221
234-234: LGTM!The to_dict return type annotation correctly includes all artifact types in the union.
internal/converter/mlmd_openapi_converter_util.go (6)
183-190: LGTM!The artifact type mappings are correctly implemented. It's appropriate that both
MetricTypeNameandMetricHistoryTypeNamemap to "metric" for API consistency.
372-379: LGTM!The experiment state mapping follows the established pattern for state properties.
383-393: LGTM!The function correctly extracts the experiment ID from the owned format, following the same pattern as
MapRegisteredModelIdFromOwned.
395-411: LGTM!The experiment run state and status mapping functions follow the established pattern for property extraction.
413-445: LGTM!The time and dataset property mapping functions correctly extract string properties following the established pattern.
449-489: LGTM!The metric and parameter property mapping functions correctly handle their respective value types (float64 for metric values, int64 for steps, and appropriate type conversions for parameters).
internal/converter/openapi_converter.go (6)
31-33: LGTM!The artifact create and update mappings correctly include the new artifact types following the established pattern.
Also applies to: 38-40
55-71: LGTM!The converter methods for DataSet, Metric, and Parameter follow the established pattern with appropriate field ignores for create and update operations.
91-101: LGTM!The Experiment and ExperimentRun converter methods follow the established pattern. Note that ExperimentRun correctly includes
StartTimeSinceEpochin the ignore list for updates, preventing modification of the run start time.
119-119: LGTM!The artifact override method correctly includes all artifact types in the ignore list.
133-149: LGTM!The override methods for the new artifact types correctly identify editable fields. Each method appropriately excludes system-generated and immutable fields.
169-179: LGTM!The override methods for Experiment and ExperimentRun correctly identify editable fields. Note that ExperimentRun includes
EndTimeSinceEpochas editable but notStartTimeSinceEpoch, which is appropriate for tracking run duration.internal/converter/openapi_mlmd_converter_util.go (8)
23-27: Improve consistency by using ParseInt instead of Atoi.Good change! Using
strconv.ParseIntwith explicit 64-bit size is more appropriate thanstrconv.Atoiwhen the return type is*int64, ensuring consistent behavior across different platforms.
352-353: Verify the name handling change is intentional.The change from generating UUIDs to returning
nilfor missing artifact names is consistent across all artifact types. This aligns with the comment "ignore name for updates" but represents a significant behavioral change.Please confirm this change is intentional and that the system properly handles
nilnames in update scenarios.Also applies to: 459-460, 478-479, 544-545, 598-599
464-530: LGTM!The DataSet mapping functions correctly handle all properties following the established pattern.
534-584: LGTM!The Metric mapping functions correctly validate that the required
Valuefield is present and handle all properties appropriately.
588-633: LGTM!The Parameter mapping functions correctly validate that both required fields (
ValueandParameterType) are present and handle the type conversion appropriately.
807-841: LGTM!The Experiment mapping functions correctly handle all properties following the established pattern.
845-916: Note the different name handling for ExperimentRun.The ExperimentRun mapping functions are correct. However, note that
MapExperimentRunNamestill generates a UUID for missing names (line 858), unlike the artifact name mappers that returnnil. This inconsistency appears intentional based on different requirements.
918-920: LGTM!The
GenerateNewNamehelper function provides a clean way to generate UUID-based names.internal/core/experiment_run.go (8)
20-80: LGTM!The
UpsertExperimentRunmethod is well-implemented with proper validation, error handling, and database conflict detection. The experiment existence check ensures referential integrity.
82-99: LGTM!The
GetExperimentRunByIdmethod correctly handles ID conversion and not-found errors.
101-145: LGTM!The
GetExperimentRunByParamsmethod correctly handles the owned entity pattern and provides clear error messages for both not-found and multiple-match scenarios.
147-188: LGTM!The
GetExperimentRunsmethod correctly implements pagination and filtering by experiment ID.
190-205: LGTM!The
UpsertExperimentRunArtifactmethod correctly delegates to the generic artifact handler and appropriately creates metric history only for metric artifacts.
207-211: LGTM!The method correctly delegates to the generic
GetArtifactsmethod. The comment acknowledges that artifact type filtering is not implemented in the EmbedMD backend.
293-373: LGTM!The
InsertMetricHistorymethod is well-implemented with proper validation, unique name generation to prevent duplicates, and appropriate fallback to current time when the update timestamp is not available. The use of converters for property mapping ensures consistency.
213-291: Remove duplicate experiment run validation.The method has duplicate validation for experiment run existence on lines 220-224. The validation on lines 216-219 should be sufficient.
- // Validate experiment run exists - _, err := b.GetExperimentRunById(*experimentRunId) - if err != nil { - return nil, fmt.Errorf("experiment run not found: %w", err) - }The timestamp suffix removal logic correctly extracts the original metric name from the history record format.
Likely an incorrect or invalid review comment.
internal/core/non_editable_fields_test.go (7)
15-46: Well-structured test suite!The test organization with separate subtests for each entity type provides good isolation and makes debugging easier.
48-96: Comprehensive coverage of RegisteredModel field protection!The test properly verifies both non-editable field protection and editable field updates.
98-146: Excellent test coverage including parent relationship protection!The test properly verifies that the RegisteredModelId parent relationship cannot be modified, which is crucial for data integrity.
148-184: LGTM!Consistent with the test pattern and provides good coverage for ServingEnvironment.
186-245: Thorough test with proper dependency setup!Good coverage of the InferenceService entity. Note that ModelVersionId is correctly tested as editable while ServingEnvironmentId and RegisteredModelId are non-editable.
247-306: Complete dependency chain setup with proper field protection validation!The test correctly verifies that ModelVersionId is non-editable for ServeModel, contrasting with InferenceService where it's editable.
308-352: Good coverage for the new ExperimentRun entity!The test properly validates non-editable field protection for this newly introduced entity type.
catalog/internal/catalog/genqlient/generated.go (1)
1-390: Skip review of auto-generated file.This file is automatically generated by genqlient. Any necessary changes should be made to the GraphQL schema, queries, or genqlient configuration rather than this generated code.
internal/core/artifact.go (7)
16-33: Good refactoring to support generic parent resources!The change from
modelVersionIdtoparentResourceIdenables artifacts to be associated with different parent types, aligning with the new experiment and experiment run features.
35-73: ModelArtifact handling correctly updated!The implementation properly uses the new
parentResourceIdparameter while maintaining backward compatibility.
74-112: DocArtifact implementation follows the established pattern!Consistent with ModelArtifact handling and properly updated for generic parent resources.
153-210: Metric artifact correctly implements MLMD-style duplicate handling!The implementation properly handles the case where metrics with the same name in the same parent context are updated rather than creating duplicates. This aligns with the comment about MLMD behavior.
271-271: Error message correctly updated for all artifact types!
333-427: Excellent refactoring for generic artifact handling!The methods properly support:
- Generic parent resource associations
- Artifact type filtering
- Unified artifact mapping with
MapToArtifact
463-536: Model artifact methods correctly updated!The methods properly use the new generic parent resource approach while maintaining type safety by filtering for model artifacts only.
api/openapi/model-registry.yaml (1)
1635-1642:ArtifactTypeQueryParamenum added – check that the back-end rejects unknown valuesSpec lists 5 values, but
/artifactsartifactTypeparam now references it.
If the Go service still accepts the previous open-ended string, calls with an invalid value may silently succeed with empty results rather than 400.Verify server-side enforcement or add a
default: model-artifact/nullable: trueas appropriate.Also applies to: 3042-3052
internal/converter/embdemd_openapi_converter_util.go (5)
608-645: LGTM! Experiment mapping functions follow established patterns.The new Experiment mapping functions are well-structured and consistent with existing entity mappings in the codebase. They properly handle nil checks and error cases.
647-734: LGTM! ExperimentRun mapping functions handle required fields correctly.The ExperimentRun mapping functions are well-implemented. The error handling for
experiment_idappropriately enforces it as a required field, which aligns with the domain model requirements.
736-818: LGTM! DataSet mapping functions are well-structured.The DataSet mapping functions properly handle all the specific properties (digest, source_type, source, schema, profile) and follow the established artifact mapping patterns.
820-881: LGTM! Metric mapping functions handle numeric conversions correctly.The Metric mapping functions are well-implemented. The int32 to int64 conversion for the step property is appropriate for the EmbedMD to OpenAPI direction.
883-940: LGTM! Parameter mapping functions properly handle enum validation.The Parameter mapping functions correctly implement the parameter_type enum validation and follow consistent patterns with other artifact types.
internal/converter/openapi_embedmd_converter_util.go (5)
10-10: Good design pattern with OpenAPIModelWrapper!The introduction of
OpenAPIModelWrapper[T]is an elegant solution for passing parent resource context without modifying the core OpenAPI types. This enables proper hierarchical naming while maintaining type safety through generics.Also applies to: 96-99, 269-271, 320-344
320-344: Well-executed refactoring to support wrapper pattern.The refactoring of existing attribute mapping functions to use the wrapper pattern is clean and maintains all necessary nil checks. The delegation to dedicated name mapping functions improves code organization and reusability.
Also applies to: 468-491, 583-618, 639-674, 710-742
744-907: LGTM! Experiment and ExperimentRun mappings are comprehensive.The new mapping functions for Experiment and ExperimentRun entities follow established patterns and properly handle all required fields. The validation for required
experiment_idin ExperimentRun is particularly well-implemented.
909-1168: LGTM! Artifact mappings handle type-specific requirements well.The new artifact mapping functions for DataSet, Metric, and Parameter are well-implemented:
- DataSet properly includes URI field
- Metric and Parameter correctly set URI to nil with explanatory comments
- The int64 to int32 conversion for Metric step field is properly handled
- All state conversions use the established pattern
1170-1236: Excellent implementation of hierarchical name generation!The name mapping functions provide a robust solution for handling entity names in a hierarchical system:
- The generic
mapEntityNameWithUUIDGenerationhelper elegantly handles both user-provided names and UUID generation- The distinction between entities that auto-generate names (most artifacts) and those that don't (ModelVersion) is well-implemented
- All functions consistently apply parent resource ID prefixing for proper hierarchical organization
internal/datastore/embedmd/service.go (1)
93-123: Good addition of debug logging and validation!The debug logging at V(2) level and comprehensive type validation with a helpful error message improve the service's reliability and debuggability.
clients/python/src/mr_openapi/models/experiment_state.py (1)
1-32: LGTM!The auto-generated
ExperimentStateenum correctly defines the lifecycle states for experiments with proper JSON deserialization support.clients/python/src/mr_openapi/models/experiment_run_state.py (1)
1-32: LGTM!The auto-generated
ExperimentRunStateenum correctly defines the lifecycle states for experiment runs, consistent with theExperimentStateimplementation.clients/python/src/mr_openapi/models/artifact_type_query_param.py (1)
1-35: LGTM!The auto-generated
ArtifactTypeQueryParamenum correctly defines all supported artifact types for API querying with proper JSON deserialization support.internal/core/artifact_test.go (4)
16-18: Good test isolation improvement!Using
SetupModelRegistryServicewith proper cleanup provides better test isolation and consistency across the test suite.
1693-2186: Excellent comprehensive test coverage for artifact type filtering!The tests thoroughly cover:
- All 5 artifact types (ModelArtifact, DocArtifact, DataSet, Metric, Parameter)
- Filtering at different scopes (general, model version, experiment run)
- Edge cases (invalid types, empty results)
- Important validation that metric history records are excluded from artifact listings
2188-2326: Good coverage of duplicate handling for metrics and parameters!The tests properly verify that:
- Artifacts with the same name are updated rather than creating duplicates
- Artifact IDs remain consistent after updates
- Only one artifact exists per unique name within an experiment run
This ensures proper upsert semantics for experiment tracking.
1846-1846: Empty-string artifactType yields unfiltered resultsThe existing tests in internal/core/artifact_test.go (e.g. the “no filter returns all types” case) explicitly use
""and assert that all artifact types are returned. WhenartifactType == "", the client omits the query parameter, which correctly fetches all artifacts. No changes required.catalog/pkg/openapi/model_artifact_type_query_param.go (8)
1-16: LGTM! Clean package structure and appropriate imports.The package declaration and imports are well-structured for an OpenAPI-generated enum type.
18-28: LGTM! Well-defined enum type with comprehensive artifact types.The string-based enum design and constant naming follow Go conventions. The artifact types cover the expected use cases for model registry querying.
30-37: LGTM! Enum values are consistent with the constants.The allowed values slice correctly mirrors the enum constants and will enable proper validation.
39-54: LGTM! Robust JSON unmarshaling with proper validation.The method correctly validates input against allowed enum values and provides clear error messages for invalid inputs.
56-65: LGTM! Well-designed constructor with comprehensive error reporting.The function properly validates input and provides helpful error messages that include the list of allowed values.
67-80: LGTM! Standard enum utility methods implemented correctly.The
IsValid()method provides consistent validation logic, and thePtr()method follows Go conventions for pointer utilities.
82-108: LGTM! Standard nullable wrapper implementation.The nullable wrapper type correctly implements the typical pattern with proper state management for value and set status.
109-116: LGTM! Correct JSON marshaling for nullable wrapper.The marshaling methods properly handle the nullable wrapper pattern, with
UnmarshalJSONcorrectly setting theisSetflag.
| /api/model_registry/v1alpha3/experiment: | ||
| summary: Path used to search for an experiment. | ||
| description: >- | ||
| The REST endpoint/path used to search for an `Experiment` entity. This path contains a `GET` operation to perform the find task. | ||
| get: | ||
| tags: | ||
| - ModelRegistryService | ||
| responses: | ||
| "200": | ||
| $ref: "#/components/responses/ExperimentResponse" | ||
| "401": | ||
| $ref: "#/components/responses/Unauthorized" | ||
| "404": | ||
| $ref: "#/components/responses/NotFound" | ||
| "500": | ||
| $ref: "#/components/responses/InternalServerError" | ||
| "503": | ||
| $ref: "#/components/responses/ServiceUnavailable" | ||
| operationId: findExperiment | ||
| summary: Get an Experiment that matches search parameters. | ||
| description: Gets the details of a single instance of an `Experiment` that matches search parameters. | ||
| parameters: | ||
| - $ref: "#/components/parameters/name" | ||
| - $ref: "#/components/parameters/externalId" | ||
| /api/model_registry/v1alpha3/experiment_run: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Singular “search” endpoints deviate from REST conventions and duplicate /experiments functionality
/api/model_registry/v1alpha3/experiment (and the analogous experiment_run endpoint) introduce a second way to “find” a resource that overlaps almost completely with GET /experiments?name=…&externalId=….
Maintaining both styles:
- Bloats the surface area of the API.
- Violates the collection-vs-item convention (singular path should identify a specific ID, not a query).
- Doubles server logic & doc maintenance.
Strongly consider removing the singular “search” paths and use the collection path plus query parameters instead.
If legacy clients rely on the old path, mark it as deprecated: true and plan removal.
🤖 Prompt for AI Agents
In api/openapi/model-registry.yaml around lines 152 to 176, the singular search
endpoints like /api/model_registry/v1alpha3/experiment deviate from REST
conventions by duplicating the functionality of the collection endpoint
/experiments with query parameters. To fix this, remove the singular search
paths to avoid API bloat and duplicated logic, or if legacy clients depend on
them, mark these singular endpoints as deprecated and plan for their removal,
consolidating search functionality under the collection endpoint with query
parameters.
| summary: Path used to get metric history for an experiment run. | ||
| description: >- | ||
| The REST endpoint/path used to get metric history for an `ExperimentRun`. This path contains a `GET` operation to retrieve metrics with optional filtering by name and step IDs. | ||
| get: | ||
| tags: | ||
| - ModelRegistryService | ||
| parameters: | ||
| - $ref: "#/components/parameters/filterQuery" | ||
| - $ref: "#/components/parameters/name" | ||
| - $ref: "#/components/parameters/stepIds" | ||
| - $ref: "#/components/parameters/pageSize" | ||
| - $ref: "#/components/parameters/orderBy" | ||
| - $ref: "#/components/parameters/sortOrder" | ||
| - $ref: "#/components/parameters/nextPageToken" | ||
| responses: | ||
| "200": | ||
| $ref: "#/components/responses/ArtifactListResponse" | ||
| "401": | ||
| $ref: "#/components/responses/Unauthorized" | ||
| "404": | ||
| $ref: "#/components/responses/NotFound" | ||
| "500": | ||
| $ref: "#/components/responses/InternalServerError" | ||
| "503": | ||
| $ref: "#/components/responses/ServiceUnavailable" | ||
| operationId: getExperimentRunMetricHistory | ||
| summary: Get metric history for an ExperimentRun | ||
| description: Gets the metric history for an `ExperimentRun` with optional filtering by metric name and step IDs. | ||
| parameters: |
There was a problem hiding this comment.
/experiment_runs/{id}/metric_history returns a generic Artifact list – likely wrong schema
Metric history is expected to be a time-series of Metric artifacts only, yet the response is
ArtifactListResponse, whose items array allows any artifact subtype.
In client code this will deserialize to mixed types or require extra filtering.
Either:
- $ref: "#/components/responses/ArtifactListResponse"
+ $ref: "#/components/responses/MetricListResponse"and add a MetricList/MetricListResponse, or keep ArtifactListResponse but document that the server guarantees all items[].artifactType == "metric".
Failing to do so is a contract bug.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| summary: Path used to get metric history for an experiment run. | |
| description: >- | |
| The REST endpoint/path used to get metric history for an `ExperimentRun`. This path contains a `GET` operation to retrieve metrics with optional filtering by name and step IDs. | |
| get: | |
| tags: | |
| - ModelRegistryService | |
| parameters: | |
| - $ref: "#/components/parameters/filterQuery" | |
| - $ref: "#/components/parameters/name" | |
| - $ref: "#/components/parameters/stepIds" | |
| - $ref: "#/components/parameters/pageSize" | |
| - $ref: "#/components/parameters/orderBy" | |
| - $ref: "#/components/parameters/sortOrder" | |
| - $ref: "#/components/parameters/nextPageToken" | |
| responses: | |
| "200": | |
| $ref: "#/components/responses/ArtifactListResponse" | |
| "401": | |
| $ref: "#/components/responses/Unauthorized" | |
| "404": | |
| $ref: "#/components/responses/NotFound" | |
| "500": | |
| $ref: "#/components/responses/InternalServerError" | |
| "503": | |
| $ref: "#/components/responses/ServiceUnavailable" | |
| operationId: getExperimentRunMetricHistory | |
| summary: Get metric history for an ExperimentRun | |
| description: Gets the metric history for an `ExperimentRun` with optional filtering by metric name and step IDs. | |
| parameters: | |
| get: | |
| tags: | |
| - ModelRegistryService | |
| parameters: | |
| - $ref: "#/components/parameters/filterQuery" | |
| - $ref: "#/components/parameters/name" | |
| - $ref: "#/components/parameters/stepIds" | |
| - $ref: "#/components/parameters/pageSize" | |
| - $ref: "#/components/parameters/orderBy" | |
| - $ref: "#/components/parameters/sortOrder" | |
| - $ref: "#/components/parameters/nextPageToken" | |
| responses: | |
| "200": | |
| $ref: "#/components/responses/MetricListResponse" | |
| "401": | |
| $ref: "#/components/responses/Unauthorized" | |
| "404": | |
| $ref: "#/components/responses/NotFound" | |
| "500": | |
| $ref: "#/components/responses/InternalServerError" | |
| "503": | |
| $ref: "#/components/responses/ServiceUnavailable" | |
| operationId: getExperimentRunMetricHistory | |
| summary: Get metric history for an ExperimentRun | |
| description: Gets the metric history for an `ExperimentRun` with optional filtering by metric name and step IDs. |
🤖 Prompt for AI Agents
In api/openapi/model-registry.yaml between lines 377 and 405, the response
schema for the GET /experiment_runs/{id}/metric_history endpoint currently uses
ArtifactListResponse, which allows any artifact subtype. This is incorrect
because the endpoint should return only Metric artifacts. To fix this, define a
new MetricListResponse schema that restricts items to Metric artifacts and
update the 200 response to reference this new schema. Alternatively, if keeping
ArtifactListResponse, explicitly document that all items have artifactType
"metric" to clarify the contract.
| description: The name/key of the metric (e.g., "accuracy", "loss", "f1_score"). | ||
| type: string | ||
| MetricUpdate: | ||
| description: A metric to be updated. | ||
| allOf: | ||
| - $ref: "#/components/schemas/BaseResourceUpdate" | ||
| - type: object | ||
| properties: | ||
| artifactType: | ||
| type: string | ||
| default: "metric" | ||
| value: | ||
| description: The numeric value of the metric. | ||
| type: number | ||
| format: double | ||
| timestamp: | ||
| description: Unix timestamp in milliseconds when the metric was recorded. | ||
| type: string | ||
| format: int64 | ||
| step: | ||
| description: The step number for multi-step metrics (e.g., training epochs). | ||
| type: integer | ||
| format: int64 | ||
| state: |
There was a problem hiding this comment.
MetricUpdate.timestamp & step defined as string/int64 mix – type confusion
timestamp:
type: string
format: int64The canonical pattern for milliseconds-since-epoch is either
type: integer
format: int64or string without format.
Same for step which is integer but also format: int64 – redundant.
Incorrect typing breaks generated clients (e.g., openapi-generator maps string/int64 to std::string).
Please align with integer/int64.
🤖 Prompt for AI Agents
In api/openapi/model-registry.yaml between lines 2267 and 2290, the MetricUpdate
schema defines the timestamp property as type string with format int64, which
causes type confusion and breaks client generation. Change the timestamp
property type from string to integer while keeping the format as int64. Also,
remove the redundant format int64 from the step property or keep it consistent
as integer with format int64. This alignment ensures proper typing and
compatibility with generated clients.
| examples: | ||
| filterQuery: | ||
| value: "name='my-model' AND state='LIVE'" | ||
| name: filterQuery | ||
| description: | | ||
| A SQL-like query string to filter the list of entities. The query supports rich filtering capabilities with automatic type inference. | ||
|
|
||
| **Supported Operators:** | ||
| - Comparison: `=`, `!=`, `<>`, `>`, `<`, `>=`, `<=` | ||
| - Pattern matching: `LIKE`, `ILIKE` (case-insensitive) | ||
| - Set membership: `IN` | ||
| - Logical: `AND`, `OR` | ||
| - Grouping: `()` for complex expressions | ||
|
|
||
| **Data Types:** | ||
| - Strings: `"value"` or `'value'` | ||
| - Numbers: `42`, `3.14`, `1e-5` | ||
| - Booleans: `true`, `false` (case-insensitive) | ||
|
|
||
| **Property Access:** | ||
| - Standard properties: `name`, `id`, `state`, `createTimeSinceEpoch` | ||
| - Custom properties: Any user-defined property name | ||
| - Escaped properties: Use backticks for special characters: `` `custom-property` `` | ||
| - Type-specific access: `property.string_value`, `property.double_value`, `property.int_value`, `property.bool_value` | ||
|
|
||
| **Examples:** | ||
| - Basic: `name = "my-model"` | ||
| - Comparison: `accuracy > 0.95` | ||
| - Pattern: `name LIKE "%tensorflow%"` | ||
| - Complex: `(name = "model-a" OR name = "model-b") AND state = "LIVE"` | ||
| - Custom property: `framework.string_value = "pytorch"` | ||
| - Escaped property: `` `mlflow.source.type` = "notebook" `` | ||
| schema: | ||
| type: string | ||
| in: query |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
filterQuery parameter opens an injection surface – document constraints & validate server-side
A free-form SQL-like string invites:
• SQL/ORM-based injection
• DoS via expensive predicates
• Ambiguous grammar differences
Mitigations to spell out:
- Exact grammar (e.g., ANTLR spec) & max length.
- Escaping rules for identifiers / literals.
- List of disallowed constructs (
;, unions, sub-selects, etc.). - Server-side parameterisation rather than string concatenation.
Please add a brief note to the description and ensure back-end validation before this hits production.
🏁 Script executed:
#!/bin/bash
# Search for occurrences of "filterQuery" across the repository to check for any existing validation or handling
rg -n "filterQuery" -A3 .Length of output: 109633
Document filterQuery grammar & enforce server‐side validation
The OpenAPI description for filterQuery should call out the exact syntax and any limits, and note that invalid filters are rejected at the server layer. Specifically:
• In api/openapi/model-registry.yaml (around lines 2964–2972), update the description to include:
– Reference to the supported grammar (see internal/db/filter package)
– Maximum string length (e.g. 512 chars)
– Escaping rules for identifiers and literals
– Disallowed constructs (;, UNION, sub-selects, etc.)
– A note that the server uses filter.Parse() (internal/db/service/generic_repository.go:115–120) to validate and will return HTTP 400 on parse errors
• Ensure any parse errors from apiutils.BuildListOption bubble up as 400 responses (already handled in internal/server/openapi/api_model_registry_service_service.go)
These changes will make the injection surface explicit and steer clients toward safe, validated queries.
🤖 Prompt for AI Agents
In api/openapi/model-registry.yaml around lines 2964 to 2972, enhance the
filterQuery description to explicitly document the supported grammar referencing
the internal/db/filter package, specify a maximum string length limit (e.g., 512
characters), detail escaping rules for identifiers and literals, and list
disallowed constructs such as semicolons, UNION, and sub-selects. Add a note
that the server validates filterQuery using filter.Parse()
(internal/db/service/generic_repository.go lines 115–120) and returns HTTP 400
on parse errors. Confirm that parse errors from apiutils.BuildListOption
propagate as 400 responses in
internal/server/openapi/api_model_registry_service_service.go.
| - $ref: "#/components/parameters/filterQuery" | ||
| - $ref: "#/components/parameters/artifactType" | ||
| - $ref: "#/components/parameters/pageSize" |
There was a problem hiding this comment.
🚨 filterQuery, artifactType, stepIds parameters are referenced but not defined
components/parameters is completely empty. All newly-added endpoints now $ref these query parameters, which renders the OpenAPI document invalid and will break server/client code-gen and tooling.
Define them once under components.parameters and make sure the $refs resolve.
components:
parameters:
+ filterQuery:
+ name: filterQuery
+ in: query
+ description: SQL-like WHERE clause supporting <, <=, =, >=, >, AND, OR.
+ required: false
+ schema:
+ type: string
+
+ artifactType:
+ name: artifactType
+ in: query
+ description: Return only artifacts of the given discriminator value
+ required: false
+ schema:
+ type: string
+ enum: [model-artifact, doc-artifact, dataset-artifact, metric, parameter]
+
+ stepIds:
+ name: stepIds
+ in: query
+ description: Comma-separated list of step IDs when querying metric history.
+ required: false
+ schema:
+ type: array
+ items:
+ type: integer
+ format: int64Also applies to: 187-188, 324-330, 443-447, 579-583, 686-690, 776-780, 1074-1080
🤖 Prompt for AI Agents
In api/openapi/src/model-registry.yaml around lines 48 to 50 and the other
specified ranges, the parameters filterQuery, artifactType, and stepIds are
referenced but not defined under components.parameters, causing invalid OpenAPI
references. Define these parameters explicitly once under components.parameters
with their appropriate schema and descriptions so that all $ref references
resolve correctly and the OpenAPI document remains valid for code generation and
tooling.
| func (b *ModelRegistryService) GetExperimentById(id string) (*openapi.Experiment, error) { | ||
| convertedId, err := strconv.ParseInt(id, 10, 32) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%v: %w", err, api.ErrBadRequest) | ||
| } | ||
|
|
||
| experiment, err := b.experimentRepository.GetByID(int32(convertedId)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("no experiment found for id %s: %w", id, api.ErrNotFound) | ||
| } | ||
|
|
||
| toReturn, err := b.mapper.MapToExperiment(experiment) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%v: %w", err, api.ErrBadRequest) | ||
| } | ||
|
|
||
| return toReturn, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling for repository operations.
The current implementation always returns a "not found" error regardless of the actual error from the repository. Consider checking if the error is actually a "not found" error before wrapping it.
experiment, err := b.experimentRepository.GetByID(int32(convertedId))
if err != nil {
- return nil, fmt.Errorf("no experiment found for id %s: %w", id, api.ErrNotFound)
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ return nil, fmt.Errorf("no experiment found for id %s: %w", id, api.ErrNotFound)
+ }
+ return nil, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (b *ModelRegistryService) GetExperimentById(id string) (*openapi.Experiment, error) { | |
| convertedId, err := strconv.ParseInt(id, 10, 32) | |
| if err != nil { | |
| return nil, fmt.Errorf("%v: %w", err, api.ErrBadRequest) | |
| } | |
| experiment, err := b.experimentRepository.GetByID(int32(convertedId)) | |
| if err != nil { | |
| return nil, fmt.Errorf("no experiment found for id %s: %w", id, api.ErrNotFound) | |
| } | |
| toReturn, err := b.mapper.MapToExperiment(experiment) | |
| if err != nil { | |
| return nil, fmt.Errorf("%v: %w", err, api.ErrBadRequest) | |
| } | |
| return toReturn, nil | |
| } | |
| func (b *ModelRegistryService) GetExperimentById(id string) (*openapi.Experiment, error) { | |
| convertedId, err := strconv.ParseInt(id, 10, 32) | |
| if err != nil { | |
| return nil, fmt.Errorf("%v: %w", err, api.ErrBadRequest) | |
| } | |
| experiment, err := b.experimentRepository.GetByID(int32(convertedId)) | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| return nil, fmt.Errorf("no experiment found for id %s: %w", id, api.ErrNotFound) | |
| } | |
| return nil, err | |
| } | |
| toReturn, err := b.mapper.MapToExperiment(experiment) | |
| if err != nil { | |
| return nil, fmt.Errorf("%v: %w", err, api.ErrBadRequest) | |
| } | |
| return toReturn, nil | |
| } |
🤖 Prompt for AI Agents
In internal/core/experiment.go around lines 58 to 75, the error handling for the
repository GetByID call always returns a "not found" error regardless of the
actual error. Modify the code to check if the error returned by
experimentRepository.GetByID is specifically a "not found" error and only then
wrap it with api.ErrNotFound; for other errors, return them as is or wrap with a
more appropriate error. This ensures accurate error reporting based on the
actual repository error.
| func testArtifactNonEditableFields(t *testing.T, service *core.ModelRegistryService) { | ||
| // Test ModelArtifact, DocArtifact, DataSet, Metric, Parameter | ||
| // Non-editable for all: Id, CreateTimeSinceEpoch, LastUpdateTimeSinceEpoch, Name, ArtifactType | ||
| // Editable: Description, ExternalId, CustomProperties, Uri, State, etc. | ||
|
|
||
| t.Run("ModelArtifact non-editable fields", func(t *testing.T) { | ||
| // Create model artifact | ||
| artifact := &openapi.Artifact{ | ||
| ModelArtifact: &openapi.ModelArtifact{ | ||
| Name: apiutils.Of("test-ma"), | ||
| Description: apiutils.Of("Original description"), | ||
| Uri: apiutils.Of("s3://original/path"), | ||
| }, | ||
| } | ||
|
|
||
| created, err := service.UpsertArtifact(artifact) | ||
| require.NoError(t, err) | ||
|
|
||
| // Store originals | ||
| originalId := *created.ModelArtifact.Id | ||
| originalName := *created.ModelArtifact.Name | ||
| originalArtifactType := *created.ModelArtifact.ArtifactType | ||
|
|
||
| time.Sleep(10 * time.Millisecond) | ||
|
|
||
| // Attempt to hack non-editable fields | ||
| updateRequest := &openapi.Artifact{ | ||
| ModelArtifact: &openapi.ModelArtifact{ | ||
| Id: created.ModelArtifact.Id, | ||
| Name: apiutils.Of("HACKED_NAME"), // Should be ignored | ||
| ArtifactType: apiutils.Of("HACKED_TYPE"), // Should be ignored | ||
| Description: apiutils.Of("Updated description"), // Should work | ||
| Uri: apiutils.Of("s3://updated/path"), // Should work | ||
| }, | ||
| } | ||
|
|
||
| updated, err := service.UpsertArtifact(updateRequest) | ||
| require.NoError(t, err) | ||
|
|
||
| // Verify protection | ||
| assert.Equal(t, originalId, *updated.ModelArtifact.Id, "ModelArtifact ID should not be changeable") | ||
| assert.Equal(t, originalName, *updated.ModelArtifact.Name, "ModelArtifact Name should not be changeable") | ||
| assert.Equal(t, originalArtifactType, *updated.ModelArtifact.ArtifactType, "ModelArtifact ArtifactType should not be changeable") | ||
| assert.Equal(t, "Updated description", *updated.ModelArtifact.Description, "ModelArtifact Description should be updatable") | ||
| assert.Equal(t, "s3://updated/path", *updated.ModelArtifact.Uri, "ModelArtifact Uri should be updatable") | ||
| }) | ||
|
|
||
| // Similar tests can be added for DocArtifact, DataSet, Metric, Parameter | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for new artifact types introduced in this PR.
The comment on line 401 correctly notes that tests should be added for DocArtifact, DataSet, Metric, and Parameter. Since this PR introduces these new artifact types, it would be valuable to include their non-editable field protection tests for completeness.
Would you like me to generate the test implementations for the other artifact types?
🤖 Prompt for AI Agents
In internal/core/non_editable_fields_test.go around lines 354 to 402, the
existing test covers non-editable field protection only for ModelArtifact. To
fix this, add similar subtests for DocArtifact, DataSet, Metric, and Parameter
artifact types following the same pattern: create an artifact, store original
non-editable fields, attempt to update those fields with hacked values, call
UpsertArtifact, and assert that non-editable fields remain unchanged while
editable fields update correctly. This will ensure comprehensive coverage for
all new artifact types introduced in the PR.
| @@ -24,7 +24,7 @@ func (b *ModelRegistryService) UpsertServingEnvironment(servingEnvironment *open | |||
| return nil, err | |||
| } | |||
|
|
|||
| withNotEditable, err := b.mapper.OverrideNotEditableForServingEnvironment(converter.NewOpenapiUpdateWrapper(existing, servingEnvironment)) | |||
| withNotEditable, err := b.mapper.UpdateExistingServingEnvironment(converter.NewOpenapiUpdateWrapper(existing, servingEnvironment)) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the new method exists and maintains equivalent functionality.
The method name change from OverrideNotEditableForServingEnvironment to UpdateExistingServingEnvironment appears to be part of a broader refactoring effort. Please ensure the new method exists in the mapper and provides equivalent functionality.
🏁 Script executed:
#!/bin/bash
# Verify the UpdateExistingServingEnvironment method exists and check for any remaining references to the old method
echo "Searching for UpdateExistingServingEnvironment method definition..."
ast-grep --pattern 'func ($$$) UpdateExistingServingEnvironment($$$) ($$$) {
$$$
}'
echo -e "\nSearching for any remaining references to OverrideNotEditableForServingEnvironment..."
rg "OverrideNotEditableForServingEnvironment" -A 2 -B 2Length of output: 5295
Update remaining call sites to use UpdateExistingServingEnvironment
The new
func (c *OpenAPIReconcilerImpl) UpdateExistingServingEnvironment(source converter.OpenapiUpdateWrapper[openapi.ServingEnvironment]) (openapi.ServingEnvironment, error)
exists in internal/converter/generated/openapi_reconciler.gen.go, but there’s still one remaining call to the old method:
• pkg/core/serving_environment.go
Replace:
withNotEditable, err := serv.openapiConv.OverrideNotEditableForServingEnvironment(
converter.NewOpenapiUpdateWrapper(existing, servingEnvironment),
)With:
withNotEditable, err := serv.openapiConv.UpdateExistingServingEnvironment(
converter.NewOpenapiUpdateWrapper(existing, servingEnvironment),
)This will complete the refactoring and ensure all “override” calls now use the standardized update method.
🤖 Prompt for AI Agents
In internal/core/serving_environment.go at line 27, replace the call to
serv.openapiConv.OverrideNotEditableForServingEnvironment with
serv.openapiConv.UpdateExistingServingEnvironment, passing the same
converter.NewOpenapiUpdateWrapper(existing, servingEnvironment) argument. This
updates the call site to use the new standardized update method as required.
|
|
||
| glog.Infof("All required types validated successfully") | ||
|
|
||
| artifactRepository := service.NewArtifactRepository(connectedDB, typesMap[defaults.ModelArtifactTypeName], typesMap[defaults.DocArtifactTypeName], typesMap[defaults.DataSetTypeName], typesMap[defaults.MetricTypeName], typesMap[defaults.ParameterTypeName], typesMap[defaults.MetricHistoryTypeName]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the long parameter list.
The NewArtifactRepository constructor now takes 6 type ID parameters, making it error-prone and difficult to maintain. Consider using a struct to encapsulate these type IDs or a builder pattern.
Create a type ID configuration struct:
-artifactRepository := service.NewArtifactRepository(connectedDB, typesMap[defaults.ModelArtifactTypeName], typesMap[defaults.DocArtifactTypeName], typesMap[defaults.DataSetTypeName], typesMap[defaults.MetricTypeName], typesMap[defaults.ParameterTypeName], typesMap[defaults.MetricHistoryTypeName])
+typeConfig := service.ArtifactTypeConfig{
+ ModelArtifactTypeID: typesMap[defaults.ModelArtifactTypeName],
+ DocArtifactTypeID: typesMap[defaults.DocArtifactTypeName],
+ DataSetTypeID: typesMap[defaults.DataSetTypeName],
+ MetricTypeID: typesMap[defaults.MetricTypeName],
+ ParameterTypeID: typesMap[defaults.ParameterTypeName],
+ MetricHistoryTypeID: typesMap[defaults.MetricHistoryTypeName],
+}
+artifactRepository := service.NewArtifactRepository(connectedDB, typeConfig)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/datastore/embedmd/service.go at line 124, the NewArtifactRepository
call has a long list of six type ID parameters, which is error-prone and hard to
maintain. Refactor by creating a struct that holds all these type IDs as fields,
then pass an instance of this struct to NewArtifactRepository instead of
individual parameters. Update the NewArtifactRepository constructor to accept
this struct, improving readability and maintainability.
| @@ -174,7 +174,7 @@ endif | |||
|
|
|||
| .PHONY: vet | |||
| vet: | |||
| ${GO} vet ./... | |||
| ${GO} vet $$(${GO} list ./... | grep -v github.com/kubeflow/model-registry/internal/db/filter) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the reason for excluding the filter package from vet.
The exclusion of github.com/kubeflow/model-registry/internal/db/filter from the vet target should be documented or verified to ensure it's intentional and necessary.
🏁 Script executed:
#!/bin/bash
# Check if the filter package exists and examine its contents
fd -t f -e "*.go" | rg "internal/db/filter" -A 5 -B 5
# Check if there are any vet issues specific to this package
if [ -d "internal/db/filter" ]; then
echo "Filter package exists, checking for vet issues:"
go vet ./internal/db/filter/... 2>&1 || echo "Vet issues found in filter package"
else
echo "Filter package does not exist"
fiLength of output: 3384
Document vet exclusion for filter package
The internal/db/filter package uses participle grammar struct tags (e.g. @@, "OR" @@, etc.) which are not valid Go struct tags and cause go vet to fail. Please add a brief explanation so future maintainers understand why this directory is excluded.
• Makefile (line 177): add a comment above the vet rule
• internal/db/filter/parser.go: add a file‐level comment describing the known vet incompatibility
Suggested snippets:
--- a/Makefile
+++ b/Makefile
@@ -174,6 +174,9 @@ vet:
- ${GO} vet $$(${GO} list ./... | grep -v github.com/kubeflow/model-registry/internal/db/filter)
+ # Exclude filter package: uses participle grammar tags that break go vet
+ ${GO} vet $$(${GO} list ./... | grep -v github.com/kubeflow/model-registry/internal/db/filter)--- a/internal/db/filter/parser.go
+++ b/internal/db/filter/parser.go
@@
+// Package filter defines a DSL parser using participle.
+// It deliberately uses grammar struct tags (e.g. `@@`, `"AND" @@`),
+// which are not valid reflect.StructTag syntax and will trigger go vet errors.
+// This file is excluded from the vet target in the Makefile.🤖 Prompt for AI Agents
In the Makefile at line 177, add a comment above the vet command explaining that
the internal/db/filter package is excluded because it uses participle grammar
struct tags that are invalid Go struct tags and cause go vet to fail.
Additionally, in internal/db/filter/parser.go, add a file-level comment
describing this known incompatibility with go vet to inform future maintainers.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests
Refactor