Typegen #28

chelouche9 · 2023-03-18T20:16:16Z

No description provided.

chelouche9 · 2023-03-18T20:18:54Z

@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 😝.

kungfooman · 2023-03-19T10:43:03Z

src/utils.js

-        .map((x, i) => [i, x])            // Get indices ([index, score])
-        .sort((a, b) => b[1] - a[1])      // Sort by log probabilities
+  items = Array.from(items)
+    .map((x, i) => [i, x]) // Get indices ([index, score])


I would rather have the comments aligned and it might be best to focus on JSDoc only in one PR, otherwise it becomes hard to judge

kungfooman · 2023-03-19T10:45:46Z

src/utils.js

+  /**
+   * Creates a new instance of the Callable class.
+   *
+   * @constructor


@constructor not needed

kungfooman · 2023-03-19T10:45:57Z

src/utils.js

+   * This method should be implemented in subclasses to provide the
+   * functionality of the callable object.
+   *
+   * @method _call


@method _call not needed

kungfooman · 2023-03-19T10:46:17Z

src/utils.js

+   * @method _call
+   * @throws {Error} Must implement _call method in subclass
+   */
+  _call(...args) {


args param is not in jsdoc

kungfooman · 2023-03-19T10:47:04Z

src/utils.js

 function isIntegralNumber(x) {
-    return Number.isInteger(x) || typeof x === 'bigint'
+  return Number.isInteger(x) || typeof x === "bigint";


Why bother replacing every ' with "?

It's my prettier. Forgot to disable it.

kungfooman · 2023-03-20T13:59:27Z

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 👍

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!

xenova · 2023-03-21T01:50:16Z

I just resolved a conflict coming from main. Let me know when you want me to review and test fully 👍

src/tensor_utils.js

chelouche9 · 2023-03-21T17:21:24Z

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.

xenova · 2023-03-21T17:48:01Z

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 :)

chelouche9 · 2023-03-23T20:08:45Z

@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.

xenova · 2023-03-23T20:12:56Z

Nice! 🎉 I'm almost done adding a few more models/tasks to main, then I'll review, merge, and finally add any missing docs.

biw · 2023-03-23T23:52:48Z

Just a general question for you @xenova, do you prefer to keep the source in JavaScript vs. moving some things over to TypeScript?

xenova · 2023-03-23T23:56:07Z

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.

kungfooman · 2023-03-24T10:22:08Z

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]));
}

src/processors.js

kungfooman · 2023-03-27T10:40:44Z

@xenova The PR is ready, all files have JSdoc now :)

Great work 👍

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.

chelouche9 · 2023-03-27T10:45:31Z

@kungfooman thanks for the feedback. The linter is turned off. I think it happened when resolving conflicts. I will have a look and fix :)

chelouche9 · 2023-03-29T16:57:41Z

@xenova just checking what is missing to merge this :)

biw · 2023-03-29T18:12:09Z

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 · 2023-03-29T22:10:15Z

@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).

xenova · 2023-03-29T22:12:25Z

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 😄

Replace with `AutoModelForObjectDetection`

…ecified)

xenova · 2023-04-04T17:32:19Z

@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?

chelouche9 · 2023-04-04T17:44:57Z

@xenova now you have permissions.

xenova · 2023-04-04T17:46:07Z

@xenova now you have permissions.

Thanks! I've pushed my changes. You and @kungfooman can do a quick skim over everything if you'd like... and then I'll merge into main 🔥

Also, I assume we have to run the build command each time before release? If so, then you're probably right that it's a good idea to remove dist from the repo. Perhaps @kungfooman can give some advice in that regard.

chelouche9 · 2023-04-04T18:30:08Z

Thanks! I've pushed my changes. You and @kungfooman can do a quick skim over everything if you'd like... and then I'll merge into main 🔥

I see you added some changes to the code except for jsdoc. For the annotation part looks good to me :)

kungfooman · 2023-04-05T10:37:59Z

Also, I assume we have to run the build command each time before release? If so, then you're probably right that it's a good idea to remove dist from the repo. Perhaps @kungfooman can give some advice in that regard.

The only reason I like to commit build artifacts is to be able to see potential regressions, e.g. while rewriting a documentation system to keep track of the changes in the generated documentation files.

In such cases I just create a throw-away repo and commit as many files I want.

But I don't like all these artifact files in the "main repo", since I don't like large repositories. Cloning just takes longer and longer and it will probably also decrease the amount of contributors over time.

It's likely to result in merge conflicts aswell and I prefer not to spend time resolving merge conflicts in build artifacts.

I also like to use the file-search in VSCode and I don't want to put "dist" in "Exclude dirs" all the time, to filter for the SSOF (single source of truth) e.g. finding some specific types to improve.

TLDR: my suggestion is to remove /dist/ from repo

chelouche9 force-pushed the typegen branch from eb119cb to 5a14917 Compare March 18, 2023 20:28

xenova mentioned this pull request Mar 18, 2023

Error: invalid input 'encoder_attention_mask' #23

Closed

chelouche9 mentioned this pull request Mar 19, 2023

npm run typegen - generate TypeScript declaration files #26

Closed

kungfooman reviewed Mar 19, 2023

