RHOAIENG-3458: Introduce edge client and model registry client #234

devguyio · 2024-03-22T04:22:24Z

Description

Using the model registry to implement the edge use case includes a lot of custom logic.

This PR adds a model registry client and an edge client as two abstractions that encapsulate such logic and can be used by the CLI directly.

This PR also refactors the models command to use the newly introduced edge client and streamlines the command design.

Tip

This PR is reviewed best commit-by-commit. Instead of using the Files changed tab:

Use the Commits tab
Click on the top commit and review that commit
Use n and p to go to the next and previous commits respectively to review each commit.

How Has This Been Tested?

cd cli
make cli-build
./odh models

./odh models add "voice-recognition-model" "A ML model for voice recognition" "v1"

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

Running odh command without any subcommands should show the help message and not execute any functionality. The listing of namespaces was only added as a test. Make the root command not runnable and remove the unecessary test.

openshift-ci-robot · 2024-03-22T04:22:28Z

@devguyio: This pull request references RHOAIENG-3458 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Description

Using the model registry to implement the edge use case includes a lot of custom logic.

This PR adds a model registry client and an edge client as two abstractions that encapsulate such logic and can be used by the CLI directly.

This PR also refactors the models command to use the newly introduced edge client and streamlines the command design.

[!TIP]
This PR is reviewed best commit-by-commit. Instead of using the Files changed tab:

Use the Commits tab

Click on the top commit and review that commit

Use n and p to go to the next and previous commits respectively to review each commit.

How Has This Been Tested?
cd cli
make cli-build
./odh models
./odh models add "voice-recognition-model" "A ML model for voice recognition" "v1"   
Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.

Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).

The developer has manually tested the changes and verified that the changes work

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-03-22T04:22:32Z

@devguyio: This pull request references RHOAIENG-3458 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Description

Using the model registry to implement the edge use case includes a lot of custom logic.

This PR adds a model registry client and an edge client as two abstractions that encapsulate such logic and can be used by the CLI directly.

This PR also refactors the models command to use the newly introduced edge client and streamlines the command design.

[!TIP]
This PR is reviewed best commit-by-commit. Instead of using the Files changed tab:

Use the Commits tab

Click on the top commit and review that commit

Use n and p to go to the next and previous commits respectively to review each commit.

How Has This Been Tested?
cd cli
make cli-build
./odh models
./odh models add "voice-recognition-model" "A ML model for voice recognition" "v1"   
Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.

Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).

The developer has manually tested the changes and verified that the changes work

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2024-03-22T04:22:36Z

@devguyio: This pull request references RHOAIENG-3458 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Description

Using the model registry to implement the edge use case includes a lot of custom logic.

This PR adds a model registry client and an edge client as two abstractions that encapsulate such logic and can be used by the CLI directly.

This PR also refactors the models command to use the newly introduced edge client and streamlines the command design.

[!TIP]
This PR is reviewed best commit-by-commit. Instead of using the Files changed tab:

Use the Commits tab

Click on the top commit and review that commit

Use n and p to go to the next and previous commits respectively to review each commit.

How Has This Been Tested?
cd cli
make cli-build
./odh models
./odh models add "voice-recognition-model" "A ML model for voice recognition" "v1"   
Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.

Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).

The developer has manually tested the changes and verified that the changes work

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

devguyio · 2024-03-22T04:23:24Z

@biswassri FYI

cli/pkg/commands/common/types.go

cli/pkg/commands/common/cmd.go

cli/pkg/edgeclient/client.go

Sara4994 · 2024-03-24T16:16:49Z

Can we have an Readme containing instructions to build and run the cli, also list of available commands and its syntax? it will be easy for others to try it out. wdyt?

grdryn · 2024-03-25T14:47:20Z

/lint

openshift-ci

@grdryn: 54 warnings.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cli/pkg/pipelines/params.go

cli/pkg/commands/common/cmd.go

cli/pkg/edgeclient/client.go

cli/pkg/httptest/server.go

grdryn

Is the JIRA ID correct in the title? It appears to reference a closed issue RHOAIENG-3458

cli/pkg/modelregistry/client.go

cli/pkg/modelregistry/errors.go

grdryn · 2024-03-25T17:49:53Z

cli/pkg/modelregistry/client.go

+			return nil, fmt.Errorf("error creating model version: %w", err)
+		} else {
+			if resp.StatusCode != 201 {
+				// TODO: Remove this workaround when model registry returns 404 when the model is not found


Is there an issue that can be linked for this?

Do you mean linked in the source code?

Yeah, possibly. I think linking issues in the source code is useful, so that when we stumble across this TODO in future, we'll have concrete places to look to see if it can be progressed.

cli/pkg/modelregistry/client.go

cli/pkg/edgeclient/client.go

cli/params.yaml

cli/pkg/edgeclient/types.go

grdryn · 2024-03-25T19:23:28Z

This PR is reviewed best commit-by-commit. Instead of using the Files changed tab [...]

One drawback of this approach is that I don't seem to be able to leave inline suggestions as easily on all files; and also I don't seem to be able to mark all files as viewed, although I can for some (I presume if a file is changed in more than one commit in the PR, it can't be marked as viewed in any single one of those).

grdryn · 2024-03-27T19:10:38Z

/remove-approve

(new process)

MarianMacik

We should also include this in the Makefile and in the PR check as well under the make test target.

cli/params.yaml

cli/pkg/commands/models/add.go

cli/pkg/pipelines/params.go

cli/pkg/modelregistry/client.go

cli/pkg/edgeclient/types.go

cli/pkg/modelregistry/errors.go

devguyio · 2024-04-09T13:05:17Z

/lint

openshift-ci

@devguyio: 4 warnings.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cli/pkg/modelregistry/client.go

devguyio · 2024-04-09T13:11:52Z

/lint

openshift-ci

@devguyio: 0 warnings.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

devguyio · 2024-04-09T13:17:39Z

Can we have an Readme containing instructions to build and run the cli, also list of available commands and its syntax? it will be easy for others to try it out. wdyt?

@Sara4994 I added a readme.

Sara4994 · 2024-04-12T13:38:43Z

This PR looks good to me, leaving this to Gerard and Landon now :)

devguyio · 2024-04-12T13:40:15Z

Holding for other reviewers

/hold

devguyio · 2024-04-12T13:41:20Z

This PR looks good to me, leaving this to Gerard and Landon now :)

And @MarianMacik

grdryn

/lgtm

LaVLaS · 2024-04-15T13:04:36Z

/unhold

MarianMacik

I have just a few more comments. Nothing blocking, though. Thanks for all the changes, the code looks even better now!

MarianMacik · 2024-04-15T20:36:56Z

cli/examples/params.yaml.sample

+  - name: testDataConfigMapName
+    value: CONFIG_MAP_NAME
+    # Whether to add a workspace directory to the pipeline. Can be set to "true" or "false"
+  - name: addDirWorkspace
+    value: "true"


These are still present but as you said they are not used for this change, so maybe it would be better to remove these from this PR?

MarianMacik · 2024-04-15T20:51:12Z

cli/pkg/commands/common/cmd.go

+	for _, f := range flags {
+		if !f.IsRootFlag() {
+			if f.IsInherited() {
+				cmd.PersistentFlags().StringP(f.String(), f.Shorthand(), f.Value(), f.Usage())
+			} else {
+				cmd.Flags().StringP(f.String(), f.Shorthand(), f.Value(), f.Usage())
+			}
+		}
+	}


I'm not a cobra expert, but with the model add command we are inheriting the FlagModelRegistryURL param from the root command. Here we are defining it again as a persistent flag even though it is not needed. If it works, then I'm fine with it. However, maybe we could register "new" flags here only if it is not a root flag AND it is not inherited. I am guessing this would be the right way to do it.

Here we're registering non-root flags only. Root flags are registered in pkg/commands/root.go. The FlagsModelRegistryURL is a root flag as declared in pkg/commands/flags/flags.go.

One logical error that I just fixed, is that root flags should also be set as inherited. I pushed a fix. It wasn't producing a bug but it was logically incorrect.

A root flag means it's defined at the root level, so can be inherited by all subcommands.

There can be a case of an inherited flag that's not root. In that case, it's defined by a specific sub-command and will be inherited by all of its nested sub-commands.

@devguyio I see, I didn't realize that it is a root flag :)

However, in case of an inherited flag that is not root, if we add the third layer of commands, such a flag will be re-added at the third layer to the persistent flags, right? Because it will need to be listed among the flags to read and because it is the same slice, it will be iterated over when registering as well.
Right now it is not an issue as everything non-root should be registered. Also, maybe it won't be an issue even afterwards because cobra maybe can handle it.

docs/cli.md

cli/pkg/commands/models/models.go

MarianMacik · 2024-04-16T08:36:41Z

cli/pkg/commands/common/cmd.go

+	for _, f := range flags {
+		if !f.IsRootFlag() {
+			if f.IsInherited() {
+				cmd.PersistentFlags().StringP(f.String(), f.Shorthand(), f.Value(), f.Usage())
+			} else {
+				cmd.Flags().StringP(f.String(), f.Shorthand(), f.Value(), f.Usage())
+			}
+		}
+	}


@devguyio I see, I didn't realize that it is a root flag :)

However, in case of an inherited flag that is not root, if we add the third layer of commands, such a flag will be re-added at the third layer to the persistent flags, right? Because it will need to be listed among the flags to read and because it is the same slice, it will be iterated over when registering as well.
Right now it is not an issue as everything non-root should be registered. Also, maybe it won't be an issue even afterwards because cobra maybe can handle it.

MarianMacik · 2024-04-16T09:58:27Z

We should also include this in the Makefile and in the PR check as well under the make test target.

@MarianMacik can you please clarify what should be included in the Makefile?

@devguyio I meant to include cli tests under the root makefile test target.

devguyio · 2024-04-16T11:01:56Z

We should also include this in the Makefile and in the PR check as well under the make test target.

@MarianMacik can you please clarify what should be included in the Makefile?

@devguyio I meant to include cli tests under the root makefile test target.

@MarianMacik I've pushed the following changes:

Added the cli-test as part of the root test target
Changed the flags packaged by renaming the rootFlag to parentFlag and now it means that this is a flag that will be declared by a parent so we're only interested in getting its value, not in declaring it.
Fixed a bug that was preventing adding two version with two different names under the same model:
- pkg/modelregistry package
  - Added FindRegisteredModel & FindModelVersion
  - Updated AutoRegisterModelVersionArtifact method to check if the model or version exist first, and create them only if they don't exist.

Using the introduced edge client, refactor the model command to simply use the edge client for listing and adding models. Also refactor the commands design to streamline the flags and commands initialization.

MarianMacik

Nice catch with that bug, thanks for incorporating my comments!
/lgtm

devguyio · 2024-04-16T16:38:46Z

/override ci/prow/test-ai-edge

openshift-ci · 2024-04-16T16:38:51Z

@devguyio: Overrode contexts on behalf of devguyio: ci/prow/test-ai-edge

In response to this:

/override ci/prow/test-ai-edge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

grdryn

/approve

openshift-ci · 2024-04-17T09:05:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grdryn

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:

~~OWNERS~~ [grdryn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Make the root command not runnable

fe90eca

Running odh command without any subcommands should show the help message and not execute any functionality. The listing of namespaces was only added as a test. Make the root command not runnable and remove the unecessary test.

openshift-ci-robot added the jira/valid-reference label Mar 22, 2024

openshift-ci bot added the approved label Mar 22, 2024

devguyio requested review from LaVLaS, StevenTobin and Sara4994 March 22, 2024 04:23

devguyio mentioned this pull request Mar 22, 2024

RHOAIENG-3459: Implement images cmd #235

Merged

3 tasks

Sara4994 requested changes Mar 24, 2024

View reviewed changes

cli/pkg/commands/common/types.go Outdated Show resolved Hide resolved

cli/pkg/commands/common/cmd.go Show resolved Hide resolved

cli/pkg/edgeclient/client.go Show resolved Hide resolved

openshift-ci bot assigned Sara4994 Mar 24, 2024

devguyio mentioned this pull request Mar 25, 2024

RHOAIENG-3458: Add a models command to the CLI #227

Merged

3 tasks

openshift-ci bot reviewed Mar 25, 2024

View reviewed changes

grdryn reviewed Mar 25, 2024

View reviewed changes

cli/pkg/edgeclient/types.go Outdated Show resolved Hide resolved

openshift-ci bot removed the approved label Mar 27, 2024

MarianMacik reviewed Apr 4, 2024

View reviewed changes

devguyio force-pushed the rhoaieng-3458-models-cmd branch from 142d095 to 9738cec Compare April 9, 2024 13:04

openshift-ci bot reviewed Apr 9, 2024

View reviewed changes

cli/pkg/modelregistry/client.go Outdated Show resolved Hide resolved

cli/pkg/modelregistry/client.go Outdated Show resolved Hide resolved

cli/pkg/modelregistry/client.go Outdated Show resolved Hide resolved

cli/pkg/modelregistry/client.go Outdated Show resolved Hide resolved

devguyio force-pushed the rhoaieng-3458-models-cmd branch from 9738cec to e9ede5b Compare April 9, 2024 13:11

openshift-ci bot reviewed Apr 9, 2024

View reviewed changes

openshift-ci bot added the lgtm label Apr 12, 2024

Sara4994 approved these changes Apr 12, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Apr 12, 2024

grdryn approved these changes Apr 15, 2024

View reviewed changes

openshift-ci bot assigned grdryn Apr 15, 2024

openshift-ci bot removed the do-not-merge/hold label Apr 15, 2024

MarianMacik reviewed Apr 15, 2024

View reviewed changes

devguyio force-pushed the rhoaieng-3458-models-cmd branch from 38f8c04 to 66fb903 Compare April 16, 2024 00:08

openshift-ci bot removed the lgtm label Apr 16, 2024

devguyio requested a review from MarianMacik April 16, 2024 00:14

MarianMacik reviewed Apr 16, 2024

View reviewed changes

devguyio force-pushed the rhoaieng-3458-models-cmd branch 2 times, most recently from 25ddb80 to b3d2259 Compare April 16, 2024 10:56

Refactor the models command to use the edge client

0e2569d

Using the introduced edge client, refactor the model command to simply use the edge client for listing and adding models. Also refactor the commands design to streamline the flags and commands initialization.

devguyio force-pushed the rhoaieng-3458-models-cmd branch from b3d2259 to 0e2569d Compare April 16, 2024 11:56

MarianMacik approved these changes Apr 16, 2024

View reviewed changes

openshift-ci bot assigned MarianMacik Apr 16, 2024

openshift-ci bot added the lgtm label Apr 16, 2024

grdryn approved these changes Apr 17, 2024

View reviewed changes

openshift-ci bot added the approved label Apr 17, 2024

openshift-merge-bot bot merged commit bca628b into opendatahub-io:main Apr 17, 2024
2 checks passed

RHOAIENG-3458: Introduce edge client and model registry client #234

RHOAIENG-3458: Introduce edge client and model registry client #234

Conversation

devguyio commented Mar 22, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

openshift-ci-robot commented Mar 22, 2024 • edited by openshift-ci bot Loading

Description

How Has This Been Tested?

Merge criteria:

openshift-ci-robot commented Mar 22, 2024 • edited by openshift-ci bot Loading

Description

How Has This Been Tested?

Merge criteria:

openshift-ci-robot commented Mar 22, 2024 • edited by openshift-ci bot Loading

Description

How Has This Been Tested?

Merge criteria:

devguyio commented Mar 22, 2024

Sara4994 commented Mar 24, 2024 • edited Loading

grdryn commented Mar 25, 2024

openshift-ci bot left a comment

Choose a reason for hiding this comment

grdryn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grdryn commented Mar 25, 2024

grdryn commented Mar 27, 2024

MarianMacik left a comment

Choose a reason for hiding this comment

devguyio commented Apr 9, 2024

openshift-ci bot left a comment

Choose a reason for hiding this comment

devguyio commented Apr 9, 2024

openshift-ci bot left a comment

Choose a reason for hiding this comment

devguyio commented Apr 9, 2024 • edited Loading

Sara4994 commented Apr 12, 2024

devguyio commented Apr 12, 2024

devguyio commented Apr 12, 2024

grdryn left a comment

Choose a reason for hiding this comment

LaVLaS commented Apr 15, 2024

MarianMacik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devguyio Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarianMacik commented Apr 16, 2024

devguyio commented Apr 16, 2024

MarianMacik left a comment

Choose a reason for hiding this comment

devguyio commented Apr 16, 2024

openshift-ci bot commented Apr 16, 2024

grdryn left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 17, 2024

devguyio commented Mar 22, 2024 •

edited

Loading

openshift-ci-robot commented Mar 22, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 22, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 22, 2024 •

edited by openshift-ci bot

Loading

Sara4994 commented Mar 24, 2024 •

edited

Loading

devguyio commented Apr 9, 2024 •

edited

Loading

devguyio Apr 16, 2024 •

edited

Loading