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

add BAAI/bge-small-en-v1.5 Optimization #1634

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Conversation

xieofxie
Copy link
Contributor

@xieofxie xieofxie commented Feb 21, 2025

Describe your changes

Checklist before requesting a review

  • Add unit tests for this change.
  • Make sure all tests can pass.
  • Update documents if necessary.
  • Lint and apply fixes to your code by running lintrunner -a
  • Is this a user-facing change? If yes, give a description of this change to be included in the release notes.
  • Is this PR including examples changes? If yes, please remember to update example documentation in a follow-up PR.

(Optional) Issue link

model_output = self.model.run_session(self.session, model_inputs)
model_output = model_output.last_hidden_state.numpy()
# select the last hidden state of the first token (i.e., [CLS]) as the sentence embedding.
return model_output[:, 0, :]

Check failure

Code scanning / CodeQL

Potentially uninitialized local variable Error

Local variable 'model_output' may be used before it is initialized.

The precision will drop when Add or Softmax types of op are quantized, so they are not included.

| Quantized Ops | precision |
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting. Would it be possible to measure the latency for each case so we get what the accuracy vs latency tradeoff is?

If the tradeoff is large maybe we can spend some time investigating the cause of the bad accuracy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Change to draft since I am still working on npu part. The pr is created in cpu machine

"calibrate_method": "MinMax",
"quant_preprocess": true,
"prepare_qnn_config": true,
"op_types_to_quantize": [
Copy link
Contributor

@jambayk jambayk Feb 21, 2025

Choose a reason for hiding this comment

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

I have a dev branch where I introduce an option called op_type_to_exclude which is used to modify op_types_to_quantize and nodes_to_exclude.

"op_types_to_exclude": PassConfigParam(

Looks like it might be useful here too when it gets merged

Copy link
Contributor

Choose a reason for hiding this comment

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

otherwise, we need to know all of the op types present in the model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently use append_first_op_types_to_quantize_list with nodes_to_exclude will do this. Will we also update this logic?

if run_config["append_first_op_types_to_quantize_list"]:

Copy link
Contributor

Choose a reason for hiding this comment

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

honestly, I am not sure why this option was added and if it is used for anything right now.

Not sure if we will touch this option and related logic but I plan to update the logic to be able to use op_types_to_exclude and nodes_to_exclude. The op_types_to_exclude has been very useful for me when I know I don't want to quantize all nodes for an op.

Copy link
Contributor

Choose a reason for hiding this comment

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

also created this PR in ort microsoft/onnxruntime#23779.

@xieofxie xieofxie marked this pull request as draft February 21, 2025 09:06
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