View reviewed changes

chelouche9 force-pushed the typegen branch from 3bf99d0 to eef6096 Compare March 20, 2023 08:35

kungfooman reviewed Mar 21, 2023

View reviewed changes

src/tensor_utils.js Outdated Show resolved Hide resolved

chelouche9 force-pushed the typegen branch from 8de8b33 to fcaac71 Compare March 21, 2023 16:34

chelouche9 force-pushed the typegen branch 2 times, most recently from 8679602 to e3ddccf Compare March 23, 2023 20:08

kungfooman reviewed Mar 27, 2023

View reviewed changes

src/processors.js Outdated Show resolved Hide resolved

xenova added 24 commits April 4, 2023 16:21

Remove T5ForConditionalGeneration import/export

5f0d592

Replace with `AutoModelForObjectDetection`

Add guard before assigning this.generation_config to gen_config

7269c82

Refactor Question Answering pipeline call function

3cc7665

Use for loop instead of for...in

deff2ea

Fix FileResponse clone method

5fc819e

Call super() in Callable constructor

18bc7d6

Reorder JSDoc

93fb872

Update Callable JSDoc

ef0093d

Ignore _call warning on closure

362a489

Remove tokenizer.batch_decode from JSDoc

74921cf

Add cleanup property to word piece decoders

0b9ace8

Allow attention mask to be array or Tensor

2576c32

Add JSDoc for tokenizers.js

e4cb674

Throw an error if text_pair is not the same type as text (when sp…

8c605b4

…ecified)

Make NoRepeatNGramLogitsProcessor helper functions part of the class

7aab9aa

Add JSDocs for NoRepeatNGramLogitsProcessor

996b6e1

Add JSDoc for LogitsProcessorList constructor

b102a10

Add JSDoc for BartForSequenceClassification

5f25ed2

Update model constructor JSDocs

060ca3d

Update PreTrainedModel constructor JSDocs

94d7b62

Remove old comment

8d19f15

Allow construction of AutoModel for marian models

216eb54

Remove duplicated comments

9211158

Update JSDocs

6c41fc0

xenova merged commit 0a77a4e into huggingface:main Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typegen #28

Typegen #28

chelouche9 commented Mar 18, 2023

chelouche9 commented Mar 18, 2023

kungfooman Mar 19, 2023

kungfooman Mar 19, 2023 •

edited

Loading

kungfooman Mar 19, 2023 •

edited

Loading

kungfooman Mar 19, 2023 •

edited

Loading

kungfooman Mar 19, 2023

chelouche9 Mar 19, 2023

kungfooman commented Mar 20, 2023

xenova commented Mar 21, 2023

chelouche9 commented Mar 21, 2023

xenova commented Mar 21, 2023

chelouche9 commented Mar 23, 2023 •

edited

Loading

xenova commented Mar 23, 2023

biw commented Mar 23, 2023

xenova commented Mar 23, 2023

kungfooman commented Mar 24, 2023

kungfooman commented Mar 27, 2023

chelouche9 commented Mar 27, 2023

chelouche9 commented Mar 29, 2023

biw commented Mar 29, 2023

xenova commented Mar 29, 2023

xenova commented Mar 29, 2023

xenova commented Apr 4, 2023

chelouche9 commented Apr 4, 2023

xenova commented Apr 4, 2023 •

edited

Loading

chelouche9 commented Apr 4, 2023

kungfooman commented Apr 5, 2023

Typegen #28

Typegen #28

Conversation

chelouche9 commented Mar 18, 2023

chelouche9 commented Mar 18, 2023

kungfooman Mar 19, 2023

Choose a reason for hiding this comment

kungfooman Mar 19, 2023 • edited Loading

Choose a reason for hiding this comment

kungfooman Mar 19, 2023 • edited Loading

Choose a reason for hiding this comment

kungfooman Mar 19, 2023 • edited Loading

Choose a reason for hiding this comment

kungfooman Mar 19, 2023

Choose a reason for hiding this comment

chelouche9 Mar 19, 2023

Choose a reason for hiding this comment

kungfooman commented Mar 20, 2023

xenova commented Mar 21, 2023

chelouche9 commented Mar 21, 2023

xenova commented Mar 21, 2023

chelouche9 commented Mar 23, 2023 • edited Loading

xenova commented Mar 23, 2023

biw commented Mar 23, 2023

xenova commented Mar 23, 2023

kungfooman commented Mar 24, 2023

kungfooman commented Mar 27, 2023

chelouche9 commented Mar 27, 2023

chelouche9 commented Mar 29, 2023

biw commented Mar 29, 2023

xenova commented Mar 29, 2023

xenova commented Mar 29, 2023

xenova commented Apr 4, 2023

chelouche9 commented Apr 4, 2023

xenova commented Apr 4, 2023 • edited Loading

chelouche9 commented Apr 4, 2023

kungfooman commented Apr 5, 2023

kungfooman Mar 19, 2023 •

edited

Loading

kungfooman Mar 19, 2023 •

edited

Loading

kungfooman Mar 19, 2023 •

edited

Loading

chelouche9 commented Mar 23, 2023 •

edited

Loading

xenova commented Apr 4, 2023 •

edited

Loading