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.

Sign up for GitHub

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

Jump to bottom

rocmPackages: rpp, mivisionx, meta package sets #260299

Merged
merged 5 commits into from
Oct 12, 2023
Merged

rocmPackages: rpp, mivisionx, meta package sets #260299

merged 5 commits into from
Oct 12, 2023

Conversation

Madouura
Copy link
Contributor

@Madouura Madouura commented Oct 10, 2023 •
edited
Loading

Description of changes

Tracking: #197885
Emulate common ROCm meta layout
These are mainly for users. I strongly suggest NOT using these in nixpkgs derivations.
See: https://rocm.docs.amd.com/en/latest/_images/image.004.png
See: https://rocm.docs.amd.com/en/latest/deploy/linux/os-native/package_manager_integration.html

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@Madouura Madouura mentioned this pull request Oct 10, 2023
Open
34 tasks
@Madouura
Copy link
Contributor Author

Madouura commented Oct 10, 2023 •
edited
Loading

Note: I chose symlinkJoin because more than likely for the user whatever documentation they're reading is going to want them to set ROCM_PATH. This is the best way to ensure there are no problems for these user-facing meta packages.

Edit: I also just went ahead and included package lists in case for some reason they are preferable in nixpkgs or the user's configuration, but I still suggest specifying them individually.

@Madouura Madouura force-pushed the pr/rocm-meta branch 2 times, most recently from 85314a4 to e922d1e Compare October 10, 2023 23:00
@lovesegfault
Copy link
Member

lovesegfault commented Oct 11, 2023 •
edited
Loading

@Madouura IMO get rid of the package lists and just make the meta-packages always be a symlinkJoin.

Nixpkgs usage should reference the underlying pkgs directly, I agree.

@Madouura
Copy link
Contributor Author

Madouura commented Oct 11, 2023 •
edited
Loading

That's what I did at first, but on second thought it might be useful to users to be able to specify say rocmPackages.meta.rocm-developer-tools in whatever flake they're using, that's why I added it. (I.E. a dev environment)

@lovesegfault
Copy link
Member

Let me see if I understand your point. You're claiming the non-symlinkJoined attrs are useful because one could do:

mkShell {
    buildInputs = [ rocmPackages.meta.foobar ];
}

And that would just work.

But why wouldn't this:

mkShell {
    buildInputs = [ rocmPackages.meta.foobar-joined ];
}

Yield the same result/experience for the end-user?

@Madouura
Copy link
Contributor Author

Honestly, that's a good point. I'll change it when I'm done with mivisionx, or maybe just go ahead and add that to this PR.

@Madouura Madouura changed the title rocmPackages: Implement meta package sets rocmPackages: rpp, mivisionx, meta package sets Oct 11, 2023
@Madouura
Copy link
Contributor Author

Everything seems to check out to me.
About time I got mivisionx working.

@ofborg ofborg bot requested review from Flakebi and dotlambda October 11, 2023 05:22
@Madouura
Copy link
Contributor Author

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

7 packages built:
  • rapidjson-git
  • rocmPackages.mivisionx (rocmPackages.mivisionx-hip ,rocmPackages_5.mivisionx ,rocmPackages_5.mivisionx-hip)
  • rocmPackages.mivisionx-cpu (rocmPackages_5.mivisionx-cpu)
  • rocmPackages.mivisionx-opencl (rocmPackages_5.mivisionx-opencl)
  • rocmPackages.rpp (rocmPackages.rpp-hip ,rocmPackages_5.rpp ,rocmPackages_5.rpp-hip)
  • rocmPackages.rpp-cpu (rocmPackages_5.rpp-cpu)
  • rocmPackages.rpp-opencl (rocmPackages_5.rpp-opencl)

@Madouura Madouura force-pushed the pr/rocm-meta branch 2 times, most recently from 63cde21 to 9aae0dd Compare October 11, 2023 12:44
@Madouura
Copy link
Contributor Author

I should note that the _fallback.dat md5sums were NOT equal from output to output.
This is why we have to do two full builds. Honestly, we should just up the build output limit, even if we can't up the final output limit, so we can just process a big build into smaller derivations.
nixpkgs-review incoming at some point.

@Madouura Madouura force-pushed the pr/rocm-meta branch 2 times, most recently from aba1305 to 33dfc0d Compare October 11, 2023 13:00
lovesegfault
lovesegfault reviewed Oct 11, 2023
View reviewed changes
Copy link
Member

@lovesegfault lovesegfault left a comment

Choose a reason for hiding this comment

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

Looks good, left a nit :)

pkgs/top-level/all-packages.nix Show resolved Hide resolved
pkgs/development/libraries/rapidjson/git.nix Outdated Show resolved Hide resolved
@lovesegfault
Copy link
Member

Also, for another PR, it would be cool to explore using lld to link the rocmPackages since I suspect that will help speed up the insanely long build times of things like rocsolver.

Just an idea, since flang does support lld, but not related to this and not strictly needed :)

@lovesegfault
Copy link
Member

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

24 packages built:
  • magma-hip
  • python310Packages.torchWithRocm
  • python310Packages.torchWithRocm.dev
  • python310Packages.torchWithRocm.dist
  • python310Packages.torchWithRocm.lib
  • python311Packages.torchWithRocm
  • python311Packages.torchWithRocm.dev
  • python311Packages.torchWithRocm.dist
  • python311Packages.torchWithRocm.lib
  • rapidjson-git
  • rocmPackages.hipblas (rocmPackages_5.hipblas)
  • rocmPackages.hipsolver (rocmPackages_5.hipsolver)
  • rocmPackages.migraphx (rocmPackages_5.migraphx)
  • rocmPackages.miopen (rocmPackages.miopen-hip ,rocmPackages_5.miopen ,rocmPackages_5.miopen-hip)
  • rocmPackages.miopen-opencl (rocmPackages_5.miopen-opencl)
  • rocmPackages.mivisionx (rocmPackages.mivisionx-hip ,rocmPackages_5.mivisionx ,rocmPackages_5.mivisionx-hip)
  • rocmPackages.mivisionx-cpu (rocmPackages_5.mivisionx-cpu)
  • rocmPackages.mivisionx-opencl (rocmPackages_5.mivisionx-opencl)
  • rocmPackages.rocalution (rocmPackages_5.rocalution)
  • rocmPackages.rocblas (rocmPackages_5.rocblas)
  • rocmPackages.rocsolver (rocmPackages_5.rocsolver)
  • rocmPackages.rpp (rocmPackages.rpp-hip ,rocmPackages_5.rpp ,rocmPackages_5.rpp-hip)
  • rocmPackages.rpp-cpu (rocmPackages_5.rpp-cpu)
  • rocmPackages.rpp-opencl (rocmPackages_5.rpp-opencl)

@Madouura
Copy link
Contributor Author

Also, for another PR, it would be cool to explore using lld to link the rocmPackages since I suspect that will help speed up the insanely long build times of things like rocsolver.

Just an idea, since flang does support lld, but not related to this and not strictly needed :)

Does rocm-modules/5/llvm/stage-3/clang.nix#L53-L59 not use the lld linker? Or is this about gfortran?

@Madouura
Copy link
Contributor Author

Madouura commented Oct 11, 2023 •
edited
Loading

Ah, this may be why.

nixpkgs/pkgs/development/rocm-modules/5/llvm/stage-3/clang.nix

Lines 69 to 72 in 6f2581d

# GPU compilation uses builtin `lld`
substituteInPlace $out/bin/{clang,clang++} \
--replace "-MM) dontLink=1 ;;" "-MM | --cuda-device-only) dontLink=1 ;;''\n--cuda-host-only | --cuda-compile-host-device) dontLink=0 ;;"
'';

@Madouura
Copy link
Contributor Author

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

