transformers.js
transformers.js copied to clipboard
xenova
Typegen
@xenova this is only the utils.js file. It will take some time lol. I also think we might add a prettier and linter so people won't override all file like me 😝.
There are a few cases of @class and @constructor which aren't needed (basically artifacts of ES5 style function-based OOP), otherwise this is looking very good :+1:
I will close my PR so we don't have to mess around with two PR's. Thank you for the great work and time!
I just resolved a conflict coming from main. Let me know when you want me to review and test fully 👍
I just resolved a conflict coming from main. Let me know when you want me to review and test fully 👍
@xenova I have left only 3 files. Hopefully in two days.
I just resolved a conflict coming from main. Let me know when you want me to review and test fully 👍
@xenova I have left only 3 files. Hopefully in two days.
Sweet! No rush :)
@xenova The PR is ready, all files have JSdoc now :)
But I have to say it is far from perfect. From now we need to make incremental changes and fixes. There are a lot of types and code adjustments to make. We can do them in small steps after it's merged.
Nice! 🎉 I'm almost done adding a few more models/tasks to main, then I'll review, merge, and finally add any missing docs.
Just a general question for you @xenova, do you prefer to keep the source in JavaScript vs. moving some things over to TypeScript?
Just a general question for you @xenova, do you prefer to keep the source in JavaScript vs. moving some things over to TypeScript?
Not too sure 👀 ... JS has been easier (and faster) to develop in since I don't have much experience with TS.
But I'm pretty sure that eventually, the project will transition to TS.
But I'm pretty sure that eventually, the project will transition to TS.
I hope not. TS is useful powering a language server, but actually writing TS is ugly once the types become a bit more complicated. And you lose the ability to load code directly as ES6 modules in the browser via <script type="module">.
I would urge caution with regards to those who seem overly enthusiastic about TS, as their enthusiasm is often just following "that shiny new thing", while being disconnected from pragmatism.
Simple job for you, @biw, to make things clear, translate this to TypeScript:
/**
* @example
* forInMap({a: 11, b: 22}, (k, v) => v); // [11, 22]
* @param {Record<KEY, VAL>} o - The object.
* @param {(key: string, value: VAL) => RET} lambda - The function that operates on the key/value pairs.
* @template {string | number | symbol} KEY - The key.
* @template VAL - Type of the value given to `lambda`.
* @template RET - Type of the return value of `lambda`.
* @returns {RET[]}
*/
function forInMap(o, lambda) {
return Object.keys(o).map(key => lambda(key, o[key]));
}
@xenova The PR is ready, all files have JSdoc now :)
Great work :+1:
I looked over it and src/utils.js received way more changes than necessary. I see the value of a linter, but maybe we have to keep it separate? It looks like it moved methods up and down etc.
@kungfooman thanks for the feedback. The linter is turned off. I think it happened when resolving conflicts. I will have a look and fix :)
I hope not. TS is useful powering a language server, but actually writing TS is ugly once the types become a bit more complicated. And you lose the ability to load code directly as ES6 modules in the browser via
<script type="module">.
Since there's already a build pipeline with webpack, this isn't an issue. 😄
I would urge caution with regards to those who seem overly enthusiastic about TS, as their enthusiasm is often just following "that shiny new thing", while being disconnected from pragmatism.
As someone who's been shipping typescript code in production for over four years, my intentions are not to chase the new shiny thing. Just trying to help out if the need is there. Not trying to force anyone to use anything 😄
Simple job for you, @biw, to make things clear, translate this to TypeScript:
/** * @example * forInMap({a: 11, b: 22}, (k, v) => v); // [11, 22] * @param {Record<KEY, VAL>} o - The object. * @param {(key: string, value: VAL) => RET} lambda - The function that operates on the key/value pairs. * @template {string | number | symbol} KEY - The key. * @template VAL - Type of the value given to `lambda`. * @template RET - Type of the return value of `lambda`. * @returns {RET[]} */ function forInMap(o, lambda) { return Object.keys(o).map(key => lambda(key, o[key])); }
Here you go!
function forInMap2<V, K extends Record<string | number | symbol, V>, Ret>(
o: K,
lambda: (key: keyof K, value: K[typeof key]) => Ret,
) {
return Object.keys(o).map((key) => lambda(key, o[key as keyof K]))
}
Not trying to start a flame war, I just wanted to be helpful if @xenova had an interest in using TypeScript.
@xenova just checking what is missing to merge this :)
Hi! No need to do anything on your side - I will get to fully reviewing (and merging) by the end of the week (hopefully lol).
Not trying to start a flame war, I just wanted to be helpful if @xenova had an interest in using TypeScript.
Haha no worries! 🤣 I'm definitely open to learning new tech, so, we shall see ;) For now, as you'd expect, I'll continue developing in JS, otherwise, I'd probably not be able to keep up with all the craziness in the AI space right now 😄
Fixed the conflicts :+1: VSCode is displaying quite a few warnings/errors, and it will probably be good to get those (mostly) resolved before merging. There are also some methods/classes which were added recently that aren't fully documented. If one of you wants to maybe help with these 2 things, that would be great!
Some other exciting news: I've been talking to some HF staff, and they proposed hosting the docs on https://huggingface.co/docs/ 🤯 , and then listing Transformers.js as a community-driven open-source project! 🔥 Super exciting!
Mentioning it here; I used this as a base for a full .ts conversion (well, not full full, but getting there! 😉) in https://github.com/jcbhmr/transformers.js/pull/13
comparison link: https://github.com/chelouche9/transformers.js/compare/typegen...jcbhmr:transformers.js:use-ts
Some other exciting news: I've been talking to some HF staff, and they proposed hosting the docs on https://huggingface.co/docs/ 🤯 , and then listing Transformers.js as a community-driven open-source project! 🔥 Super exciting!
Wow great news! Good job @xenova. Your hard work will reach much more people.
Fixed the conflicts 👍 VSCode is displaying quite a few warnings/errors, and it will probably be good to get those (mostly) resolved before merging. There are also some methods/classes which were added recently that aren't fully documented. If one of you wants to maybe help with these 2 things, that would be great!
I was off the last couple of days. I will start tomorrow to try and fix some conflicts. Do you have any leads what is the most important part need fixing?
I was off the last couple of days. I will start tomorrow to try and fix some conflicts. Do you have any leads what is the most important part need fixing?
Sounds good! 🔥 Other than some missing JSDocs (after merging main into the PR), it is just some warnings. The tests run successfully, so, there's nothing "fatally" wrong...
The main thing that pops up is a "Property not defined" error, which is understandable since not all models share the same instance variables (and because of the way I'm doing inheritance to reduce code duplication). For example, encoder-decoder must define some extra properties such as num_decoder_layers, num_decoder_heads, decoder_dim_kv, num_encoder_layers, num_encoder_heads, and encoder_dim_kv. Of course, these don't exist on the base class, so, it is giving a warning saying that we are accessing/setting a property that doesn't exist. Not too sure how to hide/fix that though. If there is a way to globally disable those problems, that could work.
So, other than just adding the missing JSDocs, you could maybe just try fix/reduce some of the warnings (no real particular order tbh). If you use VSCode, you should be able to see the "Problems" (there are quite a lot haha).
@xenova I rebased over main and there were many conflicts. I hope nothing breaks.
Merging this PR as soon as possible will help us avoid doing those time-consuming rebase. However, we don't want to add all the errors. Since adding types to the code requires a deep understanding of its usage, I suggest that I will change it to any anywhere it breaks and we will incrementally change it over time.
What do you think?
Merging this PR as soon as possible will help us avoid doing those time-consuming rebase.
Agreed! I'm trying to get to all open PRs now (with this one being the number 1 priority).
However, we don't want to add all the errors. Since adding types to the code requires a deep understanding of its usage, I suggest that I will change it to any anywhere it breaks and we will incrementally change it over time.
What do you think?
Sounds good 👌
@xenova I have fixed everything that is "type" related. Some errors are due to different js versions (like not using the spread operator below ecma2015), and some are about the _call method on objects. To fix them we need to make some code changes. We can do that on dedicated PRs later.
There were a lot of conflicts, I made sure everything is correct but I would recommend you test it and give an additional look :)
Awesome! 💪 Once again, it is kind of difficult to see where you made the changes, so, could you fix the formatting like you did before? Thanks :)
I'll try get it merged tonight if I can 🥳
Awesome! 💪 Once again, it is kind of difficult to see where you made the changes, so, could you fix the formatting like you did before? Thanks :)
@xenova done
@xenova I made another go. Now all the files are clean from the formatting and the weird code changes.
Great work, I looked through the PR once more and it just looks like minor issues, which can make it just a bit easier to read/maintain/code 👍
I am impressed by all the work, I hope it doesn't feel too nitpicky 😅 🙈
@xenova @kungfooman committed all changes. Thank you for the detailed review :)
Let me know if we need something else.
@xenova @kungfooman committed all changes. Thank you for the detailed review :)
Let me know if we need something else.
I think that is it then!!! 😍
I'm currently fixing a bug introduced by the latest version of optimum, which is quite important since it means no one can use their new models 👀
I will get to merging this into main the second that is fixed! Super excited! 🔥 Thanks to everyone for their hard work 🙏
@chelouche9 I'm almost done with my pass over everything (it's taking so long haha). I tried to push, but I apparently don't have write access to your branch. Could you add me as a collaborator?