-
Notifications
You must be signed in to change notification settings - Fork 254
Remove coef_ requirement from ONNX exporter. #361
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
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
tomaarsen
left a comment
There was a problem hiding this 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.
|
Sorry, didn't realize that pushing to my own branch would update here. First PR... |
|
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. :) |
|
Don't know why the quality check isn't passing. I ran |
Black 23.1.0 & isort 5.12.0
|
I ran the |
|
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. |
|
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... |
|
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. |
|
I found the issue I think. It's hummingbird_ml-0.4.9. Downgrading to 0.4.8 makes the tests pass for me. |
|
I'll merge main into this so we can rerun the tests :) |
|
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 thank you so much for holding my hand through my first ever PR, sorry I neglected it for months haha |
|
Haha no worries, it's also my bad for failing to review this PR for weeks or months. No big deal all around |
This PR is my attempt to solve the todo to make the ONNX exporter work with any sklearn
model_headthat doesn't have thecoef_attribute, which was deprecated forOneVsRestClassifierin sklearn 1.1.I just use
n_features_in_for the shape and for the dtype I check if it has acoef_attribute and if not I take thecoef_attribute of the first element ofestimators_.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!