24 packages built:
  • magma-hip
  • python310Packages.torchWithRocm
  • python310Packages.torchWithRocm.dev
  • python310Packages.torchWithRocm.dist
  • python310Packages.torchWithRocm.lib
  • python311Packages.torchWithRocm
  • python311Packages.torchWithRocm.dev
  • python311Packages.torchWithRocm.dist
  • python311Packages.torchWithRocm.lib
  • rapidjson-unstable
  • rocmPackages.hipblas
  • rocmPackages.hipsolver
  • rocmPackages.migraphx
  • rocmPackages.miopen (rocmPackages.miopen-hip)
  • rocmPackages.miopen-opencl
  • rocmPackages.mivisionx (rocmPackages.mivisionx-hip)
  • rocmPackages.mivisionx-cpu
  • rocmPackages.mivisionx-opencl
  • rocmPackages.rocalution
  • rocmPackages.rocblas
  • rocmPackages.rocsolver
  • rocmPackages.rpp (rocmPackages.rpp-hip)
  • rocmPackages.rpp-cpu
  • rocmPackages.rpp-opencl

@lovesegfault
Copy link
Member

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

24 packages built:
  • magma-hip
  • python310Packages.torchWithRocm
  • python310Packages.torchWithRocm.dev
  • python310Packages.torchWithRocm.dist
  • python310Packages.torchWithRocm.lib
  • python311Packages.torchWithRocm
  • python311Packages.torchWithRocm.dev
  • python311Packages.torchWithRocm.dist
  • python311Packages.torchWithRocm.lib
  • rapidjson-unstable
  • rocmPackages.hipblas
  • rocmPackages.hipsolver
  • rocmPackages.migraphx
  • rocmPackages.miopen (rocmPackages.miopen-hip)
  • rocmPackages.miopen-opencl
  • rocmPackages.mivisionx (rocmPackages.mivisionx-hip)
  • rocmPackages.mivisionx-cpu
  • rocmPackages.mivisionx-opencl
  • rocmPackages.rocalution
  • rocmPackages.rocblas
  • rocmPackages.rocsolver
  • rocmPackages.rpp (rocmPackages.rpp-hip)
  • rocmPackages.rpp-cpu
  • rocmPackages.rpp-opencl

@lovesegfault
Copy link
Member

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

24 packages built:
  • magma-hip
  • python310Packages.torchWithRocm
  • python310Packages.torchWithRocm.dev
  • python310Packages.torchWithRocm.dist
  • python310Packages.torchWithRocm.lib
  • python311Packages.torchWithRocm
  • python311Packages.torchWithRocm.dev
  • python311Packages.torchWithRocm.dist
  • python311Packages.torchWithRocm.lib
  • rapidjson-unstable
  • rocmPackages.hipblas
  • rocmPackages.hipsolver
  • rocmPackages.migraphx
  • rocmPackages.miopen (rocmPackages.miopen-hip)
  • rocmPackages.miopen-opencl
  • rocmPackages.mivisionx (rocmPackages.mivisionx-hip)
  • rocmPackages.mivisionx-cpu
  • rocmPackages.mivisionx-opencl
  • rocmPackages.rocalution
  • rocmPackages.rocblas
  • rocmPackages.rocsolver
  • rocmPackages.rpp (rocmPackages.rpp-hip)
  • rocmPackages.rpp-cpu
  • rocmPackages.rpp-opencl

@lovesegfault lovesegfault merged commit e95c10f into NixOS:master Oct 12, 2023
20 of 22 checks passed
@dotlambda dotlambda mentioned this pull request Oct 12, 2023
Closed
@Madouura Madouura deleted the pr/rocm-meta branch October 12, 2023 20:01
@SomeoneSerge SomeoneSerge mentioned this pull request Dec 26, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotlambda dotlambda dotlambda left review comments

@lovesegfault lovesegfault lovesegfault approved these changes

@Flakebi Flakebi Awaiting requested review from Flakebi

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Madouura @lovesegfault @dotlambda