Skip to content

oneDNN_2: reintroduce and unbreak, rocmPackages.migraphx: use oneDNN_2 #448964

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
Merged
dotlambda merged 4 commits into from
Oct 12, 2025
Merged

oneDNN_2: reintroduce and unbreak, rocmPackages.migraphx: use oneDNN_2 #448964

dotlambda merged 4 commits into from
Oct 12, 2025

Conversation

@LunNova
Copy link
Member

@LunNova LunNova commented Oct 5, 2025 •
edited
Loading

Reintroducing oneDNN_2 for compliance with #421201 and fixing its build with CMake 4 (#445447).

Upstream issue for oneDNN 3 compat in migraphx: ROCm/AMDMIGraphX#4351

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@LunNova LunNova requested a review from dotlambda October 5, 2025 22:32
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 6.topic: rocm ROCm is an Advanced Micro Devices software stack for graphics processing unit programming. labels Oct 5, 2025
@LunNova LunNova mentioned this pull request Oct 5, 2025
Merged
13 tasks
@Sigmanificient
Copy link
Member

Sigmanificient commented Oct 6, 2025

nixpkgs-review result

Generated using nixpkgs-review-gha

Command: nixpkgs-review pr 448964
Commit: 2b8b70a06925643715f46fdcd12a5b5e2f73114a (subsequent changes)
Merge: 45e65765f62d31640c77da0969487e464628fec7

Logs: https://github.com/Sigmanificient/nixpkgs-review-gha/actions/runs/18268014471

x86_64-linux

❌ 2 packages failed to build:
  • rocmPackages.migraphx (rocmPackages_6.migraphx)
  • rocmPackages.mivisionx (rocmPackages.mivisionx-hip, rocmPackages_6.mivisionx, rocmPackages_6.mivisionx-hip)

aarch64-linux

✅ No rebuilds

@LunNova
Copy link
Member Author

LunNova commented Oct 6, 2025

@Sigmanificient hipblaslt> Subprocess killed because actions runners are too small again 😅

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 448964 --package rocmPackages.migraphx
Commit: 2b8b70a06925643715f46fdcd12a5b5e2f73114a

x86_64-linux

✅ 1 package built:
  • rocmPackages.migraphx

@Sigmanificient
Copy link
Member

Sigmanificient commented Oct 6, 2025

@LunNova Seems like you are editing quite heavy packages 🙃

@GZGavinZhao
Copy link
Contributor

GZGavinZhao commented Oct 6, 2025 

hipblaslt> Subprocess killed

Least resource-intensive ROCm package

dotlambda
dotlambda reviewed Oct 6, 2025
View reviewed changes
pkgs/development/rocm-modules/6/migraphx/default.nix Outdated
oneDNN' = oneDNN.overrideAttrs rec {
# TODO(@LunNova): stop overriding here once oneDNN 3 is supported
# Upstream issue: https://github.com/ROCm/AMDMIGraphX/issues/4351
oneDNN' = oneDNN.overrideAttrs (old: rec {
Copy link
Member

@dotlambda dotlambda Oct 6, 2025

Choose a reason for hiding this comment

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

In light of #421201, we should probably introduce a package oneDNN_2.

Copy link
Member Author

@LunNova LunNova Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I remember asking not to drop oneDNN_2 in #416821 (comment) 😭

I guess that's cleaner, I'll revert the drop and set the team to rocm rather than the old maintainers.

Copy link
Member

@dotlambda dotlambda Oct 6, 2025

Choose a reason for hiding this comment

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

Sorry about that! I definitely like that it's the ROCm team who's responsible for oneDNN_2 now.

LunNova added 2 commits October 6, 2025 09:08
@LunNova
Revert "oneDNN_2: drop"
69c633e 
This reverts commit f596809.

let's revert to reduce risk of future oneDNN_2 breakage in migraphx.
@LunNova
rocmPackages.migraphx: use oneDNN_2 instead of oneDNN.overrideAttrs
1fd4322
@LunNova LunNova changed the title rocmPackages.migraphx: fix CMake 4 build of oneDNN override oneDNN_2: revert drop, fix CMake 4 build, rocmPackages.migraphx: use oneDNN_2 Oct 6, 2025
@LunNova LunNova changed the title oneDNN_2: revert drop, fix CMake 4 build, rocmPackages.migraphx: use oneDNN_2 oneDNN_2: reintroduce, rocmPackages.migraphx: use oneDNN_2 Oct 6, 2025
@LunNova LunNova force-pushed the push-ktynwzmznuxt branch 2 times, most recently from 3bf5f9c to 50e6f6f Compare October 6, 2025 16:21
@LunNova LunNova force-pushed the push-ktynwzmznuxt branch from 50e6f6f to f315061 Compare October 6, 2025 16:24
@LunNova LunNova requested a review from dotlambda October 6, 2025 16:24
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 12.approvals: 1 This PR was reviewed and approved by one person. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Oct 6, 2025
@LunNova LunNova changed the title oneDNN_2: reintroduce, rocmPackages.migraphx: use oneDNN_2 oneDNN_2: reintroduce and unbreak, rocmPackages.migraphx: use oneDNN_2 Oct 6, 2025
Flakebi
Flakebi approved these changes Oct 6, 2025
View reviewed changes
Copy link
Member

@Flakebi Flakebi left a comment

Choose a reason for hiding this comment

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

Didn’t try to build, but the change looks good

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Oct 6, 2025
@LunNova LunNova requested a review from LordGrimmauld October 8, 2025 05:53
@LunNova
Copy link
Member Author

LunNova commented Oct 12, 2025

@dotlambda I'd appreciate if you can merge this PR

@dotlambda dotlambda added this pull request to the merge queue Oct 12, 2025
Merged via the queue into NixOS:master with commit 02001c1 Oct 12, 2025
36 of 38 checks passed
@LunNova LunNova deleted the push-ktynwzmznuxt branch October 21, 2025 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dotlambda dotlambda dotlambda approved these changes

@mschwaig mschwaig Awaiting requested review from mschwaig

@GZGavinZhao GZGavinZhao Awaiting requested review from GZGavinZhao

@LordGrimmauld LordGrimmauld Awaiting requested review from LordGrimmauld

+1 more reviewer

@Flakebi Flakebi Flakebi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: rocm ROCm is an Advanced Micro Devices software stack for graphics processing unit programming. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@LunNova @Sigmanificient @GZGavinZhao @Flakebi @dotlambda