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

Support /exports from mongodb-rag-core #548

Merged
merged 6 commits into from
Nov 4, 2024
Merged

Conversation

mongodben
Copy link
Collaborator

@mongodben mongodben commented Oct 31, 2024

Jira: n/a

Changes

  • Switch repo to support /exports. To make this work, I had to add the following to the root tsconfig.json:
{
    // ...
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    // ...
}
  • Update imports to use new /export mongodb-rag-core package
  • Remove chatbot-eval-mongodb-public (it's unused)
    • Note: was unable to use mongodb-chatbot-evaluation b/c it has some modules used in benchmarks. we should remove that and move the modules over in a separate PR.

Notes

@mongodben mongodben marked this pull request as ready for review November 1, 2024 14:43
@mongodben mongodben requested a review from nlarew November 1, 2024 16:01
@@ -15,14 +16,14 @@ export const makeRadiantChatLlm = async ({
deployment: string;
mongoDbAuthCookie?: string;
lmmConfigOptions: Pick<
OpenAI.default.Chat.ChatCompletionCreateParams,
OpenAI.Chat.ChatCompletionCreateParams,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't dislike the namespaced types here (very explicit what it is!). Is it possible to import just ChatCompletionCreateParams without the whole OpenAI namespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think so. dont think it's a real issue tho b/c the benchmarks package is only being built in this repo, which will already install all of mongodb-rag-core, which is dependent on the openai package

Copy link
Collaborator

@nlarew nlarew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % a few questions throughout

@@ -1,33 +1,29 @@
export const makeMockOpenAIToolCall = (funcRes: Record<string, unknown>) => {
// This module is also required for the mock to work
// in the tests
const { updateFrontMatter } = jest.requireActual("mongodb-chatbot-server");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer required? Should we delete the comment above it too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def delete comment too. will implement

packages/mongodb-chatbot-server/src/test/testConfig.ts Outdated Show resolved Hide resolved
// LLM function
const translator = createJsonTranslator<SchemaType>(
model,
const validator = createTypeScriptJsonValidator<SchemaType>(
Copy link
Collaborator

@nlarew nlarew Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why update our TypeChat code? Is there a new version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a version bump in the diff - do we need to update scripts/package.json?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe? i'll set the version in package.json to whatever is most recent

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version in package.json is the latest. i'm confused what's going on, but will keep as is since it works and i dont think this usage is important enough for us to worry too much about

Comment on lines +5 to +6
"module": "NodeNext",
"moduleResolution": "NodeNext",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some considerable implications in changing the module format. I think this is something we should do, but we'll want to monitor for impact + maybe do a major version bump

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the main implication of switching to NodeNext is that now it supports both common js and esm by default rather than just commonjs.

@mongodben mongodben merged commit aaa1cce into main Nov 4, 2024
1 check passed
@mongodben mongodben deleted the core_named_exports branch November 4, 2024 20:11
nlarew pushed a commit that referenced this pull request Nov 7, 2024
* fix build errs

* Experimental modules

* fix broken tests

* update exports

* Fix broken mock
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