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

Check TypeScript errors during build #1081

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ocavue
Copy link
Contributor

@ocavue ocavue commented Dec 7, 2024

npm run typegen (and npm run build) now will check TypeScript errors.

I migrated jsconfig.json to tsconfig.json because tsc --build can only recognize tsconfig.json. tsc --build is faster than tsc because tsc --build has cache, so it's better to use tsc --build.


Since it reveals all typing errors now, I need to address all of them to make the build pass.

  • Most of them are just missing types in configs, like the one below:

    this.config.image_token_index;
                ~~~~~~~~~~~~~~~~~
                ^ Property 'image_token_index' does not exist on type 'PretrainedConfig'.

    For those errors, I simply add @ts-expect-error comment to let TypeScript ignore it. I use @ts-expect-error instead of @ts-ignore because we can remove those comments once we add missing types in the future.

  • I fixed some other types issues that I can fix.

  • One thing I noticed is that MgpstrProcessor.batch_decode has different shape compared to the method in the parent class. This might not be a good idea.

@ocavue ocavue marked this pull request as ready for review December 7, 2024 23:37
@xenova
Copy link
Collaborator

xenova commented Dec 8, 2024

Thanks for this! I'll do a full review soon, but just to address your third point: https://github.com/huggingface/transformers/blob/c8c8dffbe45ebef0a8dba4a51024e5e5e498596b/src/transformers/models/mgp_str/processing_mgp_str.py#L102 has a similar issue where MgpstrProcessor has a non-standard batch_decode function. This is because the model returns three tensors that all need to be used in the decode method. Not too sure how to make it fit the parent class type though.

@ocavue
Copy link
Contributor Author

ocavue commented Dec 8, 2024

I would consider this as an issue in the original Python transformers lib. At least its docstring is wrong.

I don't know any context about MgpstrProcessor, but maybe renaming this method to something else (e.g. just decode()) could improve it.


On the JS lib side, it's okey for me to ignore it for now. Nobody except TypeScript complains so far.

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.

2 participants