xenova / transformers.js

State-of-the-art Machine Learning for the web. Run 🤗 Transformers directly in your browser, with no need for a server!
https://huggingface.co/docs/transformers.js
Apache License 2.0
10.97k stars 669 forks source link

[Question] Any plan to rewrite js in typescript ? #386

Closed pnocera closed 10 months ago

pnocera commented 10 months ago

I'm doing it for my own usage although I'm loosing the benfit of upgrades.

Typings are usefull you know :)

While doing it I found this, in models.js, line 1027 :

let sampledTokens = sampler(logits);

should be

let sampledTokens = sampler.sample(logits);
xenova commented 10 months ago

Hi there 👋

The usage in models.js is indeed correct. Sampler extends Callable, which makes the object itself "callable", and when called, runs the .sample() method of the class. The reason for doing this is to mainly align with the python library (and how python uses __call__() methods), and allows things like pipelines to be called as functions, even though they are classes.

https://github.com/xenova/transformers.js/blob/e440372c99099c53d2efadd69c411f5c92f78efb/src/utils/generation.js#L621-L651

pnocera commented 10 months ago

Got it, thanks for answering.

kungfooman commented 10 months ago

Which IDE are you using? The project is typed already and you have IntelliSense in VSCode aswell.

You can crank up the strictness level to see more issues:

jsconfig.json:

{
    "compilerOptions": {
        // ...
        "strict": true,
        // ...
    }
}

Typings are usefull you know :)

Thank you for the information :wink: Exactly the reason we have them, but without requiring tsc in the build step - which allows you to use modern browser features like ESM and import maps: coding without bundling :+1:

pnocera commented 10 months ago

I'm using webstorm. I'm moving to typescript mostly because I'm working on a nodeJs server side project, and had errors at bundle time on require() statement. At least it's instructive to dig in the code ! kudos for the great job btw :)

kungfooman commented 10 months ago

I'm using webstorm.

Interesting, so I never used that, but just googled a bit: https://blog.jetbrains.com/webstorm/2023/06/webstorm-2023-2-eap7/

WebStorm using JSDoc

You may need to update for proper LSP support?

I'm moving to typescript mostly because I'm working on a nodeJs server side project, and had errors at bundle time on require() statement.

Nowadays node and all common bundlers have very good ESM import/export support :wink:

pnocera commented 10 months ago

I got a license for Webstorm a month ago and upgraded to the all Jetbrains suite one week after. Pycharm, Goland and Rider are awesome too. I have to admit I didn't search further after I got the error. My tsconfig.json has a target for ES2021 and changing it would probably fix it. I just went on the path "let's grab that thing and build it...oh javascript...let's move to ts" Webstorm has some nifty helpers too

image

Now I just have the processor, tokenizer and transformer to convert...

kungfooman commented 10 months ago

I just went on the path "let's grab that thing and build it...oh javascript...let's move to ts"

Yea, the brain goes into auto-pilot... I was also once convinced that everything must be TypeScript :sweat_smile:

Well, then I converted all my sources back to JavaScript... and I get things done way quicker again.

pnocera commented 10 months ago

Now that I'm reading your code I get your point.

Extremely well documented. all the typings are there :1st_place_medal:

I still like TS interfaces though. Abstract classes are cool too.

The only suggestion I would make is splitting the models into separate files, eventualy.

And once again - Bravo - as we say in french, and thanks for your work.

erickDevUp commented 10 months ago

here is a next-client example in ts and next14: https://github.com/erickDevUp/hugginface-test

kungfooman commented 10 months ago

The only suggestion I would make is splitting the models into separate files, eventualy.

Yep, it's a good point, you could open a separate issue for this request... IIRC we discussed this before, it would also fit better with the dir layout in e.g. https://github.com/huggingface/transformers/tree/main/src/transformers/pipelines

Otherwise you can close this now - this repo is heavily typed, but of course there are a few types that could always require some improvement, if you like to work with types to get to know the source code.

pnocera commented 10 months ago

Well I switched back to python, as onnx on nodejs doesn't provide cuda which is a no go for me. I'll close the issue now. Thanks.