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

cudaPackages: drop outdated aliases #331017

Merged
merged 2 commits into from
Jul 31, 2024
Merged

Conversation

srhb
Copy link
Contributor

@srhb srhb commented Jul 30, 2024

Description of changes

Related to #306276

These traces are annoying in a lot of cases, and I think we should be good to remove them by now. I nuked all of aliases.nix for cuda-modules while I was at it. If this is heavy-handed, let me know.

For me personally, I could also live with removing this check in torch, which ends up triggering every alias in cudaPackages because of the equality check;

cudaSupport && (effectiveMagma.cudaPackages != cudaPackages);

But one or the other has to go. 😸

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.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.

@srhb srhb marked this pull request as draft July 30, 2024 05:58
@srhb srhb force-pushed the drop-cuda-aliases branch from 8a0fa5d to 6c3eb7b Compare July 30, 2024 05:59
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jul 30, 2024
@srhb srhb marked this pull request as ready for review July 30, 2024 07:08
@srhb srhb marked this pull request as draft July 30, 2024 07:09
@srhb srhb force-pushed the drop-cuda-aliases branch from 656eb7b to 4191cf3 Compare July 30, 2024 07:18
@SomeoneSerge
Copy link
Contributor

  • 👍🏻 for removing the deprecated attributes, it's time
  • 👎🏻 for removing aliases.nix
  • How about we replace the comparison in pytorch with a comparison between cudaPackages.cudaVersions? The motivation of the check was to ensure people overriding one package (magma) don't forget to override the other. For simple cases, testing cudaVersion would be enough, and for more complex cases well people have to take care. The package set comparison is super annoying because it needlessly forces evaluation

I'd also maybe start with the last item and check if there are any other components forcing evaluation to reach the traces.

@srhb srhb changed the title cuda-packages: drop aliases cuda-packages: drop outdated aliases Jul 30, 2024
@srhb
Copy link
Contributor Author

srhb commented Jul 30, 2024

@SomeoneSerge Oops, I raced you. Sure, let me do the version check instead.

@srhb srhb force-pushed the drop-cuda-aliases branch 2 times, most recently from 409a556 to ab2d64d Compare July 30, 2024 07:24
@srhb srhb marked this pull request as ready for review July 30, 2024 07:25
@srhb srhb force-pushed the drop-cuda-aliases branch from ab2d64d to 2ee7a31 Compare July 30, 2024 07:26
@srhb
Copy link
Contributor Author

srhb commented Jul 30, 2024

@SomeoneSerge Alright, how's this? I had to jump some hoops to allow the empty aliases infrastructure to pass CI checks, but nothing too bad I suppose.

@srhb srhb force-pushed the drop-cuda-aliases branch from 2ee7a31 to 4fb4b3f Compare July 30, 2024 09:27
@srhb srhb force-pushed the drop-cuda-aliases branch from 4fb4b3f to 26ca4c1 Compare July 30, 2024 10:03
Comment on lines 7 to 11
# let
# mkRenamed =
# oldName: newName: newPkg:
# final.lib.warn "cudaPackages.${oldName} is deprecated, use ${newName} instead" newPkg;
# in
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, here we go: #331085

@SomeoneSerge SomeoneSerge force-pushed the drop-cuda-aliases branch 2 times, most recently from 3632982 to 6e7b739 Compare July 30, 2024 17:13
@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-linux: 1 10.rebuild-linux: 1-10 labels Jul 30, 2024
@SomeoneSerge SomeoneSerge changed the title cuda-packages: drop outdated aliases cudaPackages: drop outdated aliases Jul 31, 2024
@SomeoneSerge SomeoneSerge merged commit 5cc4b22 into NixOS:master Jul 31, 2024
24 of 27 checks passed
@srhb srhb deleted the drop-cuda-aliases branch August 1, 2024 08:01
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/enabling-nixpkgs-config-cudasupport-leads-to-oom-during-rebuild/50624/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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