Skip to content

Conversation

@bogedy
Copy link
Contributor

@bogedy bogedy commented Apr 18, 2023

This PR is my attempt to solve the todo to make the ONNX exporter work with any sklearn model_head that doesn't have the coef_ attribute, which was deprecated for OneVsRestClassifier in sklearn 1.1.

I just use n_features_in_ for the shape and for the dtype I check if it has a coef_ attribute and if not I take the coef_ attribute of the first element of estimators_.

I also expanded the ONNX test file script.

This is my first PR on anything, my apologies if I misunderstood the scope of the problem or did anything else wrong!

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

Copy link
Member

@tomaarsen tomaarsen left a comment

Choose a reason for hiding this comment

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

This is looking good! Especially for a first PR. However, I do have some notes/questions.
First of all, could you run make style to run some formatting rules, and then push those changes? This will satisfy the currently failing quality checks.

@tomaarsen tomaarsen added enhancement New feature or request exporting Regarding exporting SetFit models (e.g. ONNX) labels Apr 19, 2023
@bogedy
Copy link
Contributor Author

bogedy commented Apr 19, 2023

Sorry, didn't realize that pushing to my own branch would update here. First PR...

@tomaarsen
Copy link
Member

No worries! It'll all get squashed into one neat commit when I merge this, so you don't have to worry too much about keeping the PR "tidy" with few commits, etc. :)

@bogedy
Copy link
Contributor Author

bogedy commented Apr 21, 2023

Don't know why the quality check isn't passing. I ran make style && make quality repeatedly and double checked that I had the right versions installed. I followed all the dev instructions. Passes on my machine.

Black 23.1.0 & isort 5.12.0
@tomaarsen
Copy link
Member

@bogedy

I ran the style locally, as I think my local black & isort versions align with the ones that the CI uses.
Do you think the PR is ready?

@bogedy
Copy link
Contributor Author

bogedy commented May 22, 2023

Hi! I'm sorry I fell off this, got really busy. The PR is ready. I thought maybe I should upload a new model to hugging face since the lewtun my awesome model appears to be bugged with ONNX, but I suppose that can be taken care of later.

@bogedy
Copy link
Contributor Author

bogedy commented Jul 13, 2023

I thought this needed a lot more and neglected it but looking at the error in the unit test it was just random chance?

502 Server Error: Bad Gateway for url: https://huggingface.co/lewtun/my-awesome-setfit-model/resolve/a3c0fe9302019e63581ad34ab25e6b6c5b4aab5d/pytorch_model.bin

That URL works for me...

@tomaarsen
Copy link
Member

I get similar test failures here: #397. I haven't figured out how these started happening, as the Openvino version hasn't changed to my knowledge.

@bogedy
Copy link
Contributor Author

bogedy commented Jul 25, 2023

I found the issue I think. It's hummingbird_ml-0.4.9. Downgrading to 0.4.8 makes the tests pass for me.

@bogedy bogedy marked this pull request as ready for review July 25, 2023 20:39
@tomaarsen
Copy link
Member

I'll merge main into this so we can rerun the tests :)

@tomaarsen
Copy link
Member

This is looking good I think! Thanks, and apologies for the delays and random CI issues. Oh, and thanks for fixing the latest CI issues for me!

@tomaarsen tomaarsen merged commit a0b69b4 into huggingface:main Jul 25, 2023
@bogedy
Copy link
Contributor Author

bogedy commented Jul 25, 2023

@tomaarsen thank you so much for holding my hand through my first ever PR, sorry I neglected it for months haha

@bogedy bogedy deleted the onnx_change branch July 26, 2023 15:23
@tomaarsen
Copy link
Member

Haha no worries, it's also my bad for failing to review this PR for weeks or months. No big deal all around

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request exporting Regarding exporting SetFit models (e.g. ONNX)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants