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

[TODO]: Run tests on M1 #776

Open
KnathanM opened this issue Apr 10, 2024 · 4 comments · May be fixed by #863
Open

[TODO]: Run tests on M1 #776

KnathanM opened this issue Apr 10, 2024 · 4 comments · May be fixed by #863
Assignees
Labels
todo add an item to the to-do list
Milestone

Comments

@KnathanM
Copy link
Contributor

Notes
#711 showed that our code couldn't run on an M1 MBP. #761 fixed this by explicitly using float32's instead of float64's. Perhaps we should run our tests on an M1 image so that future changes don't accidentally reintroduce float64 usage.

@KnathanM KnathanM added the todo add an item to the to-do list label Apr 10, 2024
@KnathanM KnathanM added this to the v2.1.0 milestone Apr 10, 2024
@JacksonBurns
Copy link
Member

The CI currently specifies macos-latest which resolves to the macos-14 image running on M1, see the GitHub docs.

We didn't catch this earlier because our tests must have just missed it.

@KnathanM
Copy link
Contributor Author

Thanks for pointing that out. Our tests have been working on macos-latest even with using some float64's. Do you know why our tests worked while #711 had the issue? Is that something we can reproduce in the GitHub tests so it isn't missed in the future?

@JacksonBurns
Copy link
Member

I looked back at the issue and realized that it's actually because the user was running using MPS (see PyTorch docs), basically the apple version of CUDA/ROCM acceleration. Our unit tests probably would have found this if anyone had run them on a machine using MPS-pytorch. The github actions runners don't have GPUs unless we upgrade to this so we have no way to catch it right now. But really it's a pretty small problem - we can probably safely ignore this and just fix these issues as they arise.

@kevingreenman
Copy link
Member

Reopening per the discussion in #841

#841 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
todo add an item to the to-do list
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants