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

fix: use workaround for dynamic import #956

Merged

Conversation

perzeuss
Copy link
Contributor

@perzeuss perzeuss commented Aug 9, 2023

resolves #953

Description of changes

Test plan

At present, we lack a testing setup specifically tailored for a browser environment. Implementing this would be a necessary step to implement tests for changes in this branch.

I've tested it locally using this change: https://github.com/jeffchuber/nextjs-chroma/pull/1/files

@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readbility, Modularity, Intuitiveness)

@jeffchuber
Copy link
Contributor

@perzeuss this is great! great work figuring this out.

I wanted to square up this comment from @shuding (on the vercel nextjs team afaik) #824 (comment) --> would this be an alternate way of solving the same issue?

@perzeuss
Copy link
Contributor Author

@perzeuss this is great! great work figuring this out.

I wanted to square up this comment from @shuding (on the vercel nextjs team afaik) #824 (comment) --> would this be an alternate way of solving the same issue?

No, the comment you're referring to is about the lack of an ECMAScript Modules (ESM) build. As I mentioned in issue #953, Chroma isn't currently optimized for browser environments. The suggestion in the comment is to include an ESM build target, which would generate a separate build specifically for browser environments. They did not know that we try to import a module which actually does not exist.

The main challenge we're facing is with Next.js's handling of module imports in the build process. Specifically, the bundler recognizes the import, tries to resolve the module to prevent later issues and throws an error when it can't find the package, even when the import is wrapped in a try-catch block. The try-catch block only applies when we actually try to import the module at runtime.

Our workaround involves wrapping the dynamic import so that the import is only evaluated at runtime. This way, the build tools do not try to locate the module and the try-catch block functions as expected.

@HammadB HammadB requested a review from jeffchuber August 10, 2023 17:42
clients/js/src/utils.ts Outdated Show resolved Hide resolved
@perzeuss perzeuss mentioned this pull request Aug 10, 2023
@chekun
Copy link

chekun commented Aug 12, 2023

anyone reviews this and get it merged?

@perzeuss perzeuss force-pushed the fix/dynamic-import-workaround branch from 2e4960a to 471995e Compare August 16, 2023 16:02
@perzeuss
Copy link
Contributor Author

@jeffchuber I am happy with the PR now, we could merge it :)

Copy link
Contributor

@jeffchuber jeffchuber left a comment

Choose a reason for hiding this comment

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

thanks! a couple questions. I tested locally and works great

clients/js/package.json Outdated Show resolved Hide resolved
clients/js/src/utils.ts Outdated Show resolved Hide resolved
@jeffchuber
Copy link
Contributor

@perzeuss why do we need to pin to the older version of openai? What broke?

@perzeuss
Copy link
Contributor Author

perzeuss commented Aug 17, 2023

@perzeuss why do we need to pin to the older version of openai? What broke?

@jeffchuber there is a breaking change with openai@^4
We'd have to migrate to this version in another branch.
openai@4 is 1 day old btw, so its not a downgrade.

I changed the pinning because actually we just care about the major versions.

@jeffchuber
Copy link
Contributor

@perzeuss thanks again for this! i was testing this tonight with https://github.com/jeffchuber/nextjs-chroma/

'use client' // 👈 use it here

import Image from 'next/image'
import styles from './page.module.css'
import { useEffect, useState } from 'react';
const {ChromaClient, OpenAIEmbeddingFunction} = require('chromadb');
const openaiClient = require("openai");

export default function Home() {

  const [hb, setData] = useState("none");

  useEffect(() => {
    console.log('useEffect');
    console.log('openaiClient', openaiClient);
    const embedder = new OpenAIEmbeddingFunction({openai_api_key: "sk-"})

    const client = new ChromaClient();
    const heartbeatFn = async () => {
      let hb = await client.heartbeat();
      console.log('hb', hb);
      setData(hb)

      const embeddings = embedder.generate(["document1","document2"])
      console.log('embeddings', embeddings);

      return hb
    }
    console.log('heartbeat', heartbeatFn());
  }, []);
Screenshot 2023-08-22 at 10 24 09 PM

But it's throwing this error - I can confirm that const openaiClient = require("openai"); work directly - however for some reason chromadb can't access it 🤔

@mharis
Copy link

mharis commented Aug 24, 2023

@perzeuss @jeffchuber How is it looking? After this update, langchain also requires dependency update. Right now, everything breaks unfortunately on nextjs.

@perzeuss
Copy link
Contributor Author

@perzeuss thanks again for this! i was testing this tonight with https://github.com/jeffchuber/nextjs-chroma/

'use client' // 👈 use it here

import Image from 'next/image'
import styles from './page.module.css'
import { useEffect, useState } from 'react';
const {ChromaClient, OpenAIEmbeddingFunction} = require('chromadb');
const openaiClient = require("openai");

export default function Home() {

  const [hb, setData] = useState("none");

  useEffect(() => {
    console.log('useEffect');
    console.log('openaiClient', openaiClient);
    const embedder = new OpenAIEmbeddingFunction({openai_api_key: "sk-"})

    const client = new ChromaClient();
    const heartbeatFn = async () => {
      let hb = await client.heartbeat();
      console.log('hb', hb);
      setData(hb)

      const embeddings = embedder.generate(["document1","document2"])
      console.log('embeddings', embeddings);

      return hb
    }
    console.log('heartbeat', heartbeatFn());
  }, []);
Screenshot 2023-08-22 at 10 24 09 PM But it's throwing this error - I can confirm that `const openaiClient = require("openai");` work directly - however for some reason `chromadb` can't access it 🤔

@jeffchuber you need to use the import syntax to import the esm build.

@jeffchuber jeffchuber self-requested a review August 31, 2023 15:46
Copy link
Contributor

@jeffchuber jeffchuber left a comment

Choose a reason for hiding this comment

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

this works! 🎉 just need to remove the dupe fn in utils

@perzeuss perzeuss force-pushed the fix/dynamic-import-workaround branch from 40a94c5 to 0946be5 Compare August 31, 2023 15:53
@jeffchuber jeffchuber merged commit 81ab0b1 into chroma-core:main Aug 31, 2023
tazarov pushed a commit to amikos-tech/chroma-core that referenced this pull request Aug 31, 2023
resolves chroma-core#953

## Description of changes
 -  Bug fixes
- implement a workaround in
clients/js/src/embeddings/WebAIEmbeddingFunction.ts to resolve chroma-core#953

## Test plan
At present, we lack a testing setup specifically tailored for a browser
environment. Implementing this would be a necessary step to implement
tests for changes in this branch.

I've tested it locally using this change:
https://github.com/jeffchuber/nextjs-chroma/pull/1/files
- http://localhost:3000/api/hello (node version) works fine
- http://localhost:3000 (browser version) works fine
tazarov pushed a commit to amikos-tech/chroma-core that referenced this pull request Aug 31, 2023
resolves chroma-core#953

## Description of changes
 -  Bug fixes
- implement a workaround in
clients/js/src/embeddings/WebAIEmbeddingFunction.ts to resolve chroma-core#953

## Test plan
At present, we lack a testing setup specifically tailored for a browser
environment. Implementing this would be a necessary step to implement
tests for changes in this branch.

I've tested it locally using this change:
https://github.com/jeffchuber/nextjs-chroma/pull/1/files
- http://localhost:3000/api/hello (node version) works fine
- http://localhost:3000 (browser version) works fine
tazarov pushed a commit to amikos-tech/chroma-core that referenced this pull request Aug 31, 2023
resolves chroma-core#953

## Description of changes
 -  Bug fixes
- implement a workaround in
clients/js/src/embeddings/WebAIEmbeddingFunction.ts to resolve chroma-core#953

## Test plan
At present, we lack a testing setup specifically tailored for a browser
environment. Implementing this would be a necessary step to implement
tests for changes in this branch.

I've tested it locally using this change:
https://github.com/jeffchuber/nextjs-chroma/pull/1/files
- http://localhost:3000/api/hello (node version) works fine
- http://localhost:3000 (browser version) works fine
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.

Issues running npm:chromadb inside nextjs
4 participants