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.tinygrad: init at 0.8.0 #287914

Merged
merged 3 commits into from
Mar 14, 2024
Merged

Conversation

GaetanLepage
Copy link
Contributor

Description of changes

Add tinygrad, a simple and powerful neural network framework.

cc @SomeoneSerge @MatthewCroughan @wozeparrot

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:
  • 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.05 Release Notes (or backporting 23.05 and 23.11 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.

@wozeparrot
Copy link
Member

Just noting here for future reference, but gpuctypes has been moved in-tree for future releases.


pythonImportsCheck = [ "gpuctypes" ];

# All tests require a GPU to work. This is not possible in the sandbox.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite true, since a lot of the tests can be run with a different backend, like "CLANG", "LLVM", or "CPU"

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh my bad read package name wrong, thought this was in tinygrad

Copy link
Member

@wozeparrot wozeparrot Feb 11, 2024

Choose a reason for hiding this comment

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

But the compilation tests should still be possible to run since they should not need a GPU to be present: https://github.com/tinygrad/gpuctypes/blob/c4c4394239dce4d4ecfe7016ca268c49cb2d02a4/test/test_hip.py#L19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We tried to run the OpenCL ones but got runtime error... Do you think that test_hip.py has better chances ?

Copy link
Member

@wozeparrot wozeparrot Feb 12, 2024

Choose a reason for hiding this comment

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

The opencl ones do require a device but hip and cuda, not the hipdevice or cudadevice tests should be runnable

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes some remote sense maybe. Doesn't OpenCL use the runtime driver to compile the device code, like opengl does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After blackmagic-patching, @SomeoneSerge and I were able to run the compilation tests for HIP and CUDA !

@GaetanLepage
Copy link
Contributor Author

Result of nixpkgs-review pr 287914 run on x86_64-linux 1

6 packages built:
  • python311Packages.gpuctypes
  • python311Packages.gpuctypes.dist
  • python311Packages.tinygrad
  • python311Packages.tinygrad.dist
  • python312Packages.gpuctypes
  • python312Packages.gpuctypes.dist

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Feb 11, 2024
@ofborg ofborg bot requested a review from wozeparrot February 11, 2024 03:17
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Feb 11, 2024
@GaetanLepage
Copy link
Contributor Author

Result of nixpkgs-review pr 287914 run on x86_64-linux 1

6 packages built:
  • python311Packages.gpuctypes
  • python311Packages.gpuctypes.dist
  • python311Packages.tinygrad
  • python311Packages.tinygrad.dist
  • python312Packages.gpuctypes
  • python312Packages.gpuctypes.dist

substituteInPlace gpuctypes/hip.py \
--replace "/opt/rocm/lib/libamdhip64.so" "${rocmPackages.clr}/lib/libamdhip64.so" \
--replace "/opt/rocm/lib/libhiprtc.so" "${rocmPackages.clr}/lib/libhiprtc.so" \
--replace "hipGetDevicePropertiesR0600" "hipGetDeviceProperties"
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thought: this is likely going to break with rocmPackages_6, and this also means this patch doesn't account for the possibility of .override { rocmPackages = somethingElse; }. The failure isn't going to be silent (we know the tests fail unless the symbol is found) so that's enough to get started

Copy link
Contributor

Choose a reason for hiding this comment

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

@NixOS/rocm-maintainers could you actually please test this? We've no idea if this works. We could mark this as broken = rocmSupport but then we'll most likely never find out if it works because nobody would care to override and test it...

Copy link
Member

Choose a reason for hiding this comment

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

I've ran into issues with just patching it like this, the patch is slightly more complicated since the hipDeviceProp_t struct changes between rocm 5 and rocm 6, which is why they have the R0600 suffix. I've patched it by just adding the autogenned struct from rocm 5 and replacing hipDeviceProp_tR0600 with hipDeviceProp_t

Copy link
Contributor

Choose a reason for hiding this comment

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

@wozeparrot so does something need to be changed? By the way, if you've got access to amd hardware, could you maybe run the tests?

Copy link
Member

Choose a reason for hiding this comment

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

Currently I'm running this patch: https://github.com/wozeparrot/tinygrad-nix/blob/main/hip.patch, but this is against tinygrad master where gpuctypes is no longer used but the patch should apply to gpuctypes as well.

By the way, if you've got access to amd hardware, could you maybe run the tests?

Yea I can give running the tests a try.

@GaetanLepage GaetanLepage force-pushed the gpuctypes branch 2 times, most recently from 0037c48 to df30f9e Compare February 20, 2024 23:22
Comment on lines +80 to +86
pytestFlagsArray = lib.optionals (!testCudaRuntime) [
"-k" "'not TestCUDADevice'"
] ++ lib.optionals (!testRocmRuntime) [
"-k" "'not TestHIPDevice'"
] ++ lib.optionals (testCudaRuntime || testOpenclRuntime || testRocmRuntime) [
"-v"
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we can disable the tests conditionally on whether a GPU is available in the sandbox:
https://gist.github.com/GaetanLepage/5cbaefd048c13626eb2eacf567809e49

Based on: #256230

'';

disabledTests = [
# Require internet access
Copy link
Contributor

Choose a reason for hiding this comment

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

A note for future: for small, compact, and reusable things like MNIST, maybe would could actually add a FOD. If we ever do, we probably want to expose it somewhere as an attribute so we can use it other derivations. A subject for a separate discussion really

@SomeoneSerge
Copy link
Contributor

@ofborg build python3Packages.tinygrad

@GaetanLepage
Copy link
Contributor Author

Result of nixpkgs-review pr 287914 run on aarch64-linux 1

2 packages failed to build:
  • python311Packages.tinygrad
  • python311Packages.tinygrad.dist
4 packages built:
  • python311Packages.gpuctypes
  • python311Packages.gpuctypes.dist
  • python312Packages.gpuctypes
  • python312Packages.gpuctypes.dist

@GaetanLepage
Copy link
Contributor Author

Result of nixpkgs-review pr 287914 run on x86_64-linux 1

6 packages built:
  • python311Packages.gpuctypes
  • python311Packages.gpuctypes.dist
  • python311Packages.tinygrad
  • python311Packages.tinygrad.dist
  • python312Packages.gpuctypes
  • python312Packages.gpuctypes.dist

Copy link
Contributor

Choose a reason for hiding this comment

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


       > Executing pythonRuntimeDepsCheck
       > Checking runtime dependencies for tinygrad-0.8.0-py3-none-any.whl
       >   - pyobjc-framework-metal not installed
       >   - pyobjc-framework-libdispatch not installed
       For full logs, run 'nix log /nix/store/yzz6r1irlh2l783y8wq6jdbpri3j387q-python3.11-tinygrad-0.8.0.drv'.

aarch64-darwin https://logs.ofborg.org/?key=nixos/nixpkgs.287914&attempt_id=1be6809e-6aa0-4d50-acce-e1e3616654a8

@GaetanLepage
Copy link
Contributor Author

Result of nixpkgs-review pr 287914 run on x86_64-linux 1

6 packages built:
  • python311Packages.gpuctypes
  • python311Packages.gpuctypes.dist
  • python311Packages.tinygrad
  • python311Packages.tinygrad.dist
  • python312Packages.gpuctypes
  • python312Packages.gpuctypes.dist

@GaetanLepage
Copy link
Contributor Author

@ofborg eval

@SomeoneSerge
Copy link
Contributor

To sum up:

  • The ROCm support is currently enabled but untested.
  • The requiredSystemFeatures-based device tests are, afaik, unconventional and hand't been explicitly approved by anyone, but I consider them harmless and useful.
  • The OpenCL and CUDA functionality have been verified using the upstream pytest suites.
  • IIRC the patches should be mostly upstreamable?

@SomeoneSerge SomeoneSerge merged commit 961dbce into NixOS:master Mar 14, 2024
23 checks passed
@GaetanLepage GaetanLepage deleted the gpuctypes branch March 14, 2024 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants