Move mps data to CPU in util.to_numpy

[adelevie]

Ostensibly solves #883, though I am not very familiar with this codebase.

[BenjaminBossan]

Thank you for the bug report and providing a possible solution. This is probably hard to test, especially with the CI we have. Did you test it for your use case and did it solve the issue?

The M1 support from PyTorch still seems to be WIP (see their issue 77764 (not linking there to avoid spam)) but I guess there is no harm in adding support for this even without a proper way of testing, since the change seems innocuous enough.

[adelevie]

I can say that the TypeError: can't convert mps:0 device type tensor to numpy. error went away and the model did train with no errors.

I figured duck-typing hasattr(X, 'is_mps') was the safest way to work around torch versions that lack MPS support. Looks like it didn't break anything already in CI.

[BenjaminBossan]

I can say that the TypeError: can't convert mps:0 device type tensor to numpy. error went away and the model did train with no errors.

Okay, that's great.

I figured duck-typing hasattr(X, 'is_mps') was the safest way to work around torch versions that lack MPS support. Looks like it didn't break anything already in CI.

Yes, I believe that's the best way of doing it (short of PyTorch providing an official, backwards compatbile method to check it).

I don't believe there is much more that can be done here, so I'm willing to merge. I'll just wait a few days to see if @ottonemo or @thomasjpfan have anything to add.

[thomasjpfan]

Overall, I am okay with this PR.

I think we can add a quick test for to_numpy that only runs when mps is available. Here is a snippet to check if mp is available:

hasattr(torch.backends, "mps") and torch.backends.mps.is_available()

[thomasjpfan]

Nit:

Suggested change

	if hasattr(X, 'is_mps'):
	if X.is_mps:
	X = X.cpu()
	if hasattr(X, 'is_mps') and X.is_mps:
	X = X.cpu()

[BenjaminBossan]

I think we can add a quick test for to_numpy that only runs when mps is available

Yes, why not. It will probably not run for the time being but at least we have something if mps should become available on CI (or one of the skorch users with a MB runs the test locally). @adelevie would you be so kind as to add a test?

[adelevie]

I poked around the test suite last week, and tried subclassing TestToNumpy and overriding the tensor fixtures to use equivalent tensors moved to the mps device. I had a little trouble getting that working, but is that an approach you're looking for, or something smaller, like a quick sanity check with a single test of mps support?

[BenjaminBossan]

Let's just add a new test function to the existing class. The function checks if mps is available, as suggested by Thomas, and skips if it isn't. Then create an mps tensor, run to_numpy on it, and check that it's a numpy array. That should be good enough.

[adelevie]

Sounds good, shouldn’t be too much trouble.

…

On Mon, Aug 8, 2022 at 9:46 AM Benjamin Bossan ***@***.***> wrote: Let's just add a new test function to the existing class. The function checks if mps is available, as suggested by Thomas, and skips if it isn't. Then create an mps tensor, run to_numpy on it, and check that it's a numpy array. That should be good enough. — Reply to this email directly, view it on GitHub <#884 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVGBTXZ5ESAS3W3CE2KV3VYEFVDANCNFSM55TTOGTA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[adelevie]

Should be good to go. I squashed to single cleaner commit, hope that's ok.

[thomasjpfan]

Thank you for the update! I tested this on my m1 mac and the test runs and passes.

[BenjaminBossan]

Thanks @adelevie and @thomasjpfan.

I squashed to single cleaner commit, hope that's ok.

Yeah, that's okay, but we would have squashed anyway, so it wasn't necessary.