-
Notifications
You must be signed in to change notification settings - Fork 6
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
[Merged by Bors] - feat: add kbfaq trace (nlu-902) #458
Conversation
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.
Just want to make sure we build a knowledge base type that encompasses every use-case.
|
||
export interface TraceFrame extends BaseTraceFrame<TraceFramePayload> { | ||
type: TraceType.KNOWLEDGE_BASE_FAQ; | ||
} |
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 would recommend creating a more generic knowledge-base
enum value, then breaking it down into sub-types. We have a completely different knowledge-base
trace which also needs to be eventually accounted for.
The trace types should be probably something more like this:
export enum TraceType {
KNOWLEDGE_BASE = 'knowledgeBase',
}
export enum KnowledgeBaseCtxType {
FAQ = 'faq',
DOCUMENTS = 'documents',
}
interface AbstractKnowledgeBasePayload extends BaseResponseTrace {
contextType: KnowledgeBaseCtxType;
}
export interface FAQPayload extends AbstractKnowledgeBasePayload {
contextType: KnowledgeBaseCtxType.FAQ;
faqQuestion: string;
faqAnswer: string;
query: string;
}
export interface DocumentsPayload extends AbstractKnowledgeBasePayload {
contextType: KnowledgeBaseCtxType.DOCUMENTS;
chunks: {
score: number;
documentID: string;
documentData: unknown;
}[];
query: {
messages: string[];
output: string;
}
}
export type KnowledgeBasePayload = FAQPayload | DocumentsPayload;
export interface TraceFrame extends BaseTraceFrame<KnowledgeBasePayload> {
type: TraceType.KNOWLEDGE_BASE;
}
From the perspective of a user consuming the DM API, having a more hierarchical type structure makes it simpler to write code that processes the traces, e.g, if I want only the knowledge base traces, I can just filter by knowlege-base-context
rather than less elegant approaches like matching by regex or manually checking every type knowledge-base-faq
, knowledge-base-documents
, knowldge-base-something-new-we-add-later
.
You don't need to actually write the DocumentsPayload
type for now. I don't have context on what the right type is myself. just include the KnowledgeBaseCtxType.DOCUMENTS
in your PR and write this as a placeholder:
export type KnowledgeBasePayload = FAQPayload;
I or someone on Platform or the KB team can write a more accurate DocumentsPayload
type later on.
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.
Edited my comment to change the enum name from knowledge_base_context
to knowledgeBase
to keep backwards compatibility with the existing implementation of general-runtime
.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. |
bors r+ |
Add trace type for KB FAQ.
Pull request successfully merged into master. Build succeeded: |
Add trace type for KB FAQ.