Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore error from extending ONNX Tensor #439

Closed

Conversation

kungfooman
Copy link
Contributor

Fixes #437

(it requires a npm run typegen and npm publish)

@@ -36,6 +36,7 @@ const DataTypeMap = new Map([

const ONNXTensor = ONNX.Tensor;

/** @ts-ignore Base constructors must all have the same return type. ts(2510) */
Copy link
Collaborator

@xenova xenova Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @ts-ignore Base constructors must all have the same return type. ts(2510) */
// @ts-ignore

I don't see anywhere in the ts docs where this format (+ comments) is supported?

Also, in v3, We won't need to extend from the base tensor class, and that will be a more permanent solution

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @xenova, it won't work like that - tsc will simply strip a // ... comment and we end up with a Tensor type declaration file that continues to trigger this lib check error, whereas /* ... */ comments are kept and therefore the lib check error disappears (don't ask me who comes up with these decisions 😅)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's interesting... 👀 In that case, it might be worth making the update to not extend from the base tensor class now, since there are many other typing issues which are showing. See here to see what I'm referring to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, you implemented every method that was used from ONNX.Tensor into transformers Tensor class? I like that, it keeps everything a bit closer to this code and easier to understand.

I guess it may break semver/backwards-compatibility a bit if people on v2 still use ONNX.Tensor methods that a maybe not re-implemented yet? So at least for v3 its a good deal.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fortunately, there shouldn't be any backwards-compatibility issues since our Tensor reimplements all the same functionality/properties, and is usable wherever an ORT tensor is allowed.

With onnxruntime-web >=1.17.0, this will be an issue (but that isn't a problem here since we're frozen at 1.14.0.

Would you be interested in making the PR? If not, I can adapt my v3 code and make a standalone PR in the meantime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xenova Thank you for the info! Currently I'm low on time, so I would appreciate if you can do it.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@kungfooman
Copy link
Contributor Author

Also, in v3, We won't need to extend from the base tensor class, and that will be a more permanent solution

Very nice 👍

@xenova
Copy link
Collaborator

xenova commented Dec 12, 2023

Superseded by #451

@xenova xenova closed this Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error TS2510: Base constructors must all have the same return type.
3 participants