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

[demo] Add binary & scalar embedding quantization #683

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

Conversation

jonathanpv
Copy link
Contributor

This PR addressed the below issue, I've also added an example project showcasing the binary embeddings and using hamming distance function as @xenova gave an example of earlier in the below discussion.

  • Closes [feat] Add binary & scalar embedding quantization support to Transformers.js #681

  • Let me know if you'd like any changes to this, perhaps we can make another options object in javascript like a QuantizationOptions so it's obvious which parameters are grouped together logically

  • I also added a texts === undefined check since I ran into an issue earlier where I was accidentally not passing in any texts and was met with an unfriendly error.

For some reason though I don't get the same similarity numbers as Xenova did on his example but the numbers are at least similar

Hopefully the UI can help debug some of my code if there needs to be changes.

Here's an image of it:

image

@xenova
Copy link
Owner

xenova commented Apr 9, 2024

Thanks for the PR! Would you mind separating out the example app from the core quantization logic? We can possibly integrate the example app at a later stage, but I think a good starting point for now is to just provide a bit of sample code.

@jonathanpv
Copy link
Contributor Author

Thanks for the PR! Would you mind separating out the example app from the core quantization logic? We can possibly integrate the example app at a later stage, but I think a good starting point for now is to just provide a bit of sample code.

Ah, yep forgot to this a while back, here it is! Let me know of any changes you'd like

@xenova
Copy link
Owner

xenova commented Apr 10, 2024

Feel free to rebase off main (which includes your other PR: #681) 🤗

@xenova xenova changed the title Add binary & scalar embedding quantization support to Transformers.js [demo] Add binary & scalar embedding quantization Apr 10, 2024
@do-me
Copy link
Contributor

do-me commented Apr 11, 2024

This is awesome! Maybe a little more explanation in the docs would be good. Like what's the (performance) difference between binary and ubinary here and what's the default right now (I guess none)? 

@xenova I'm a little confused that there is no option normal / none. Or is the idea that if you do not pass the parameter, none of the two options is applied and instead it's simply normal (float) precision?

@jonathanpv
Copy link
Contributor Author

jonathanpv commented Apr 12, 2024

This is awesome! Maybe a little more explanation in the docs would be good. Like what's the (performance) difference between binary and ubinary here and what's the default right now (I guess none)? 

@xenova I'm a little confused that there is no option normal / none. Or is the idea that if you do not pass the parameter, none of the two options is applied and instead it's simply normal (float) precision?

yep idea is to not pass the param if you don't plan on using it, its set as a default param / named param

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters

I can improve the docs for this for sure

@do-me
Copy link
Contributor

do-me commented Apr 12, 2024

Thanks for your reply. Found the related line here:

    if (!['binary', 'ubinary'].includes(precision)) {
        throw new Error("The precision must be either 'binary' or 'ubinary'");
    }

I understand the idea of the paradigm. However, it causes silightly more work for downstream applications. In my case, I wanted to create a dropdown menu for UI users with 3 values:

  • binary
  • ubinary
  • normal/full/float (however you want to call it)

I would always pass the precision param as it's the easiest to do in client-side JS. However, due to the current logic I need to add more complexity, checking whether a user selected normal mode and not pass the precision param in the function call in case.
Is there any advantage in not including the normal mode or would you mind adding it?

@jonathanpv
Copy link
Contributor Author

Thanks for your reply. Found the related line here:

    if (!['binary', 'ubinary'].includes(precision)) {
        throw new Error("The precision must be either 'binary' or 'ubinary'");
    }

I understand the idea of the paradigm. However, it causes silightly more work for downstream applications. In my case, I wanted to create a dropdown menu for UI users with 3 values:

  • binary
  • ubinary
  • normal/full/float (however you want to call it)

I would always pass the precision param as it's the easiest to do in client-side JS. However, due to the current logic I need to add more complexity, checking whether a user selected normal mode and not pass the precision param in the function call in case. Is there any advantage in not including the normal mode or would you mind adding it?

Hmm good call out, the reason I added quantize was to make it explicit you'd be quantizing. It technically does add more complexity from a UI / code perspective

Trying to think if quantize yes/no would add another field besides precision, if so then quantizeOptions would need to be created, can't think of anything at the moment.

technically we could flatten out the param to just be 'precision'

to be super clear in your suggestion:

your current scenario makes your UI render the dropdown conditionally based on if the 'quantize embedding' is set to true eg

checkbox:
checkbox label: quantize embedding?

dropdown:
if checkbox yes, then render precision options


vs just having precision his ui would be the three dropdown

precision dropdown:
ubinary
binary
float32 (full)

So basically right now the cases are:

cases now: 4


quantize: false
precision: binary (this is ignored)

===

quantize: false
precision: ubinary (this is ignored)

===

quantize: true
precision: binary

===

quantize: true
precision: ubinary

cases if flattend: 3

precision: binary
===
precision: ubinary
===
precision: float32 / full ?

@do-me
Copy link
Contributor

do-me commented Apr 12, 2024

UI

your current scenario makes your UI render the dropdown conditionally based on if the 'quantize embedding' is set to true eg

Exactly. One dropdown with all 3 flattened options would be much easier to use than conditionally changing the UI and JSON I need to pass to the function.

--

By the way: there are now two quantize/d options in transformers.js, one for loading a quantized model in pipeline and one for your precision settings.

@Th3G33k
Copy link

Th3G33k commented Apr 14, 2024

+1 with do-me

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.

[feat] Add binary & scalar embedding quantization support to Transformers.js
4 participants