microsoft / hummingbird

Hummingbird compiles trained ML models into tensor computation for faster inference.
MIT License
3.32k stars 274 forks source link

Security vuln update from Dependabot alerts #635

Closed ksaur closed 1 year ago

ksaur commented 1 year ago

The summary is that although we tried to fix this right away, we were blocked on onnx dependencies. For now, we patched the pipeline with an older version, and will fix this later.

ksaur commented 1 year ago

Still some ONNX problem, opset conflict?

onnxruntime.capi.onnxruntime_pybind11_state.InvalidArgument: [ONNXRuntimeError] : 2 : INVALID_ARGUMENT : 
Failed to load model with error: D:\a\_work\1\s\onnxruntime\core/graph/model_load_utils.h:57 

onnxruntime::model_load_utils::ValidateOpsetForDomain ONNX Runtime only *guarantees* support for models stamped 
with official released onnx opset versions. Opset 18 is under development and support for this is limited. The operator 
schemas and or other functionality may change before next ONNX release and in this case ONNX Runtime will not 
guarantee backward compatibility. Current official support for domain ai.onnx is till opset 17.

Removing this pin causes us to get onnx-1.13.0 instead of onnx-1.12.0

ksaur commented 1 year ago

See also https://github.com/microsoft/onnxruntime/issues/14060

ksaur commented 1 year ago

This is still failing because it causes us to pull the new onnx. Maybe we pin to onnx-1.12.0 until they fix the opset conflict, and unpin the protobuf?

Working (protobuf<=3.20.1,>=3.20.0):

Requirement already satisfied: onnx in c:\hostedtoolcache\windows\python\3.8.10\x64\lib\site-packages (from onnxconverter-common>=1.6.0->hummingbird-ml==0.4.7) (1.12.0)
---
Requirement already satisfied: onnx>=1.2.1 in c:\hostedtoolcache\windows\python\3.8.10\x64\lib\site-packages (from skl2onnx==1.13.1) (1.12.0)

Failing (protobuf>=3.20.2):

Requirement already satisfied: onnx in c:\hostedtoolcache\windows\python\3.8.10\x64\lib\site-packages (from onnxconverter-common>=1.6.0->hummingbird-ml==0.4.7) (1.13.0)
---
Requirement already satisfied: onnx>=1.2.1 in c:\hostedtoolcache\windows\python\3.8.10\x64\lib\site-packages (from skl2onnx==1.13.1) (1.13.0)

The thing that I don't understand is...otherwise, all onnx packages are equal. Both the working and not working have onnxmltools-1.11.0 onnxruntime-1.13.1 skl2onnx-1.12 and sklearn-onnx at head.

So is it really just the "onnx" package with the 1.13.0 release on Dec 12?

ksaur commented 1 year ago

Oh my goodness. ~Pinning onnx doesn't work because they still can't install for python 3.10 via pip https://github.com/onnx/onnx/issues/4146~ ?

It's somehow related to the way it's being installed, but it's not clear if it's an ordering issue or...?

 × Running setup.py install for onnx did not run successfully.
  │ exit code: 1

Running out of ideas.

ksaur commented 1 year ago

Ok it works if we pin onnx to 1.12 in the pipeline. Ideally this would be in setup.py, but that appears to be not supported.

Is it acceptable to just pin onnx in the pipeline for now? What do you think @interesaaat? Could that break things for users? Having onnx==1.13 failed the following tests only, and i'm assuming onnxml users would already have non-conflicting onnx packages?

tests\test_onnxml_binarizer_converter.py FFF..                         
tests\test_onnxml_imputer_converter.py FF.FF                           
tests\test_onnxml_label_encoder_converter.py sFF.                  
tests\test_onnxml_linear_converter.py FF...                    
tests\test_onnxml_normalizer_converter.py .FFF         
tests\test_onnxml_scaler_converter.py F.FF...FF                       
xadupre commented 1 year ago

This error happens when onnx is released, onnxruntime does not implement the latest opset yet. When the parameter target_opset is not specified, sklearn-onnx takes the latest one coming from onnx. You should add target_opset=17.

ksaur commented 1 year ago

@xadupre If we add target_opset=17 that would be a significant amount of changes across many files, of which we would then need to change back, so that is not an option.
Is there some other work-around, or ways to install things so that we are unblocked?

xadupre commented 1 year ago

If you add pip install onnx==1.12.0 before line https://github.com/microsoft/hummingbird/blob/main/.github/workflows/pythonapp.yml#L47, it should work.

ksaur commented 1 year ago

Yes, that definitely does work, but we were worried that that would break things for users only using setup.py.

Maybe that's ok for now!

ksaur commented 1 year ago

Let's go with this to unblock the vuln fix, and (at least temporarily) hold our release on the releases of onnxruntime and sklearn-onnx. @xadupre please let us know, thanks!

codecov-commenter commented 1 year ago

Codecov Report

Merging #635 (32cd427) into main (c862916) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #635   +/-   ##
=======================================
  Coverage   90.25%   90.25%           
=======================================
  Files          78       78           
  Lines        4545     4545           
  Branches      840      840           
=======================================
  Hits         4102     4102           
  Misses        252      252           
  Partials      191      191           
Flag Coverage Δ
unittests 90.25% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.