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

[Merged by Bors] - feat: add kbfaq trace (nlu-902) #458

Closed
wants to merge 3 commits into from

Conversation

e-vandenberg
Copy link
Contributor

Add trace type for KB FAQ.

Copy link
Contributor

@zhihil zhihil left a 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;
}
Copy link
Contributor

@zhihil zhihil Oct 10, 2023

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.

Screen Shot 2023-10-10 at 4 59 59 PM

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.

Copy link
Contributor

@zhihil zhihil Oct 10, 2023

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.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

warning 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.
Read more here

@e-vandenberg
Copy link
Contributor Author

bors r+

bors-vf bot pushed a commit that referenced this pull request Oct 10, 2023
Add trace type for KB FAQ.
@bors-vf
Copy link

bors-vf bot commented Oct 10, 2023

Pull request successfully merged into master.

Build succeeded:

@bors-vf bors-vf bot changed the title feat: add kbfaq trace (nlu-902) [Merged by Bors] - feat: add kbfaq trace (nlu-902) Oct 10, 2023
@bors-vf bors-vf bot closed this Oct 10, 2023
@bors-vf bors-vf bot deleted the erik/add-faq-trace/nlu-902 branch October 10, 2023 22:37
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.

2 participants