Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: optimize model service apis #454

Merged
merged 1 commit into from
Dec 27, 2023
Merged

feat: optimize model service apis #454

merged 1 commit into from
Dec 27, 2023

Conversation

bjwswang
Copy link
Collaborator

What type of PR is this?

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

}
// sort by creation timestamp
llmList.Nodes = append(llmList.Nodes, embedderList.Nodes...)
sort.Slice(llmList.Nodes, func(i, j int) bool {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xff-dev We can use the CreationTimestamp which has been pased into ModelService for this sort.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xff-dev add a newNodeList for sort

@@ -40,6 +40,13 @@ var (
StatusFalse = "False"
)

// ModelType
var (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: change to const

"github.com/kubeagi/arcadia/pkg/embeddings"
"github.com/kubeagi/arcadia/pkg/utils"
)

// Embedder2model convert unstructured `CR Embedder` to graphql model `Embedder`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Embedder2model convert unstructured `CR Embedder` to graphql model `Embedder`
// Embedder2model convert unstructured CR `Embedder` to graphql model `Embedder`

nit

// if pageSize is -1 which means unlimited pagesize,return all
if pageSize == common.UnlimitedPageSize {
page = 1
pageSize = totalCount
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return &generated.PaginatedResult{
		TotalCount:  totalCount,
		HasNextPage: end < totalCount,
		Nodes:       result,
	}, nil

maybe in line 298, I guess this total is inaccurate, If keyword is not empty, the results have been filtered, but the total number has not changed. HasNextPage will always be true...

It should be a problem elsewhere.

"github.com/kubeagi/arcadia/pkg/llms"
"github.com/kubeagi/arcadia/pkg/utils"
)

// LLM2model convert unstructured `CR LLM` to graphql model `Llm`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// LLM2model convert unstructured `CR LLM` to graphql model `Llm`
// LLM2model convert unstructured CR `LLM` to graphql model `Llm`

nit

if llm.Spec.Provider.GetType() == v1alpha1.ProviderTypeWorker {
w, err := worker.ReadWorker(ctx, c, llm.Name, llm.Namespace)
if err == nil {
if w.Status != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apiserver/pkg/embedder/embedder.go may need also check w.Status != nil

@bjwswang bjwswang merged commit ed4e8d8 into kubeagi:main Dec 27, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants