(EAI-587): Clean up shared imports in mongodb-rag-core + migrate `@azure/openai` to `openai` #546

mongodben · 2024-10-30T18:27:52Z

Jira: https://jira.mongodb.org/browse/EAI-587

Changes

Switch codebase to use openai module instead of @azure/openai
Rename modules exported from mongodb-rag-core to better folow namespace convention of CamelCase (w/ first letter uppercase)
- Due to small differences in the SDKs' APIs, there were some small associated code refactors.

Notes

mongodben · 2024-10-31T14:01:51Z

packages/mongodb-rag-core/src/index.ts

 export * from "mongodb";
-export * from "@azure/openai";
+export * as OpenAI from "openai";
 export * from "./langchain";


@nlarew in future PR, i wanna do the hared langchain and mongodb exports w/ the * as Name convention for continuing the better namespacing of modules in this package,

ie:

export * as MongoDB from "mongodb"; export * as Langchain from "langchain";

not doing in this PR b/c it'd dramatically increase the # of files touched.

relatedly, i think it'd probably be more correct to change these external dependencies that we re-export as peerDependencies in the package.json, but also since our monorepo is the main consumer of the mongodb-rag-core package, it probably would add more overhead and annoyance to us than value

I'm directionally ok with that export * as Namespace approach, though I want to make sure that it wouldn't make it harder to do tree shaking or otherwise bloat bundles.

If it is an issue, we might consider an alternative of two package entry points. i.e.

In this file, we export * from "./langchain" so that there's no namespace wrapper object

We need to create that file we export from, i.e. ./langchain/index.ts. It does the export * from "openai";

In package.json we add a exports for ./langchain.

So basically I could do either of these as a consumer:

import { LangChainGithubRepoLoader } from "mongodb-rag-core"; import { LangChainGithubRepoLoader } from "mongodb-rag-core/langchain";

Maybe too complex?

Interesting thought on the peer dependencies, I'd have to think through it a bit or see it in action

i'm notionally in agreement w/ the idea of mongodb-rag-core/<namespace>, however in practice i've repeatedly run into issues w/ any packages that we compose like this.

that's why i didn't use that pattern. but let me use this as an opportunity to at least properly understand why this doesn't work, even if we cant switch to it..tho maybe we'll be able to switch..let's see.

going to merge this PR, and play w/ this in separate PR. would require touching a lot of files b/c so many imports would be changed

nlarew

LGTM! Yeoman's work on this one. Just a few small nits & questions.

nlarew · 2024-10-31T17:42:04Z

packages/chatbot-server-mongodb-public/src/processors/extractMongoDbMetadataFromUserMessage.ts

@@ -41,7 +40,7 @@ const systemPrompt = `You are an expert data labeler employed by MongoDB.
 You must label metadata about the user query based on its context in the conversation.
 Your pay is determined by the accuracy of your labels as judged against other expert labelers, so do excellent work to maximize your earnings to support your family.`;

