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

documentation update for splade++ huggingface -> onnx #2556

Merged
merged 7 commits into from
Aug 2, 2024

Conversation

AndreSlavescu
Copy link
Contributor

Updated documentation for up to date e2e splade conversion to onnx (huggingface -> onnx)

@lintool lintool requested a review from cadurosar July 31, 2024 19:15
@lintool
Copy link
Member

lintool commented Jul 31, 2024

@cadurosar can you please take a look at this?

Copy link

@carlos-lassance carlos-lassance left a comment

Choose a reason for hiding this comment

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

This looks very good, just a few comments

docs/onnx-conversion.md Show resolved Hide resolved

---

Another important component is being able to specify the dimensions of the input and output tensors. This is achieved by the following code:

Choose a reason for hiding this comment

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

I would say more that we are giving the dynamic axis names here and not specifying the dimensions

docs/onnx-conversion.md Outdated Show resolved Hide resolved
cd src/main/python/onnx
# Now run the script
python3 run_onnx_model_inference.py --model_path models/splade-cocondenser-ensembledistil-optimized.onnx \
--model_name naver/splade-cocondenser-ensembledistil

Choose a reason for hiding this comment

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

maybe an explanation here that we get the model nome to use the tokenizer?
Another possibility is using the model name to load the hf version and then do a comparison between both to check that things are ok (always reassuring to someone using this for the first time)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I can add more details for this

@lintool
Copy link
Member

lintool commented Aug 1, 2024

@AndreSlavescu Can you also add a blurb on how to test the newly generated SPLADE++ ED model e2e? E.g., point to the repro, "swap in" the new model in ~/.cache/, etc.

@lintool lintool merged commit 1073485 into castorini:master Aug 2, 2024
1 check passed
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.

3 participants