Conversation
|
You got a little ahead of me, but happy to see the contribution! I set the repo settings to enable Actions; I will try to close and reopen the PR to see if that triggers a build. If not, then you might need to create a new PR. |
| run: | ||
| - | |
There was a problem hiding this comment.
Looks like there are some syntax issues in your workflow definitions. In particular, run: should be a string, not an array.
| run: | |
| - | | |
| run: | |
.github/workflows/pull_request.yml
Outdated
| - uses: actions/checkout@v5 | ||
| - uses: ./.github/actions/setup-macos | ||
| - uses: ./.github/actions/build-macos |
There was a problem hiding this comment.
If we're not doing a second workflow (like a nightly or weekly main build or some kind of release build) then you could do all of the actions inline. Up to you for how complex this needs to be
There was a problem hiding this comment.
OK, I think one set of actions would be fine. We don't need nightlies or release builds as people pick it up via swiftpm (and the build is on them).
There was a problem hiding this comment.
so do you suggest folding the two actions together or just put the actions inline in the workflow?
I think we will want a linux build pretty soon so maybe just combine all the actions in build-macos
There was a problem hiding this comment.
macOS and linux building is sometimes different enough that we can't combine those into a single reusable/composite actions. For now I would fold setup-macos and build-macos into pull_request.yaml and then pull things out in the future as we discover that we need them.
There was a problem hiding this comment.
ok, sounds good. I will get that set up in the morning
| print("target: b = \(b), m = \(m)") | ||
| print("parameters: \(model.parameters())") | ||
| // print("target: b = \(b), m = \(m)") | ||
| // print("parameters: \(model.parameters())") |
There was a problem hiding this comment.
These were very large and not useful (unless it fails).
| xcrun --show-sdk-build-version | ||
| swift --version | ||
| rm -rf ~/Library/Developer/Xcode/DerivedData/* | ||
| xcodebuild build-for-testing -scheme mlx-swift-Package -destination 'platform=macOS' |
There was a problem hiding this comment.
This builds both the normal targets and the test targets.
| shell: sh | ||
| run: | | ||
| xcrun xctest ~/Library/Developer/Xcode/DerivedData/mlx-swift-*/Build/Products/Debug/CmlxTests.xctest | ||
| xcrun xctest ~/Library/Developer/Xcode/DerivedData/mlx-swift-*/Build/Products/Debug/MLXTests.xctest |
There was a problem hiding this comment.
Run the tests (avoids the connection to service named com.apple.testmanagerd.control was invalidated issue).
|
@madrob I think this is working now, see https://github.com/ml-explore/mlx-swift/actions/runs/19180312566 |
|
https://github.com/ml-explore/mlx-swift/actions/runs/19180312566/job/54835197394?pr=297#step:10:1058 Is this a relevant failure? Shouldn't block this merge, but possibly worth investigating afterwards. |
That is actually the error handling code (where we catch c++ exceptions and turn them into swift errors) -- this is one of the tests. It might be worth a comment in the code or a print maybe to indicate this is expected because you are right it looks a bit alarming! |
@madrob I copied what you did for mlx and simplified for what we need here. I don't know if you were planning to set this up in mlx-swift, if so this may be useful, if not we can use what I have. I think I additionally need repo settings to activate but I can't see what you did in mlx