js-pytorch: Adding types and fixing bugs on TypeScript implementation

Changes necessary in the develop branch before merging into main:

  • Adding types and fixing bugs of TypeScript implementation.
  • Have zero errors pop up when running npm run check.
  • Removing tsconfig.json allowances before merging.

About this issue

  • Original URL
  • State: closed
  • Created 3 months ago
  • Reactions: 1
  • Comments: 22 (22 by maintainers)

Most upvoted comments

I don’t think I was being specific enough for which I apologise, no technically not errors of TS but using any essentially disables type checking and generally speaking we should try to use any very judiciously, there’s usually a way to narrow this type down. If absolutely necessary you can use an unknown type which is safer from a typing perspective, if we don’t know the type, it can’t be passed around or operated on without typescript throwing an error or being narrowed down further by a type guard.

https://tannerdolby.com/writing/writing-safe-code-with-typescript/ - see here. https://dmitripavlutin.com/typescript-unknown-vs-any/

Also if you haven’t check out the TS-ESLINT docs here https://typescript-eslint.io/rules/no-explicit-any/

  1. If you are happy to have assigned but not used variables, you can change this ESlint rule - this is the flexibility of eslint. I have no preference for this really!
  2. There is a thing called conditional recursive types that can cover deeply nested arrays https://www.basedash.com/blog/guide-to-typescript-recursive-type - have to be a little careful with the nested data though.

Please check in case something with my local repo is wrong. Btw with Husky and lint-staged, this wont be a problem as it will not make the commits/push whilst there are linting/type check errors.

In the current updated code, i did a linting check (as the check script was removed in one of the commits)

They’re 99% about having any types (you can see this in ‘tensor’ class instance properties types in tensor.ts), there was around 230 before, so a good chunk has been done.

Quick work on TS errors! Think the jest syntax looks fine. Good catch on type checking in jest.

I think there’s definitely room for some unit tests in the future and maybe some refactoring of these three tests (not sure whether you’d call these e2e or integration) depending on future tests, extracting the neural net creation and some of the setup for example.

Mmm needs a bit of investigating, but the time taking in test running is mostly due to the last test, having played around with an initial jest setup, the first two tests take around 1-2 seconds which I think is acceptable, transformer test takes around 10seconds to complete. Do you happen to have the times prior to the PR for comparison (I’m using Macbook Pro m3 btw) ?

Couple of things that do stand out for me,

  1. These are most definitely integration tests which will take a longer time than unit tests, i think maybe some carefully placed mocks could help speed things up
  2. The tests are a little erratic as i’ve had a few runs of it and they’ve failed because the loss went above > 0.1 (The assertion is that loss should be less than or equal to 0.1) - should we have a slightly wider criteria for loss ?

@eduardoleao052 i’ll work on utils.ts btw.