-const fewShotExamples: ChatCompletionMessageParam[] = [
+const fewShotExamples: OpenAI.default.Chat.ChatCompletionMessageParam[] = [


Weird type is an unfortunate side effect of the namespace exports :/

yeah, also the openai package does some funky stuff w/ exports

packages/chatbot-server-mongodb-public/src/processors/makeStepBackUserQuery.test.ts

nlarew · 2024-10-31T17:54:34Z

packages/chatbot-server-mongodb-public/src/test/mockOpenAi.ts

+  // This module is also required for the mock to work
+  // in the tests
+  const { updateFrontMatter } = jest.requireActual("mongodb-chatbot-server");


Interesting - do you know why updateFrontMatter but not any other exports from the server?

yea b/c that's needed in one of the methods. don't want it overwritten

I guess the meat of the question is - when would we need to add other methods here? Like is this for a specific test or just in general we need the actual function?

nlarew · 2024-10-31T18:00:55Z

packages/mongodb-artifact-generator/src/chat/makeClassifier.ts

+    if (
+      response.function_call === undefined ||
+      response.function_call === null
+    ) {


Can keep this as a one-liner if you want

Suggested change

if (

response.function_call === undefined ||

response.function_call === null

) {

if ((response.function_call ?? undefined) === undefined) {

or if the typing makes sense could go even simpler

Suggested change

if (

response.function_call === undefined ||

response.function_call === null

) {

if (!response.function_call) {

keeping explicit in case of ""

nlarew · 2024-10-31T18:06:07Z

packages/mongodb-chatbot-server/src/routes/generateResponse.ts

@@ -458,16 +460,21 @@ export async function streamGenerateResponseMessage({
    });
  }

-  return { messages: newMessages };
+  return { messages: newMessages.map(convertMessageFromLlmToDb) };


Why add the convertMessageFromLlmToDb() call here?

b/c in our implementation it's still type functionCall, whereas the new openai lib is function_call

nlarew · 2024-10-31T18:17:06Z

packages/mongodb-rag-core/src/dataSources/MdOnGithubDataSource.test.ts

  ...baseChatbotRepoConfig,
  name: "ingest_testData",
  metadata: {
    productName: "Ingest Test Data",
  },
-  filter: (path) => path.includes("ingest/testData"),
+  filter: (path) => path.includes(Path.join("mongodb-rag-core", "testData")),


Do we need the Path import here? Can't you just do "mongodb-rag-core/testData" since this is joining two static strings?

not if you're on windows!

…BackUserQuery.test.ts Co-authored-by: Nick Larew <nick.larew@mongodb.com>

mongodben added 10 commits October 29, 2024 17:10

fix broken core tests

56b3b90

merge shuffle changes

482b973

remove azure/openai stuff

4f94904

Fix build errs

3ad4765

Fix build err

97ff2f5

more build err fixes

629e1b5

fix test errs

dbd2216

fix code broken

bb25cb5

fix broken tests

9aaf3b9

suite passing

32c30be

mongodben marked this pull request as ready for review October 31, 2024 13:54

mongodben commented Oct 31, 2024

View reviewed changes

mongodben changed the title ~~(EAI-587): Clean up shared imports in mongodb-rag-core~~ (EAI-587): Clean up shared imports in mongodb-rag-core + migrate @azure/openai to openai Oct 31, 2024

nlarew approved these changes Oct 31, 2024

View reviewed changes

Update packages/chatbot-server-mongodb-public/src/processors/makeStep…

a0d6415

…BackUserQuery.test.ts Co-authored-by: Nick Larew <nick.larew@mongodb.com>

mongodben merged commit 3eb647d into main Oct 31, 2024
1 check was pending

mongodben deleted the EAI-587 branch October 31, 2024 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(EAI-587): Clean up shared imports in mongodb-rag-core + migrate `@azure/openai` to `openai` #546

(EAI-587): Clean up shared imports in mongodb-rag-core + migrate `@azure/openai` to `openai` #546

mongodben commented Oct 30, 2024 •

edited

Loading

mongodben Oct 31, 2024

nlarew Oct 31, 2024

nlarew Oct 31, 2024

mongodben Oct 31, 2024

mongodben Oct 31, 2024

mongodben Oct 31, 2024

nlarew left a comment

nlarew Oct 31, 2024

mongodben Oct 31, 2024

nlarew Oct 31, 2024

mongodben Oct 31, 2024

nlarew Oct 31, 2024 •

edited

Loading

nlarew Oct 31, 2024

mongodben Oct 31, 2024

nlarew Oct 31, 2024

mongodben Oct 31, 2024

nlarew Oct 31, 2024

mongodben Oct 31, 2024

(EAI-587): Clean up shared imports in mongodb-rag-core + migrate @azure/openai to openai #546

(EAI-587): Clean up shared imports in mongodb-rag-core + migrate @azure/openai to openai #546

Conversation

mongodben commented Oct 30, 2024 • edited Loading

Changes

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlarew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlarew Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

(EAI-587): Clean up shared imports in mongodb-rag-core + migrate `@azure/openai` to `openai` #546

(EAI-587): Clean up shared imports in mongodb-rag-core + migrate `@azure/openai` to `openai` #546

mongodben commented Oct 30, 2024 •

edited

Loading

nlarew Oct 31, 2024 •

edited

Loading