-
Notifications
You must be signed in to change notification settings - Fork 125
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(api): implement /chat and /ask apis in Node.js (#5) #23
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.
We could improve some stuff for better maintenance and extensibility but we can also merge for now.
import { MessageBuilder } from '../message-builder.js'; | ||
import { AskApproach } from './approach.js'; | ||
import { messagesToString } from '../message.js'; | ||
|
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.
For readability and maintenance, I would move this to a global variables file and outside.
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'm not sure that would be a good idea, as these prompt are specific to this particular approach. Other approaches have their own set of prompts, and mixing them altogether in a different file would make it more confusing IMO
const top = overrides?.top ? Number(overrides?.top) : 3; | ||
const excludeCategory: string | undefined = overrides?.exclude_category; | ||
const filter = excludeCategory ? `category ne '${excludeCategory.replace("'", "''")}'` : undefined; | ||
|
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.
Will we be using this when integrating a vector database? Otherwise maybe let's delete unnecessary code to make it easier to distro.
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 already use a vector DB, it's Cognitive Search. The code is commented because of the missing SDK features, otherwise it would be used. This is currently a big issue as without using the semantic search and vector search, the results are pretty crap :/
If the question is not in English, translate the question to English before generating the search query. | ||
If you cannot generate a search query, return just the number 0. | ||
`; | ||
|
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.
Again, I would move all this to a global file.
chatClient = new OpenAI({ | ||
...commonOptions, | ||
baseURL: `${openAiUrl}/openai/deployments/${config.azureOpenAiChatGptDeployment}`, | ||
}); |
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.
Are this API endpoints stable?
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, they're used in the AOAI SDK too.
This service will probably be replaced by the AOAI SDK anyways (#22)
Test it locally
.env
file from azd inpackages/api
npm install && npm start
at the root of the projectImplemented
/chat
API, but with limitations (see [sdk] missing options from JS document-search SDK #21)Missing (for later PRs)
/ask
API (and approaches)