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.

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

Add MetalPerformanceShadersGraph Bindings #14303

Merged
merged 59 commits into from
May 10, 2022
Merged

Conversation

praeclarum
Copy link
Contributor

@praeclarum praeclarum commented Mar 3, 2022

I'm very pleased to present full bindings to the MetalPerformanceShadersGraph framework!

I'm happy with how everything turned out with the exception of a few notes and questions below.

I re-implemented Apple's MNIST sample (from https://developer.apple.com/documentation/metalperformanceshadersgraph/training_a_neural_network_using_mps_graph) here:

https://gist.github.com/praeclarum/b8077771fb341a1f9c28240113e00425

Fixes #14286

Notes

  • Although the API says it works on macOS 11, it has bugs and crashes with errors even with Apple’s Swift examples. It’s better on macOS 12. iOS 14 and on is fine.

  • MPSGraphSparseStorageType has terrible names. They match Apple's but I wish they were better.

  • I added convenience methods to MPSNDArray and MPSGrapTensorData and the Variable and Constant operations to decrease the amount of unsafe code users have to write. I currently do this for 32-bit floats, the most common data type.

Questions

  • In Target.cs GatherFrameworks, Metal is removed from simulator builds. Doesn’t Metal work now?

  • Is the way I handle MPSGraphType OK? MPSGraphType was introduced in iOS 15 (macOS 12) and became the base class for MPSGraphShapedType which existed in iOS 14. In iOS 14, MPSGraphShapedType inherits directly from NSObject. To keep compatibility, MPSGraphType is type-erased to just NSObject and implementers have to add NSCopying.

@rolfbjarne rolfbjarne added community Community contribution ❤ run-dotnet-tests Run all the .NET tests labels Mar 3, 2022
@rolfbjarne
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

I'll continue reviewing/address questions tomorrow with a fresh brain heh, but so far it is looking great!

src/MetalPerformanceShaders/MPSNDArray.cs Outdated Show resolved Hide resolved
src/MetalPerformanceShaders/MPSNDArray.cs Outdated Show resolved Hide resolved
src/MetalPerformanceShadersGraph/MPSGraphEnums.cs Outdated Show resolved Hide resolved
src/MetalPerformanceShadersGraph/MPSGraphEnums.cs Outdated Show resolved Hide resolved
src/MetalPerformanceShadersGraph/MPSGraphEnums.cs Outdated Show resolved Hide resolved
src/MetalPerformanceShadersGraph/MPSGraphExtensions.cs Outdated Show resolved Hide resolved
src/MetalPerformanceShadersGraph/MPSGraphExtensions.cs Outdated Show resolved Hide resolved
src/metalperformanceshadersgraph.cs Show resolved Hide resolved
src/metalperformanceshadersgraph.cs Show resolved Hide resolved
src/metalperformanceshadersgraph.cs Show resolved Hide resolved
@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥

Result file D:\a\1\s\Reports\TestSummary-simulator\TestSummary.md not found.

Pipeline on Agent XAMBOT-1101.Monterey'
Merge 6e54512 into a896490

@rolfbjarne
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@praeclarum
Copy link
Contributor Author

@rolfbjarne I know I’ve seen this error before, but I frustratingly can’t remember the context..

I’m pretty sure the Apple sample produces this error too on old bad implementations of MPSGraph. I know I ran into it, got frustrated and moved on. I was working with the Swift sample at the time, but I can’t remember the OS.

I can’t tell from the tests whether this is working on Mac 12 (M1 and Intel). If it is, then I’m willing to chalk this up to old bugs in Mac 11. I know the Apple sample works on Mac 12.2 M1. If this does too, then I think we should just disable the test for older Mac versions.

@rolfbjarne
Copy link
Member

@praeclarum ok, I'll just make the test require macOS 12+

@rolfbjarne
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥

Result file D:\a\1\s\Reports\TestSummary-simulator\TestSummary.md not found.

Pipeline on Agent XAMBOT-1109.Monterey'
Merge 5323498 into e9333ba

@rolfbjarne
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [CI Build] Artifacts 📚

Artifacts were not provided.

Pipeline on Agent XAMBOT-1167.Monterey
Hash: 53234981def2e09aa0facd84996dec8c9760bda2

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📋 [PR Build] API Diff 📋

