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

devguyio · 2024-03-13T08:48:09Z

Description

Add a models command that lists registered models from the model registry.

This PR is stacked on top of #222 .

How Has This Been Tested?

Install models registry in your kubecluster following this video
Port-forward port 8080 of the modelregistry-sample service in the namespace where you installed the model registry

kubectl port-forward services/modelregistry-sample 8080

From the cli directory, run the cli using go, the URL of the model registry is set via the env var MODEL_REGISTRY_URL which defaults to http://localhost:8080 .

cd cli
go run cmd/main.go

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

openshift-ci-robot · 2024-03-13T08:48:13Z

@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

Add a models command that lists registered models from the model registry.

This PR is stacked on top of #222 .

How Has This Been Tested?

Install models registry in your kubecluster following this video

Port-forward port 8080 of the modelregistry-sample service in the namespace where you installed the model registry
kubectl port-forward services/modelregistry-sample 8080
From the cli directory, run the cli using go, the URL of the model registry is set via the env var MODEL_REGISTRY_URL which defaults to http://localhost:8080 .
cd cli
go run cmd/main.go
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-13T13:53:59Z

/assign @StevenTobin @LaVLaS

openshift-ci-robot · 2024-03-13T13:55:02Z

@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

Add a models command that lists registered models from the model registry.

This PR is stacked on top of #222 .

How Has This Been Tested?

Install models registry in your kubecluster following this video

Port-forward port 8080 of the modelregistry-sample service in the namespace where you installed the model registry
kubectl port-forward services/modelregistry-sample 8080
From the cli directory, run the cli using go, the URL of the model registry is set via the env var MODEL_REGISTRY_URL which defaults to http://localhost:8080 .
cd cli
go run cmd/main.go
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 · 2024-03-18T14:52:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devguyio, Sara4994

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~~ [devguyio]

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

MarianMacik

Looks good, I have just one question about the currently unused fields of the model.

MarianMacik · 2024-03-19T22:41:58Z

cli/pkg/commands/models.go

+	cursor               int
+	choices              int


@devguyio I noticed that these are not used. Are they a preparation for the next step when a use will be able to choose a model and operate on it?

grdryn

Feel free to disregard my Go-related comments, since I haven't used it in about 3 years probably, and have never used the bubbletea stuff 🙃

grdryn · 2024-03-25T01:20:27Z

cli/pkg/commands/models.go

@@ -0,0 +1,163 @@
+/*
+Copyright 2023 KStreamer Authors


Is this copyright assignment correct? Just checking in case it's from some pre-existing default in your editor.

grdryn · 2024-03-25T01:28:09Z

cli/pkg/commands/models.go

+		return errMsg{err}
+	}
+	if httpRes.StatusCode != 200 {
+		fmt.Printf("not 200: %d\n", httpRes.StatusCode)


Is this log message redundant, given the error message being returned? How does fmt.Printf get handled in the interactive text UI?

grdryn · 2024-03-25T01:40:03Z

cli/pkg/commands/models.go

+	return registeredModelsMsg(models)
+}
+
+type model struct {


I wonder if this would be better named more specifically to say that model here is a bubbletea concept, given that the file name (models.go) refers to AI models in a model registry? 😵‍💫

grdryn · 2024-03-25T01:43:11Z

cli/pkg/commands/models.go

+func (m model) View() string {
+	if m.err != nil {
+		return errorStyle.Render(fmt.Sprintf("Error: %s", m.err))
+		// return ""


This comment could probably be deleted

grdryn · 2024-03-25T01:54:03Z

cli/pkg/commands/models.go

+		{Title: "Description", Width: 60},
+	}
+
+	rows := make([]table.Row, 0)


I guess the length 0 here could be more optimally set to len(m.registeredModelsList.Items)? That might have an impact on the code structure though, since the nil check from line 111 would have to encompass this too

grdryn · 2024-03-25T01:57:15Z

cli/pkg/commands/models.go

+			os.Exit(1)
+		}
+		if m.(model).err != nil {
+			// cmd.PrintErrf("Error: %v\n", m.(model).err)


This commented code could probably be removed

devguyio · 2024-03-25T11:53:58Z

@grdryn thanks for the review. This PR was merged by mistake and a lot of the comments you mentioned were addressed in #234 . Basically, the models command was refactored in #234 to use the edge client and also be ready for merge.

So I suggest that you check that PR and consider it the proposed state of the models command? wdyt?

grdryn · 2024-03-25T12:08:01Z

Yeah, for sure, I plan to get around to looking at #234 soon too. I just figured that one would make more sense once I'd seen these earlier ones (that I know you'd mentioned had been merged accidentally).

devguyio added 2 commits March 5, 2024 21:22

Add an inital root CLI

bd1b702

Add listing registered models from model registry

8439dcf

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

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

devguyio requested review from Sara4994, LaVLaS and StevenTobin March 13, 2024 09:42

openshift-ci bot assigned LaVLaS and StevenTobin Mar 13, 2024

Sara4994 approved these changes Mar 18, 2024

View reviewed changes

openshift-ci bot assigned Sara4994 Mar 18, 2024

openshift-ci bot added the lgtm label Mar 18, 2024

openshift-merge-bot bot merged commit 9b74fea into opendatahub-io:main Mar 18, 2024
2 checks passed

MarianMacik reviewed Mar 19, 2024

View reviewed changes

grdryn reviewed Mar 25, 2024

View reviewed changes

devguyio mentioned this pull request Apr 9, 2024

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

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

devguyio commented Mar 13, 2024 •

edited

Loading

openshift-ci-robot commented Mar 13, 2024 •

edited by openshift-ci bot

Loading

Description

How Has This Been Tested?

Merge criteria:

devguyio commented Mar 13, 2024

openshift-ci-robot commented Mar 13, 2024 •

edited by openshift-ci bot

Loading

Description

How Has This Been Tested?

Merge criteria:

openshift-ci bot commented Mar 18, 2024

MarianMacik left a comment

MarianMacik Mar 19, 2024

grdryn left a comment

grdryn Mar 25, 2024

grdryn Mar 25, 2024

grdryn Mar 25, 2024

grdryn Mar 25, 2024

grdryn Mar 25, 2024

grdryn Mar 25, 2024

devguyio commented Mar 25, 2024

grdryn commented Mar 25, 2024

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

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

Conversation

devguyio commented Mar 13, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

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

Description

How Has This Been Tested?

Merge criteria:

devguyio commented Mar 13, 2024

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

Description

How Has This Been Tested?

Merge criteria:

openshift-ci bot commented Mar 18, 2024

MarianMacik left a comment

Choose a reason for hiding this comment

MarianMacik Mar 19, 2024

Choose a reason for hiding this comment

grdryn left a comment

Choose a reason for hiding this comment

grdryn Mar 25, 2024

Choose a reason for hiding this comment

grdryn Mar 25, 2024

Choose a reason for hiding this comment

grdryn Mar 25, 2024

Choose a reason for hiding this comment

grdryn Mar 25, 2024

Choose a reason for hiding this comment

grdryn Mar 25, 2024

Choose a reason for hiding this comment

grdryn Mar 25, 2024

Choose a reason for hiding this comment

devguyio commented Mar 25, 2024

grdryn commented Mar 25, 2024

devguyio commented Mar 13, 2024 •

edited

Loading

openshift-ci-robot commented Mar 13, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 13, 2024 •

edited by openshift-ci bot

Loading