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

Make argmin/armax support identical data types and add int64 support #21641

Merged
merged 8 commits into from
Sep 23, 2024
Merged

Make argmin/armax support identical data types and add int64 support #21641

merged 8 commits into from
Sep 23, 2024

Conversation

cbourjau
Copy link
Contributor

@cbourjau cbourjau commented Aug 6, 2024

Description

This PR adds further int64 kernels for ArgMin and ArgMax. Furthermore, this PR adds int8 and uint8 kernels to ArgMin to keep the coverage between the two equivalent.

Motivation and Context

We ran into the missing kernels in ndonnx. Especially the int64 data type support is important for us since int64 is a common default when working with NumPy/array API code.

@cbourjau cbourjau mentioned this pull request Aug 6, 2024
Merged
@cbourjau
Copy link
Contributor Author

@baijumeswani might you be in a position to review this?

@cbourjau
Copy link
Contributor Author

@pranavsharma Could you maybe give this a review and/or trigger the CI?

@cbourjau
Copy link
Contributor Author

@xadupre Since you are also close by in the git-blame: Might you have time take a look at this PR, please?

@cbourjau
Copy link
Contributor Author

cbourjau commented Sep 2, 2024

@skottmckay might you be in a position to give this a review or to recommend a reviewer?

xadupre
xadupre previously approved these changes Sep 2, 2024
View reviewed changes
@cbourjau
Copy link
Contributor Author

cbourjau commented Sep 4, 2024

Thanks for the approval @xadupre ! Could you also trigger the CI or do you not have the permissions to do so?

@baijumeswani
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline

@baijumeswani
Copy link
Contributor

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows ARM64 QNN CI Pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, Windows x64 QNN CI Pipeline, Linux MIGraphX CI Pipeline, Big Models

@baijumeswani
Copy link
Contributor

/azp run ONNX Runtime React Native CI Pipeline, orttraining-amd-gpu-ci-pipeline, Linux Android Emulator QNN 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 9 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@baijumeswani
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline

@baijumeswani
Copy link
Contributor

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows ARM64 QNN CI Pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, Windows x64 QNN CI Pipeline, Linux MIGraphX CI Pipeline, Big Models

@baijumeswani
Copy link
Contributor

/azp run ONNX Runtime React Native CI Pipeline, orttraining-amd-gpu-ci-pipeline, Linux Android Emulator QNN 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 9 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@baijumeswani
Copy link
Contributor

cbourjau some unit tests are failing. Example failure:

1: [ RUN      ] ReductionOpTest.ArgMin_uint8
1: 2024-09-16 20:44:31.5490080 [W:onnxruntime:Default, tensorrt_execution_provider.cc:2586 onnxruntime::TensorrtExecutionProvider::GetCapability] [TensorRT EP] No graph will run on TensorRT execution provider
1: D:\a\_work\1\s\onnxruntime\test\providers\base_tester.cc(317): error: Expected equality of these values:
1:   expect_result
1:     Which is: 4-byte object <00-00 00-00>
1:   ExpectResult::kExpectFailure
1:     Which is: 4-byte object <01-00 00-00>
1: Initialize failed but expected success: Could not find an implementation for ArgMin(1) node with name 'node1'
1: Google Test trace:
1: D:\a\_work\1\s\onnxruntime\test\providers\base_tester.cc(827): registered execution providers: TensorrtExecutionProvider
1: Stack trace:
1:   00007FF7B29C80E9: (unknown)
1:   00007FF7B29CA3F4: (unknown)
1:   00007FF7B29C8FAE: (unknown)
1:   00007FF7B29C90E8: (unknown)
1:   00007FF7B2C3D449: (unknown)
1:   00007FF7B38D7992: (unknown)
1:   00007FF7B38D7856: (unknown)
1:   00007FF7B38FCED2: (unknown)
1:   00007FF7B38FCFF8: (unknown)
1:   00007FF7B38FD1A1: (unknown)
1: ... Google Test internal frames ...
1: 
1: [  FAILED  ] ReductionOpTest.ArgMin_uint8 (9 ms)

Could you please address the failures?

@baijumeswani
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline

@baijumeswani
Copy link
Contributor

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows ARM64 QNN CI Pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, Windows x64 QNN CI Pipeline, Linux MIGraphX CI Pipeline, Big Models

@baijumeswani
Copy link
Contributor

/azp run ONNX Runtime React Native CI Pipeline, orttraining-amd-gpu-ci-pipeline, Linux Android Emulator QNN 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 5 pipeline(s).

@baijumeswani
Copy link
Contributor

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows ARM64 QNN CI Pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, Windows x64 QNN CI Pipeline, Linux MIGraphX CI Pipeline, Big Models

@baijumeswani
Copy link
Contributor

/azp run ONNX Runtime React Native CI Pipeline, orttraining-amd-gpu-ci-pipeline, Linux Android Emulator QNN 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 8 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@cbourjau
Copy link
Contributor Author

cbourjau commented Sep 18, 2024 •
edited
Loading

Apologies for the extra friction, @baijumeswani ! I didn't register the new kernels with the older opsets. Things should be fixed now. The tests correctly run through locally on osx-arm64.

Edit: Had to fix one more linting error; one of the OSX tests seem to fail due to a network error.

@baijumeswani
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline

@baijumeswani
Copy link
Contributor

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows ARM64 QNN CI Pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, Windows x64 QNN CI Pipeline, Linux MIGraphX CI Pipeline, Big Models

@baijumeswani
Copy link
Contributor

/azp run ONNX Runtime React Native CI Pipeline, orttraining-amd-gpu-ci-pipeline, Linux Android Emulator QNN CI Pipeline, Windows GPU CUDA CI Pipeline, Windows GPU DML CI Pipeline, Windows GPU Doc Gen CI Pipeline

@baijumeswani baijumeswani added the merge on green This PR can be merged when the CI is green. Please merge. label Sep 18, 2024
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

baijumeswani
baijumeswani previously approved these changes Sep 18, 2024
View reviewed changes
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@cbourjau
Copy link
Contributor Author

I am now skipping the tests for int8 inputs on TensorRT. Unfortunately, I don't have the hardware to test it. Apologies for the additional iterations!

@baijumeswani
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline

@baijumeswani
Copy link
Contributor

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows ARM64 QNN CI Pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, Windows x64 QNN CI Pipeline, Linux MIGraphX CI Pipeline, Big Models

@baijumeswani
Copy link
Contributor

/azp run ONNX Runtime React Native CI Pipeline, orttraining-amd-gpu-ci-pipeline, Linux Android Emulator QNN 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 8 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@baijumeswani
Copy link
Contributor

I am now skipping the tests for int8 inputs on TensorRT. Unfortunately, I don't have the hardware to test it. Apologies for the additional iterations!

@jywu-msft is it ok to skip these tests on tensorrt?

@jywu-msft
Copy link
Member

I am now skipping the tests for int8 inputs on TensorRT. Unfortunately, I don't have the hardware to test it. Apologies for the additional iterations!

@jywu-msft is it ok to skip these tests on tensorrt?

OK. we will revisit the int8 test.

@baijumeswani baijumeswani merged commit 1a84f53 into microsoft:main Sep 23, 2024
78 checks passed
@baijumeswani
Copy link
Contributor

@cbourjau thank you for the contribution. Apologies for the delay in reviewing and merging. :)

@cbourjau cbourjau deleted the more-argmin-argmax-kernels branch September 26, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baijumeswani baijumeswani baijumeswani approved these changes

@xadupre xadupre xadupre left review comments

Assignees
No one assigned
Labels
merge on green This PR can be merged when the CI is green. Please merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cbourjau @baijumeswani @jywu-msft @xadupre