xenova
npm run typegen - generate TypeScript declaration files
Fixes https://github.com/xenova/transformers.js/issues/25
This is a basic type setup, which enables type-checking/Intellisense aswell:
I just added two example JSDoc comments, to display how it works.
Since @chelouche9 asked for it, I would like to assign you to work out more types. Are you capable of doing so?
Whenever you have added more types, you can run:
npm run typegen
And commit both the JSDoc comments and the dist/types changes, so we have synchronized types.
Wow this is great! Thanks so much! I'll wait for @chelouche9 to respond before proceeding to merge into main.
Excuse the potentially silly question (I am not too familiar with typescript haha), but does it have to be a dependency on the library itself, or can it be made into a dev dependency?
I could also try to add types while I write the code. I assume I just follow what you did and add annotations to the methods?
@kungfooman sure, I would be happy to add more types to the project. Just to make sure I understand, I need to add new JSDoc comments, then run the typegen command and the declaration files will be created?
@xenova anything I need to know about running the project, tests, etc..?
@xenova anything I need to know about running the project, tests, etc..?
My "test-suite" (if you can even call it that) is very simple. I made it before actually creating the npm package, so it is still just an ugly script that I run with npm run test (https://github.com/xenova/transformers.js/blob/main/tests/index.js)
To run the tests with local models (ofc much faster for development), you need to have the models stored in the models folder (https://github.com/xenova/transformers.js/tree/main/models) with the same structure as the HF repo I use to store the remote models: https://huggingface.co/Xenova/transformers.js. For example, you will have folders like this:
./models/onnx/quantized/openai/whisper-tiny.en/speech2seq-lm-with-past
Alternatively, you can disable using local models: https://github.com/xenova/transformers.js/blob/4ad2581604f2b8da37e8842f9453d02ae1c4f5e4/tests/index.js#L14
but since I haven't implemented local caching, you will end up downloading the model each time you run the tests (not ideal)
Other than that, you should just be able to clone the github repo and it should work fine.
@xenova great, I will start working on that once you merge :)
Actually, do you mind working on this branch (or a fork of it)? Then I can merge it when it's all ready?
Sure. Although it might take some time to complete all, I though we can split the work to not lose track of the master.
Sure. Although it might take some time to complete all, I though we can split the work to not lose track of the master.
Great! The library is expanding extremely quickly, so there might be quite a lot of breaking changes, but many functions should remain the same (and I will take responsibility for managing any conflicts before merging into main)
Excuse the potentially silly question (I am not too familiar with typescript haha), but does it have to be a dependency on the library itself, or can it be made into a dev dependency?
Sorry, my bad, TypeScript is of course a dev dependency only. I fixed the package.json and package-lock.json via npm install --save-dev typescript.
I could also try to add types while I write the code. I assume I just follow what you did and add annotations to the methods?
Right, it's rather simple but some types can become complicated aswell. JSDoc is pretty powerful, but if I don't know the syntax, I've had good experiences with just asking OpenAI :sweat_smile:
@kungfooman sure, I would be happy to add more types to the project. Just to make sure I understand, I need to add new JSDoc comments, then run the typegen command and the declaration files will be created?
That's right, everything is quite straight-forward with types. :+1:
Sure. Although it might take some time to complete all, I though we can split the work to not lose track of the master.
I have the same sentiment, typing is something that developes over time. Usually when I'm stuck with some code that I cannot understand so far, I add types as a mental prop e.g.
It doesn't take too much time, in VSCode you just move the cursor over e.g. a method, type /**[enter] and it will auto-generate the entire @param template.
Right, it's rather simple but some types can become complicated aswell. JSDoc is pretty powerful, but if I don't know the syntax, I've had good experiences with just asking OpenAI 😅
I am using it as well to do all the heavy lifting 😝
Sorry, my bad, TypeScript is of course a dev dependency only. I fixed the package.json and package-lock.json via npm install --save-dev typescript.
I added it to my PR. I cannot push to your branch @kungfooman. If you guys have a better idea to manage those branhes, let me know.
I added it to my PR. I cannot push to your branch @kungfooman. If you guys have a better idea to manage those branhes, let me know.
Thank you for being pro-active! I added you both as collaborators on my fork, so that should work, once you accepted the invite.
I hope you know how to add remotes and pull/push to them
Another way is of course to wait until this is merged and then do PR's as usually. But in the mean time, this should be possible now.
@kungfooman I applied all the changes on my fork already. I will keep doing it from there :)
@kungfooman I applied all the changes on my fork already. I will keep doing it from there :)
Nice, so either:
- This will be merged first and you can merge main branch
- Your PR will be merged directly
I have no hard feelings for either, I'm glad about your support :muscle: :+1:
Once a typing base exists, it will be easier to make more fine-tuned type fixes here and there in smaller/more-manageable PR's
@kungfooman are you actively work on this branch? I though for some reason you have made the setup and stopped
@kungfooman are you actively work on this branch?
I just keep it up to date basically, so either way (1) or (2) should work out nicely
@kungfooman If you don't mind, I will keep it on my fork. I have already set it up and everything is working fine :) Nonetheless, maybe @xenova will decide to merge this PR first. I will make sure to update and fix conflicts.