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

js_release_alpha/1.5.12 #1314

Merged
merged 19 commits into from
Nov 7, 2023
Merged

js_release_alpha/1.5.12 #1314

merged 19 commits into from
Nov 7, 2023

Conversation

jeffchuber
Copy link
Contributor

@jeffchuber jeffchuber commented Oct 31, 2023

should be stacked on top of #1305

merge #1305 first

to ship this, we are removing WebAI and Transformers support (they are much trickier). They will be added back soon.

Copy link

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)

this.api_key = cohere_api_key;
this.model = model || "large";
}

public async generate(texts: string[]) {
const response = await CohereAiApi.embed({

if (this.cohereAiApi === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this part into the import function. Because it is import logic and not generation logic. It would make the generate function cleaner if you would do

await CohereEmbeddingFunction.import();
const response = await this.cohereAiApi.embed({

The import function would then immediately return if the package is already imported.

Copy link
Contributor

Choose a reason for hiding this comment

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

you could move it to another function called "loadClient"
something like

private async loadClient(){

if(this.cohereAiApi) return;
   try {
                // eslint-disable-next-line global-require,import/no-extraneous-dependencies
                const { cohere } = await CohereEmbeddingFunction.import();
                CohereAiApi = cohere;
                CohereAiApi.init(this.api_key);
            } catch (_a) {
                // @ts-ignore
                if (_a.code === 'MODULE_NOT_FOUND') {
                    throw new Error("Please install the cohere-ai package to use the CohereEmbeddingFunction, `npm install -S cohere-ai`");
                }
                throw _a; // Re-throw other errors
            }
            this.cohereAiApi = CohereAiApi;

        }


}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i like this change - thanks! will implement

@jeffchuber jeffchuber changed the title Fix/dependency mgmt for JS client js_release_alpha/1.5.12 Nov 1, 2023
@jeffchuber
Copy link
Contributor Author

So this is really dumb, but we ended up having to move off of yarn to just doing npm run to avoid issues with jackspeak cliui string-width dep issues. (very common if you google it)

@jeffchuber
Copy link
Contributor Author

alas- fix one thing, another nasty issue presents its head. i will continue to debug this.

Copy link

Choose a reason for hiding this comment

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

i think this was the main part where I got ESM/CJS error.
using ESNext certainly can fix it

Copy link

Choose a reason for hiding this comment

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

if you consider 'chromadb' is a CommonJS module as the main problem:

import { ChromaClient } from 'chromadb';
         ^^^^^^^^^^^^
SyntaxError: Named export 'ChromaClient' not found. The requested module 'chromadb' is a CommonJS module, which may not support all module.exports as named exports.                                                                                    ai 
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'chromadb';
const { ChromaClient } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jexroid i dont quite understand - do you approve of this PR or do you think we should do it a different way?

Copy link

Choose a reason for hiding this comment

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

No, the best considerations have been made. here is what I mean:
as you can see in the last code we had: "module": "CommonJS". which tells typescript to ONLY
use Commonjs, no matter what you have been told to package.json module to do. it will return you commonjs type for your javascript which will not allow you to use:

import { ChromaClient } from 'chromadb';

because It's not commonjs but it's the module type and you will get :

SyntaxError: Named export 'ChromaClient' not found. The requested module 'chromadb' is a CommonJS module, which may not support all module.exports as named exports.                                                                                    ai 
CommonJS modules can always be imported via the default export, for example using:

but when @perzeuss changed it to ESNext, it told typescript to use a standard and support version of module type. here is a link that can help you with ESNext:
microsoft/TypeScript#24082
https://www.typescriptlang.org/tsconfig#target

from my perspective, this was the Main problem that causes ESM/Commonjs syntax error.
if there is any other misunderstanding that you need to be sure about, I would be pleased to help

Copy link

@jexroid jexroid left a comment

Choose a reason for hiding this comment

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

the main problem was in tsconfig and ESNext will fix it

yarn.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

can be removed?!

Copy link
Contributor

Choose a reason for hiding this comment

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

you should commit this file, if you remove yarn.lock

yarn
yarn test:run

# moved off of yarn to npm to fix issues with jackspeak/cliui/string-width versions #1314
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this jackspeak/cliui/string-width issue about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you google it - there is a bunch about it - eg

isaacs/jackspeak#5

@jeffchuber
Copy link
Contributor Author

Trying to get tests to cooperate

@jeffchuber jeffchuber merged commit 4e2fafa into main Nov 7, 2023
95 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants