python3Packages.torch: switch to apple-sdk_13

[mikatammi]

Since #346043 was merged to staging, switch to apple-sdk_13 on Darwin. This enables the use of MPS (Metal Performance Shaders) on macOS.

Fixes #243868

Quick test:

$ nix-shell -p 'python3.withPackages (ps: with ps; [ torch ])' -I nixpkgs=.

[nix-shell:~/nixpkgs.git]$ python
Python 3.12.7 (main, Oct  1 2024, 02:05:46) [Clang 16.0.6 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import torch
>>> x = torch.ones(5, device="mps")
>>> y = x * 2
>>> print(y)
tensor([2., 2., 2., 2., 2.], device='mps:0')

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
    From package tests, tester-compileCpu was broken because there's no OpenMP in macOS. I hacked it so I could run both CPU and MPS tests on macOS, but I'm going to propose fixes to tests in another PR, because it might become lenghty discussion.
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[mikatammi]

I also want to include tests for MPS but I was a bit unsure were they even working for other platforms than CUDA. Tests in separate PR here: #351782

[samuela]

diff lgtm

thanks for putting this together @mikatammi ! I think we can merge this on the condition that we commit to adding tests, either in #351782 or elsewhere

[mikatammi]

Strangely I noticed tests started failing in accelerate-package, which depends on torch, after this change

[mikatammi]

I'm wondering should I just create additional package called torchWithMps, in similar fashion than there now exists torchWithCuda and torchWithRocm. The MPS should then be explicitly disabled in basic torch package, as it gets automatically detected now that the change to use apple-sdk_13 as buildInput was made

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/title-the-darwin-sdks-have-been-updated/55295/1

[samuela]

Strangely I noticed tests started failing in accelerate-package, which depends on torch, after this change

Interesting, do you have logs for these failures?

I'm wondering should I just create additional package called torchWithMps, in similar fashion than there now exists torchWithCuda and torchWithRocm. The MPS should then be explicitly disabled in basic torch package, as it gets automatically detected now that the change to use apple-sdk_13 as buildInput was made

Not the craziest idea ever, but I feel like it would be simpler to avoid adding another derivation flag and alias if at all possible

[mikatammi]

#353184

I created a separate PR for fixing up issues in accelerate package.

[mikatammi]

Strangely I noticed tests started failing in accelerate-package, which depends on torch, after this change

Interesting, do you have logs for these failures?

#353184 (comment)

[mikatammi]

Strangely I noticed tests started failing in accelerate-package, which depends on torch, after this change

Interesting, do you have logs for these failures?

#353184 (comment)

Apparently my fixes for skipping some of the accelerate tests were already merged before this PR which breaks them

[SomeoneSerge]

(I see there's already an approval; feel free to merge, I self-requested a review just so this doesn't get lost)

[SomeoneSerge]

I'm wondering should I just create additional package called torchWithMps, in similar fashion than there now exists torchWithCuda and torchWithRocm.

• I'd rather delete torchWith{Cuda,Rocm}, they're broken anyway (one has to rebuild dependencies like MPI with cuda/rocm too, and one shouldn't mix cuda pytorch with non-cuda rev dependencies; tldr one shouldn't use this attribute, one should just create a whole new consistent package set)
• There's no need for a separate attribute in case of MPS because we should just always enable it when it's supported, I assume it just comes with the box. Is there any hardware that wouldn't be supported by this mps torch?

[samuela]

It would have to be so ancient that I think it's outside our support window. Even in the off chance this gets built and run on a machine w/o Metal, that would be a runtime error avoidable by setting device= correctly, no?

[SomeoneSerge]

Maybe at some point we should move these to env, and use optionalAttrs for platform-specific stuff, because the way we do it now we trigger a useless rebuild on linux. But that's OK.

[SomeoneSerge]

(just waiting for ofborg-eval now)