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

python3Packages.torch.tests: MPS tests for darwin #351782

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikatammi
Copy link
Contributor

@mikatammi mikatammi commented Oct 28, 2024

Things done

Depends on #351778. Add new test to pytorch for Darwin platform. It tests CPU without torch.compile decorator, and MPS if MPS device is present.

Modify pytorch tests so it also tests MPS during CPU tests on Darwin. I'm not sure is this the right way to do this but at least this is a good starting point for discussion. I was also unsure do the tests work for other platforms. MPS is built for macOS by default if #351778 gets merged in its current state, so it does not need specific torchWithMps build.

  • 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:
  • 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 mikatammi force-pushed the pytorch_test_pr branch 2 times, most recently from 437e3f9 to 58c970d Compare October 28, 2024 00:09
@mikatammi
Copy link
Contributor Author

Also got bitten by nixfmt check, fixing that made the diff quite hard to read

@ofborg ofborg bot added 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 28, 2024
Copy link
Member

@samuela samuela left a comment

Choose a reason for hiding this comment

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

a lot of the diff appears to be just formatting changes. let's try to keep the git blame clean

@vcunat vcunat changed the base branch from staging-next to master October 31, 2024 09:47
@mikatammi
Copy link
Contributor Author

a lot of the diff appears to be just formatting changes. let's try to keep the git blame clean

I re force-pushed the whole thing and now the diff only shows the lines I changed, however I now get complaint from the nix formatting checks.

@mikatammi mikatammi force-pushed the pytorch_test_pr branch 2 times, most recently from 692e739 to 3b563f0 Compare November 3, 2024 18:54
@mikatammi
Copy link
Contributor Author

a lot of the diff appears to be just formatting changes. let's try to keep the git blame clean

I re force-pushed the whole thing and now the diff only shows the lines I changed, however I now get complaint from the nix formatting checks.

Made another commit on top to clearly show which part is formatting and which is not

@mikatammi mikatammi force-pushed the pytorch_test_pr branch 2 times, most recently from d88b8a6 to 07596eb Compare November 5, 2024 21:26
@SomeoneSerge SomeoneSerge self-requested a review November 6, 2024 15:37
@mikatammi
Copy link
Contributor Author

Actually are you doing merge commits nowadays? Should I make separate PR for the .git-blame-ignore-revs in case the commit hash gets mangled

@SomeoneSerge
Copy link
Contributor

Actually are you doing merge commits nowadays

Yes we do, I think it's ok to keep it in 1 PR

@mikatammi mikatammi force-pushed the pytorch_test_pr branch 4 times, most recently from 8d5b802 to 83a4928 Compare November 8, 2024 23:47
@mikatammi
Copy link
Contributor Author

I changed this now so there's a separate test for Darwin. I think in the end it is cleaner this way

libraries,
pythonPackages,
}:
(cudaPackages.writeGpuTestPython.override { python3Packages = pythonPackages; })
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need writeGpuTestPython then (it adds a gpuCheck attribute with requiredSystemFeatures set to access CUDA in the sandbox).

FWIW you make this a simple

runCommand "test-torch-mps" { nativeBuildInputs = [ (python.withPackages ...) ]; } ''
  python << \EOF
  import torch
  ...
  EOF
''

This maybe has the undesired side-effect that getting a script to run outside the sandbox would take an extra step, but also I don't think anyone would actually need that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about this version? I made it to use writeShellApplcation and simply put the python script to separate file because it doesn't need any generated things from Nix side

@mikatammi mikatammi force-pushed the pytorch_test_pr branch 2 times, most recently from 805896a to 9a5874a Compare November 9, 2024 03:51
Comment on lines +31 to +33
tester-darwin = callPackage ./mk-darwin-check.nix {
libraries = ps: [ ps.torch ];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not run the test as part of the derivation though:)

Good idea to move the script into a separate file

It's still a bit disappointing that the script is essentially the same as for cuda/rocm, the difference is just one line which we could simply parameterize (e.g. instead of the @ decorator syntax, we could f = compile(f) under an if guard)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we somehow also add this as part of the tests for the package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this good to go in now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we somehow also add this as part of the tests for the package?

We need to make a derivation which runs this script during the build, and we need to put that derivation under passthru.tests

Tests for Darwin. Both CPU and MPS are tested.

Signed-off-by: Mika Tammi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 6.topic: python 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants