-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add documentloader CRD and add audio handler #771
Conversation
nkwangleiGIT
commented
Feb 28, 2024
- Add documentloader CRD to handle uploaded files from user(read file from s3)
- Add audio handler using whisper, so we can transcribe audio to txt and then chat with audio content
if !ok || len(files) == 0 { | ||
return args, errors.New("empty file list") | ||
} | ||
if err := cli.Get(ctx, types.NamespacedName{Namespace: p.RefNamespace(), Name: p.Ref.Name}, instance); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should split resource get into func (p *Executor) Init
, like
arcadia/pkg/appruntime/knowledgebase/knowledgebase.go
Lines 41 to 48 in 352145b
func (k *Knowledgebase) Init(ctx context.Context, cli client.Client, _ map[string]any) error { | |
instance := &v1alpha1.KnowledgeBase{} | |
if err := cli.Get(ctx, types.NamespacedName{Namespace: k.RefNamespace(), Name: k.Ref.Name}, instance); err != nil { | |
return fmt.Errorf("can't find the knowledgebase in cluster: %w", err) | |
} | |
k.Instance = instance | |
return nil | |
} |
And add func (p *Executor) Ready()
like
arcadia/pkg/appruntime/knowledgebase/knowledgebase.go
Lines 54 to 56 in 352145b
func (k *Knowledgebase) Ready() (isReady bool, msg string) { | |
return k.Instance.Status.IsReadyOrGetReadyMessage() | |
} |
Since the state update has not been implemented yet, we can implement this along with the state update in one pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add it to make documentloader always ready, and check if we need to add some logic later.
for _, file := range files { | ||
ossInfo := &arcadiav1alpha1.OSS{Bucket: p.RefNamespace()} | ||
ossInfo.Object = filepath.Join("upload", file) | ||
klog.Infoln("handling file", ossInfo.Object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use klog.FromContext(ctx).xxx
to replace klog
, We add a requestid
to each request in the logs to make debugging easier, in
arcadia/apiserver/pkg/requestid/requestid.go
Lines 29 to 36 in 352145b
func addRequestIDToLog(c *gin.Context, id string) { | |
ctx := c.Request.Context() | |
logger := klog.FromContext(ctx) | |
newLogger := logger.WithValues("requestID", id) | |
newLogger.Info("new request") | |
c.Request = c.Request.WithContext(klog.NewContext(ctx, newLogger)) | |
c.Next() |
But it looks like this is an issue elsewhere as well, and we can leave that for a subsequent pr along with an update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, klog it more easy to use
) | ||
|
||
const ( | ||
Group = "arcadia.kubeagi.k8s.com.cn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to double check that since we are using group name arcadia.kubeagi.k8s.com.cn
, should we move these files to api/base
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think about this before, but documentloader is a Node that should run with app runtime, so place it to app-node.
api/base seems just resources that does not have any Node run logic, so don't use it although they have the same group. Let's keep it for now?