[CoreML] Adapt to `MLMultiArray.dataPointer` deprecation #17726

NickLucche · 2023-09-27T21:42:54Z

Description

This PR addresses #17652.
The deprecated MLMultiArray.dataPointer is replaced with .getBytesWithHandler, as suggested by the docs.
For now, I am only checking that the output MLMultiArray is contiguous, returning unsupported operation when that is not the case.
I think this is already better than what we have right now, so we can block unsafe calls to .dataPointer (if any..).

I would be happy to implement the handling of the non-contiguous case (replacing memcpy for such cases) as suggested by @edgchen1, but I am not sure how to reproduce that case to add a corresponding unit-test.
Would we have to define a custom MLCustomLayer to get a non-contiguous output from a model..?

Motivation and Context

Fix #17652.

edgchen1

thanks for the PR! had a few comments.

onnxruntime/core/providers/coreml/model/model.mm

edgchen1 · 2023-09-29T22:20:53Z

I would be happy to implement the handling of the non-contiguous case (replacing memcpy for such cases) as suggested by @edgchen1, but I am not sure how to reproduce that case to add a corresponding unit-test.
Would we have to define a custom MLCustomLayer to get a non-contiguous output from a model..?

I think it's fine to only handle the contiguous case for now. Not clear to me how to test that either.

edgchen1 · 2023-09-29T22:21:31Z

/azp run MacOS CI Pipeline

azure-pipelines · 2023-09-29T22:21:43Z

Azure Pipelines successfully started running 1 pipeline(s).

NickLucche · 2023-09-30T12:33:13Z

Thanks a lot for reviewing the PR!

NickLucche · 2023-09-30T12:33:35Z

@microsoft-github-policy-service agree

onnxruntime/core/providers/coreml/model/model.mm

edgchen1 · 2023-10-02T16:32:29Z

/azp run MacOS CI Pipeline

azure-pipelines · 2023-10-02T16:32:43Z

Azure Pipelines successfully started running 1 pipeline(s).

edgchen1 · 2023-10-03T17:41:36Z

/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

edgchen1 · 2023-10-03T17:42:05Z

/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, ONNX Runtime React Native CI Pipeline, Windows x64 QNN CI Pipeline

azure-pipelines · 2023-10-03T17:42:19Z

Azure Pipelines successfully started running 9 pipeline(s).

azure-pipelines · 2023-10-03T17:42:48Z

Azure Pipelines successfully started running 9 pipeline(s).

edgchen1 · 2023-10-04T00:18:24Z

there are some errors from the MacOS CI build:

https://dev.azure.com/onnxruntime/onnxruntime/_build/results?buildId=1156472&view=logs&j=07136875-e4b3-5a15-ff50-026d13f57d01&t=ff34eec9-4cff-535b-345b-da5da417f512

/Users/runner/work/1/s/onnxruntime/core/providers/coreml/model/model.mm:177:21: error: comparison of integers of different signs: 'int' and 'NSUInteger' (aka 'unsigned long') [-Werror,-Wsign-compare]
  for (int i = 1; i < [shape count]; i++) batch_elems *= [shape[i] longLongValue];
                  ~ ^ ~~~~~~~~~~~~~
/Users/runner/work/1/s/onnxruntime/core/providers/coreml/model/model.mm:339:15: error: 'getBytesWithHandler:' is only available on macOS 12.3 or newer [-Werror,-Wunguarded-availability-new]
        [data getBytesWithHandler:^(const void* bytes, NSInteger size) {
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Applications/Xcode_14.3.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk/System/Library/Frameworks/CoreML.framework/Headers/MLMultiArray.h:125:1: note: 'getBytesWithHandler:' has been marked as being introduced in macOS 12.3 here, but the deployment target is macOS 10.14.0
- (void)getBytesWithHandler:(void (NS_NOESCAPE ^)(const void *bytes, NSInteger size))handler API_AVAILABLE(macos(12.3), ios(15.4), watchos(8.5), tvos(15.4)) NS_REFINED_FOR_SWIFT;
^
/Users/runner/work/1/s/onnxruntime/core/providers/coreml/model/model.mm:339:15: note: enclose 'getBytesWithHandler:' in an @available check to silence this warning
        [data getBytesWithHandler:^(const void* bytes, NSInteger size) {
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/1/s/onnxruntime/core/providers/coreml/model/model.mm:352:50: error: comparison of integers of different signs: 'int64_t' (aka 'long long') and 'const unsigned long' [-Werror,-Wsign-compare]
            ORT_RETURN_IF_NOT(coreml_buffer_size == output_data_byte_size,
                              ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/1/s/include/onnxruntime/core/common/common.h:202:19: note: expanded from macro 'ORT_RETURN_IF_NOT'
  ORT_RETURN_IF(!(condition), __VA_ARGS__)
                  ^~~~~~~~~
/Users/runner/work/1/s/include/onnxruntime/core/common/common.h:193:9: note: expanded from macro 'ORT_RETURN_IF'
    if (condition) {                                                                                           \
        ^~~~~~~~~
/Users/runner/work/1/s/onnxruntime/core/providers/coreml/model/model.mm:359:50: error: comparison of integers of different signs: 'int64_t' (aka 'long long') and 'const unsigned long' [-Werror,-Wsign-compare]
            ORT_RETURN_IF_NOT(coreml_buffer_size == output_data_byte_size,
                              ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/1/s/include/onnxruntime/core/common/common.h:202:19: note: expanded from macro 'ORT_RETURN_IF_NOT'
  ORT_RETURN_IF(!(condition), __VA_ARGS__)
                  ^~~~~~~~~
/Users/runner/work/1/s/include/onnxruntime/core/common/common.h:193:9: note: expanded from macro 'ORT_RETURN_IF'
    if (condition) {                                                                                           \
        ^~~~~~~~~
/Users/runner/work/1/s/onnxruntime/core/providers/coreml/model/model.mm:370:50: error: comparison of integers of different signs: 'int64_t' (aka 'long long') and 'unsigned long' [-Werror,-Wsign-compare]
            ORT_RETURN_IF_NOT(coreml_buffer_size == num_elements * sizeof(int32_t),
                              ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/1/s/include/onnxruntime/core/common/common.h:202:19: note: expanded from macro 'ORT_RETURN_IF_NOT'
  ORT_RETURN_IF(!(condition), __VA_ARGS__)
                  ^~~~~~~~~
/Users/runner/work/1/s/include/onnxruntime/core/common/common.h:193:9: note: expanded from macro 'ORT_RETURN_IF'
    if (condition) {                                                                                           \
        ^~~~~~~~~
5 errors generated.

NickLucche · 2023-10-04T09:01:44Z

I run the ci_build pipeline on my side, looks ok.
The only error to be addressed is this one:

'getBytesWithHandler:' is only available on macOS 12.3 or newer
note: 'getBytesWithHandler:' has been marked as being introduced in macOS 12.3 here, but the deployment target is macOS 10.14.0

What's your suggestion here? Do we check the target version and use the deprecated method for older builds or..?

I dug a bit into the CI too but I am not sure if the macos version is left to azure availability to decide atm (couldn't spot a macos version, just MacosArch). Could be good to additionally build on latest macOS anyway (I take that back if it's already being done).

edgchen1 · 2023-10-04T16:51:34Z

I run the ci_build pipeline on my side, looks ok. The only error to be addressed is this one:
'getBytesWithHandler:' is only available on macOS 12.3 or newer
note: 'getBytesWithHandler:' has been marked as being introduced in macOS 12.3 here, but the deployment target is macOS 10.14.0
What's your suggestion here? Do we check the target version and use the deprecated method for older builds or..?

I dug a bit into the CI too but I am not sure if the macos version is left to azure availability to decide atm (couldn't spot a macos version, just MacosArch). Could be good to additionally build on latest macOS anyway (I take that back if it's already being done).

I think the deployment target is specified here:

onnxruntime/tools/ci_build/github/azure-pipelines/templates/mac-cpu-packing-jobs.yml

Line 34 in 1bc1157

MACOSX_DEPLOYMENT_TARGET: '10.14'

If we need to support older deployment targets, we can do something like you said, conditionally use the deprecated method.

We are building on a more recent MacOS version, but MACOSX_DEPLOYMENT_TARGET specifies the minimum version we support.

@snnn @pranavsharma @skottmckay What's the minimum MacOS version that we need to support?

NickLucche · 2023-10-26T08:14:50Z

Hey, any news on the best strategy here? :)

edgchen1 · 2023-10-30T18:54:25Z

Hey, any news on the best strategy here? :)

Sorry for the delay. Following up with the team about it.

edgchen1 · 2023-11-15T18:41:57Z

/azp run Linux MIGraphX CI Pipeline, orttraining-amd-gpu-ci-pipeline

azure-pipelines · 2023-11-15T18:42:10Z

Azure Pipelines successfully started running 2 pipeline(s).

azure-pipelines · 2023-11-15T18:42:14Z

Azure Pipelines successfully started running 9 pipeline(s).

azure-pipelines · 2023-11-15T18:42:20Z

Azure Pipelines successfully started running 9 pipeline(s).

edgchen1 · 2023-11-17T18:12:05Z

/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

edgchen1 · 2023-11-17T18:12:15Z

/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, ONNX Runtime React Native CI Pipeline, Windows x64 QNN CI Pipeline

edgchen1 · 2023-11-17T18:12:23Z

/azp run Linux MIGraphX CI Pipeline, orttraining-amd-gpu-ci-pipeline

azure-pipelines · 2023-11-17T18:12:35Z

Azure Pipelines successfully started running 2 pipeline(s).

azure-pipelines · 2023-11-17T18:12:49Z

Azure Pipelines successfully started running 9 pipeline(s).

azure-pipelines · 2023-11-17T18:12:56Z

Azure Pipelines successfully started running 9 pipeline(s).

onnxruntime/core/providers/coreml/model/model.mm

Co-authored-by: Edward Chen <[email protected]>

edgchen1 · 2023-11-17T19:56:50Z

/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

edgchen1 · 2023-11-17T19:56:57Z

/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, ONNX Runtime React Native CI Pipeline, Windows x64 QNN CI Pipeline

edgchen1 · 2023-11-17T19:57:14Z

/azp run Linux MIGraphX CI Pipeline, orttraining-amd-gpu-ci-pipeline

azure-pipelines · 2023-11-17T19:57:26Z

Azure Pipelines successfully started running 2 pipeline(s).

azure-pipelines · 2023-11-17T19:57:28Z

Azure Pipelines successfully started running 9 pipeline(s).

azure-pipelines · 2023-11-17T19:57:33Z

Azure Pipelines successfully started running 9 pipeline(s).

edgchen1 · 2023-11-18T02:00:47Z

@NickLucche thanks for your contribution and your patience with this PR. glad we got it merged!

NickLucche · 2023-11-18T13:49:00Z

Thank you for your help and patience! @edgchen1

@edgchen1

…7726) ### Description This PR addresses microsoft#17652. The deprecated `MLMultiArray.dataPointer` is replaced with `.getBytesWithHandler`, as suggested by the docs. For now, I am only checking that the output `MLMultiArray` is contiguous, returning unsupported operation when that is not the case. I think this is already better than what we have right now, so we can block unsafe calls to `.dataPointer` (if any..). I would be happy to implement the handling of the non-contiguous case (replacing `memcpy` for such cases) as suggested by @edgchen1, but I am not sure how to reproduce that case to add a corresponding unit-test. Would we have to define a custom `MLCustomLayer` to get a non-contiguous output from a model..? ### Motivation and Context Fix microsoft#17652. --------- Co-authored-by: nicolo-lucchesi <[email protected]> Co-authored-by: Edward Chen <[email protected]>

NickLucche and others added 5 commits September 4, 2023 17:35

fixed performance tuning link

724f9a1

fixed python api link

9e68d5e

support only contiguous model outputs

4e498dc

Merge branch 'microsoft:main' into main

3ddcad7

replace MLMultiArray.dataPointer with getBytesWithHandler

ce77170

edgchen1 reviewed Sep 29, 2023

View reviewed changes

NickLucche added 2 commits September 30, 2023 13:58

pr review changes

b2e8fdb

checking getBytesWithHandler returned size against expected one

37e39e5

edgchen1 reviewed Oct 2, 2023

View reviewed changes

onnxruntime/core/providers/coreml/model/model.mm Outdated Show resolved Hide resolved

120 chars line length limit

6dda63f

edgchen1 previously approved these changes Oct 3, 2023

View reviewed changes

fixed comparison types errors

7b1128e

NickLucche dismissed edgchen1’s stale review via 7b1128e October 4, 2023 08:47

edgchen1 mentioned this pull request Oct 30, 2023

[CoreML] Adapt to MLMultiArray.dataPointer being deprecated #17652

Closed

changed tensor_info to pointer to avoid a shape vector copy

f88132d

NickLucche dismissed edgchen1’s stale review via f88132d November 16, 2023 12:48

NickLucche requested a review from edgchen1 November 17, 2023 16:37

edgchen1 previously approved these changes Nov 17, 2023

View reviewed changes

edgchen1 reviewed Nov 17, 2023

View reviewed changes

onnxruntime/core/providers/coreml/model/model.mm Outdated Show resolved Hide resolved

NickLucche dismissed edgchen1’s stale review via dc82dfb November 17, 2023 19:21

formatting

dc82dfb

Co-authored-by: Edward Chen <[email protected]>

NickLucche requested a review from edgchen1 November 17, 2023 19:22

edgchen1 approved these changes Nov 17, 2023

View reviewed changes

edgchen1 merged commit cbb85b4 into microsoft:main Nov 18, 2023
72 checks passed

NickLucche mentioned this pull request Feb 13, 2024

[Documentation] ORT 1.17.0 contributions list is incomplete #19444

Closed

[CoreML] Adapt to MLMultiArray.dataPointer deprecation #17726

[CoreML] Adapt to MLMultiArray.dataPointer deprecation #17726

Conversation

NickLucche commented Sep 27, 2023 • edited by edgchen1 Loading

Description

Motivation and Context

edgchen1 left a comment

Choose a reason for hiding this comment

edgchen1 commented Sep 29, 2023

edgchen1 commented Sep 29, 2023

azure-pipelines bot commented Sep 29, 2023

NickLucche commented Sep 30, 2023

NickLucche commented Sep 30, 2023

edgchen1 commented Oct 2, 2023

azure-pipelines bot commented Oct 2, 2023

edgchen1 commented Oct 3, 2023

edgchen1 commented Oct 3, 2023

azure-pipelines bot commented Oct 3, 2023

azure-pipelines bot commented Oct 3, 2023

edgchen1 commented Oct 4, 2023

NickLucche commented Oct 4, 2023

edgchen1 commented Oct 4, 2023

NickLucche commented Oct 26, 2023

edgchen1 commented Oct 30, 2023

edgchen1 commented Nov 15, 2023

azure-pipelines bot commented Nov 15, 2023

azure-pipelines bot commented Nov 15, 2023

azure-pipelines bot commented Nov 15, 2023

edgchen1 commented Nov 17, 2023

edgchen1 commented Nov 17, 2023

edgchen1 commented Nov 17, 2023

azure-pipelines bot commented Nov 17, 2023

azure-pipelines bot commented Nov 17, 2023

azure-pipelines bot commented Nov 17, 2023

edgchen1 commented Nov 17, 2023

edgchen1 commented Nov 17, 2023

edgchen1 commented Nov 17, 2023

azure-pipelines bot commented Nov 17, 2023

azure-pipelines bot commented Nov 17, 2023

azure-pipelines bot commented Nov 17, 2023

edgchen1 commented Nov 18, 2023

NickLucche commented Nov 18, 2023

[CoreML] Adapt to `MLMultiArray.dataPointer` deprecation #17726

[CoreML] Adapt to `MLMultiArray.dataPointer` deprecation #17726

NickLucche commented Sep 27, 2023 •

edited by edgchen1

Loading