API Current PR diff

ℹ️ API Diff (from PR only) (please review changes)

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

Generator diff

ℹ️ Generator Diff (please review changes)

Pipeline on Agent XAMBOT-1164.Monterey'
Hash: 53234981def2e09aa0facd84996dec8c9760bda2

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

  • linksdk
  • linkall

Pipeline on Agent
Hash: 53234981def2e09aa0facd84996dec8c9760bda2

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: 53234981def2e09aa0facd84996dec8c9760bda2

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌

Tests failed on VSTS: simulator tests iOS.

Test results

23 tests failed, 130 tests passed.

Failed tests

  • link sdk/Mac [dotnet]/Debug [dotnet]: Failed (Test run failed.
    Tests run: 117 Passed: 107 Inconclusive: 0 Failed: 1 Ignored: 9)
  • link sdk/Mac [dotnet]/Release [dotnet]: Failed (Test run failed.
    Tests run: 117 Passed: 106 Inconclusive: 0 Failed: 1 Ignored: 10)
  • link sdk/Mac Catalyst [dotnet]/Debug [dotnet]: Failed (Test run failed.
    Tests run: 129 Passed: 119 Inconclusive: 0 Failed: 1 Ignored: 9)
  • link sdk/Mac Catalyst [dotnet]/Release [dotnet]: Failed (Test run failed.
    Tests run: 129 Passed: 118 Inconclusive: 0 Failed: 1 Ignored: 10)
  • link sdk/Mac Modern/Debug: Failed (Test run failed.
    Tests run: 9 Passed: 8 Inconclusive: 0 Failed: 1 Ignored: 0)
  • link sdk/Mac Modern/Release: Failed (Test run failed.
    Tests run: 9 Passed: 8 Inconclusive: 0 Failed: 1 Ignored: 0)
  • link sdk/iOS Unified 64-bits - simulator/Debug [dotnet]: Failed
  • link sdk/iOS Unified 64-bits - simulator/Release [dotnet]: Failed
  • link sdk/iOS Unified 64-bits - simulator/Debug: Failed
  • link sdk/iOS Unified 64-bits - simulator/Release: Failed
  • link sdk/tvOS - simulator/Release [dotnet]: Failed
  • link sdk/tvOS - simulator/Release: Failed
  • trimmode link/Mac [dotnet]/Debug [dotnet]: Failed (Test run failed.
    Tests run: 117 Passed: 107 Inconclusive: 0 Failed: 1 Ignored: 9)
  • trimmode link/Mac [dotnet]/Release [dotnet]: Failed (Test run failed.
    Tests run: 117 Passed: 106 Inconclusive: 0 Failed: 1 Ignored: 10)
  • trimmode link/Mac Catalyst [dotnet]/Debug [dotnet]: Failed (Test run failed.
    Tests run: 129 Passed: 119 Inconclusive: 0 Failed: 1 Ignored: 9)
  • trimmode link/Mac Catalyst [dotnet]/Release [dotnet]: Failed (Test run failed.
    Tests run: 129 Passed: 118 Inconclusive: 0 Failed: 1 Ignored: 10)
  • trimmode link/iOS Unified 64-bits - simulator/Debug [dotnet]: Failed
  • trimmode link/iOS Unified 64-bits - simulator/Release [dotnet]: Failed
  • trimmode link/tvOS - simulator/Debug [dotnet]: Failed
  • trimmode link/tvOS - simulator/Release [dotnet]: Failed
  • link all/Mac Modern/Debug: Failed (Test run failed.
    Tests run: 20 Passed: 18 Inconclusive: 0 Failed: 1 Ignored: 1)
  • link all/Mac Modern/Release: Failed (Test run failed.
    Tests run: 20 Passed: 18 Inconclusive: 0 Failed: 1 Ignored: 1)
  • introspection/iOS Unified 64-bits - simulator/Debug: Crashed

Pipeline on Agent XAMBOT-1023.Monterey'
Merge 5323498 into 32c29c1

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

Looks like all test failures but one are network related (https://github.com/xamarin/maccore/issues/1067).

The remaining one seems to be a simulator hiccup.

@rolfbjarne rolfbjarne merged commit bd4fee0 into xamarin:main May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution ❤
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bind MetalPerformanceShadersGraph Framework
6 participants