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.

Sign up for GitHub

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

Jump to bottom

Add support for ai.onnx.ml.TreeEnsemble.v5 #21851

Closed
wants to merge 19 commits into from
Closed

Add support for ai.onnx.ml.TreeEnsemble.v5 #21851

wants to merge 19 commits into from

Conversation

bili2002
Copy link
Contributor

@bili2002 bili2002 commented Aug 25, 2024 •
edited
Loading

Description

This PR adds support for the TreeEnsemble operator added in version 5 of the ai.onnx.ml opset. The solution maps the values of the attributes of the v5 operator into attributes of the already supported version 3. Whereas, the membership_values are unrolled into chains of BRANCH_EQ nodes.

Note that the changes made in #21222 may roll up these BRANCH_EQ chains into a single node again to retain efficient support for categorical features.

Ultimately, it may be desirable to have an implementation that directly uses the v5 attributes. However, mapping the v5 attributes into the v3 layout appears to be an easier first step to gaining initial support for the v5 operator.

Motivation and Context

Solves onnx/onnx#5874.

cbourjau
cbourjau reviewed Aug 26, 2024
View reviewed changes
Copy link
Contributor

@cbourjau cbourjau left a comment

Choose a reason for hiding this comment

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

Thanks for taking on this complex task! I have a few minor comments at this point.

onnxruntime/core/providers/cpu/ml/tree_ensemble.h Outdated Show resolved Hide resolved
onnxruntime/core/providers/cpu/ml/tree_ensemble.h Outdated Show resolved Hide resolved
@bili2002 bili2002 marked this pull request as ready for review August 28, 2024 16:50
@cbourjau
Copy link
Contributor

@xadupre could you maybe take a look at this?

@xadupre
Copy link
Member

xadupre commented Sep 18, 2024

Can you look into the failing build? The compilation fails on Windows because a warning is treated as an error (usually an implicit cast to change into a static_cast<...>(...).

@bili2002 bili2002 requested a review from xadupre September 20, 2024 08:19
xadupre
xadupre previously approved these changes Sep 20, 2024
View reviewed changes
@xadupre
Copy link
Member

xadupre commented Sep 20, 2024

/azp run inux CPU CI Pipeline,Windows CPU CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bili2002 bili2002 requested a review from xadupre September 23, 2024 00:00
xadupre
xadupre previously approved these changes Sep 23, 2024
View reviewed changes
@xadupre
Copy link
Member

xadupre commented Sep 23, 2024

/azp run Big Models,Linux Android Emulator QNN CI Pipeline,Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@xadupre
Copy link
Member

xadupre commented Sep 23, 2024

/azp run MacOS CI Pipeline,ONNX Runtime Web CI Pipeline,Windows ARM64 QNN CI Pipeline,Windows CPU CI Pipeline,Windows GPU CUDA CI Pipeline,Windows GPU DML CI Pipeline,Windows GPU Doc Gen CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 7 pipeline(s).

@xadupre
Copy link
Member

xadupre commented Sep 23, 2024

/azp run Windows GPU TensorRT CI Pipeline,Windows x64 QNN CI Pipeline,onnxruntime-binary-size-checks-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@bili2002 bili2002 requested a review from xadupre September 24, 2024 22:04
@bili2002
Copy link
Contributor Author

bili2002 commented Oct 3, 2024

@xadupre, could you start the CI again?

@xadupre
Copy link
Member

xadupre commented Oct 4, 2024

/azp run inux CPU CI Pipeline,Windows CPU CI Pipeline

@xadupre
Copy link
Member

xadupre commented Oct 4, 2024

/azp run Big Models,Linux Android Emulator QNN CI Pipeline,Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@xadupre
Copy link
Member

xadupre commented Oct 4, 2024

/azp run MacOS CI Pipeline,ONNX Runtime Web CI Pipeline,Windows ARM64 QNN CI Pipeline,Windows CPU CI Pipeline,Windows GPU CUDA CI Pipeline,Windows GPU DML CI Pipeline,Windows GPU Doc Gen CI Pipeline

@xadupre
Copy link
Member

xadupre commented Oct 4, 2024

/azp run Windows GPU TensorRT CI Pipeline,Windows x64 QNN CI Pipeline,onnxruntime-binary-size-checks-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 7 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@xadupre xadupre mentioned this pull request Oct 7, 2024
Merged
@xadupre
Copy link
Member

xadupre commented Nov 5, 2024

Moved to #22333.

@xadupre xadupre closed this Nov 5, 2024
xadupre added a commit that referenced this pull request Nov 22, 2024
@xadupre @bili2002 @cbourjau
Implementation of TreeEnsemble ai.onnx.ml==5 (#22333)
a2ba3cb 
### Description
Merges PR #21851, #21222.

Implements TreeEnsemble from ai.onnx.ml==5 (CPU).

---------

Co-authored-by: Bilyana Indzheva <[email protected]>
Co-authored-by: Bilyana Indzheva <[email protected]>
Co-authored-by: Christian Bourjau <[email protected]>
guschmue pushed a commit that referenced this pull request Dec 2, 2024
@xadupre @bili2002
@cbourjau @guschmue
Implementation of TreeEnsemble ai.onnx.ml==5 (#22333)
486fb01 
### Description
Merges PR #21851, #21222.

Implements TreeEnsemble from ai.onnx.ml==5 (CPU).

---------

Co-authored-by: Bilyana Indzheva <[email protected]>
Co-authored-by: Bilyana Indzheva <[email protected]>
Co-authored-by: Christian Bourjau <[email protected]>
ankitm3k pushed a commit to intel/onnxruntime that referenced this pull request Dec 11, 2024
@xadupre @bili2002
@cbourjau @ankitm3k
Implementation of TreeEnsemble ai.onnx.ml==5 (microsoft#22333)
23f88f3 
### Description
Merges PR microsoft#21851, microsoft#21222.

Implements TreeEnsemble from ai.onnx.ml==5 (CPU).

---------

Co-authored-by: Bilyana Indzheva <[email protected]>
Co-authored-by: Bilyana Indzheva <[email protected]>
Co-authored-by: Christian Bourjau <[email protected]>
ankitm3k pushed a commit to intel/onnxruntime that referenced this pull request Dec 11, 2024
@xadupre @bili2002
@cbourjau @ankitm3k
Implementation of TreeEnsemble ai.onnx.ml==5 (microsoft#22333)
0346a06 
### Description
Merges PR microsoft#21851, microsoft#21222.

Implements TreeEnsemble from ai.onnx.ml==5 (CPU).

---------

Co-authored-by: Bilyana Indzheva <[email protected]>
Co-authored-by: Bilyana Indzheva <[email protected]>
Co-authored-by: Christian Bourjau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xadupre xadupre xadupre left review comments

@cbourjau cbourjau cbourjau left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bili2002 @cbourjau @xadupre