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

magma: 2.0.2 -> 2.5.0 #61347

Merged
merged 1 commit into from Sep 6, 2019
Merged

magma: 2.0.2 -> 2.5.0 #61347

merged 1 commit into from Sep 6, 2019

Conversation

tbenst
Copy link
Contributor

@tbenst tbenst commented May 12, 2019

Motivation for this change

Without magma, many linalg functions in pytorch are broken on GPU. I also had to update magma to support newer versions of CUDA.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@tbenst
Copy link
Contributor Author

tbenst commented May 13, 2019

@wavewave let me know if you have any input! (If you are the former “ianwookim” that is)

@tbenst tbenst mentioned this pull request May 14, 2019
@tbenst tbenst mentioned this pull request Jun 16, 2019
@stites stites mentioned this pull request Jul 18, 2019
10 tasks
@stites
Copy link
Member

stites commented Jul 25, 2019

Heads-up, MAGMA also allows for building with MKL support and I might try to shoe-horn this build optimization into #65041 or layer on an additional PR here.

@tbenst
Copy link
Contributor Author

tbenst commented Aug 30, 2019

Now that PyTorch 1.2 #65041 (comment) is ready, this is blocking. Would someone be able to review? Thanks!

Edit: I plan to remove the changes to pytorch, and just have the addition of magma, but would be great to make all changes in one swoop

@stites
Copy link
Member

stites commented Aug 30, 2019

It's worth pointing out that I have been using this code and running the full pytorch test suite with it (on cudatoolkit 10 and 9.2), so you can consider this functional and tested. Code changes get a +1 from me.

@wavewave
Copy link
Contributor

@tbenst thank you for upgrading this! 👍

@tbenst
Copy link
Contributor Author

tbenst commented Aug 31, 2019

Thanks @FRidh your suggestion worked. @wavewave should I update your handle from ianwookin to wavewave?

@ofborg ofborg bot removed the 6.topic: python label Aug 31, 2019
@tbenst tbenst changed the title pytorch: build with magma magma: 2.0.2 -> 2.5.0 Aug 31, 2019
@ofborg ofborg bot requested a review from wavewave August 31, 2019 19:08
Copy link
Contributor

@wavewave wavewave left a comment

Choose a reason for hiding this comment

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

I checked this works. btw, please take the maintainership since I am not very familar with recent updates of magma. :-)
(and it is more desirable to be listed as ianwookim as listed in /maintainers/maintainer-list.nix.)
Thanks again! @tbenst

@FRidh
Copy link
Member

FRidh commented Sep 2, 2019

undefined variable 'wavewave' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-2-shlevy.ewr1.nix.ci/pkgs/development/libraries/science/math/magma/default.nix:46:39

@tbenst
Copy link
Contributor Author

tbenst commented Sep 4, 2019

@wavewave @FRidh done!

@tbenst
Copy link
Contributor Author

tbenst commented Sep 4, 2019

Just reviewed #65041 and realized that @stites's excellent work building on-top of this pull request for adding MKL might best be added here. Let me know if you want to be added as a maintainer?

@stites
Copy link
Member

stites commented Sep 5, 2019

I'm probably not a good maintainer for magma -- it's a slightly magical build system which I haven't fully figured out. I'd be happy to be added as a maintainer as a second pair of eyes, however.

@tbenst
Copy link
Contributor Author

tbenst commented Sep 5, 2019

Ok, just wanted to make sure that I acknowledged your contributions to this pull request :). In that case I think this is good to go!

@FRidh FRidh merged commit 6d7282d into NixOS:master Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants