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

pythonPackages.pytorch-lightning: init at 0.5.3.2 #77155

Closed
wants to merge 2 commits into from

Conversation

@tbenst
Copy link
Contributor

@tbenst tbenst commented Jan 7, 2020

Motivation for this change

Add PyTorch Lightning, the lightweight PyTorch wrapper for ML researchers

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 7, 2020

unfortunately, pytorch is broken right now, so this doesn't build

@tbenst
Copy link
Contributor Author

@tbenst tbenst commented Jan 7, 2020

No worries, can wait until the major python packages update is merged. It does build on my pull request branch (which is an old commit on master from Dec 12). Also, with cherry-pick patches we have this building on latest nixpkgs unstable.

Copy link
Contributor

@jonringer jonringer left a comment

otherwise LGTM

failures are from python38Packages.torch

[6 built (1 failed), 3 copied (9.1 MiB), 3.6 MiB DL]
error: build of '/nix/store/43mvmb2wnaqii07q1aqyz47jhxha43a4-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/77155
2 package failed to build:
python38Packages.pytorch-lightning python38Packages.test-tube

2 package built:
python37Packages.pytorch-lightning python37Packages.test-tube
description = "PyTorch Lightning is the lightweight PyTorch wrapper for ML researchers";
homepage = https://github.com/williamFalcon/pytorch-lightning;
license = licenses.asl20;
# maintainers = [ maintainers. ];

This comment has been minimized.

@jonringer

jonringer Jan 10, 2020
Contributor

please add yourself as a maintainer

doCheck = false;

# checkPhase = ''
# coverage run --source pytorch_lightning -m py.test pytorch_lightning tests pl_examples -v --doctest-modules

This comment has been minimized.

@jonringer

jonringer Jan 10, 2020
Contributor

you don't have to run all the tests

Suggested change
# coverage run --source pytorch_lightning -m py.test pytorch_lightning tests pl_examples -v --doctest-modules
py.test pytorch_lightning tests -k 'not this_network_test'
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Feb 4, 2020

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

https://discourse.nixos.org/t/allow-internet-access-during-checkphase/5754/1

@jonringer
Copy link
Contributor

@jonringer jonringer commented Feb 16, 2020

please squash fix-ups

@tbenst tbenst force-pushed the tbenst:pytorch-lightning branch from 38d03d9 to 27d0518 Feb 16, 2020
@tbenst
Copy link
Contributor Author

@tbenst tbenst commented Feb 16, 2020

Squashed & rebased for mlflow, building overnight. Likely need to exclude additional tests that require internet

Copy link
Contributor

@doronbehar doronbehar left a comment

nixpkgs-review fails with:

builder for '/nix/store/hpb8g409pf92l2zg1hwz6i9bkzin73r7-python3.7-pytorch-lightning-0.5.3.2.drv' failed with exit code 4; last 10 log lines:
  running install tests
  ============================= test session starts ==============================
  platform linux -- Python 3.7.7, pytest-5.3.5, py-1.8.1, pluggy-0.13.1 -- /nix/store/2dcsn57cgaxs92ha5swihrab0g3l2h6g-python3-3.7.7/bin/python3.7
  cachedir: .pytest_cache
  rootdir: /build/source, inifile: setup.cfg
  collected 0 items                                                              
  
  ============================ no tests ran in 0.00s =============================
  ERROR: file not found: test_running_test_pretrained_model
  
builder for '/nix/store/yray8z3h8lw93zsb5fb3c330zbrcqqzv-python3.8-pytorch-lightning-0.5.3.2.drv' failed with exit code 4; last 10 log lines:
  running install tests
  ============================= test session starts ==============================
  platform linux -- Python 3.8.2, pytest-5.3.5, py-1.8.1, pluggy-0.13.1 -- /nix/store/081w0sp7jnh33548cihj3a9y9jpn9b3s-python3-3.8.2/bin/python3.8
  cachedir: .pytest_cache
  rootdir: /build/source, inifile: setup.cfg
  collected 0 items                                                              
  
  ============================ no tests ran in 0.00s =============================
  ERROR: file not found: test_running_test_pretrained_model
  
cannot build derivation '/nix/store/6ybfn8l8xfcpdbpp7xi57yn6hrlybpk4-env.drv': 2 dependencies couldn't be built
[4 built (2 failed), 65 copied (1083.5 MiB), 202.0 MiB DL]
error: build of '/nix/store/6ybfn8l8xfcpdbpp7xi57yn6hrlybpk4-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/77155
2 packages failed to build:
python37Packages.pytorch-lightning python38Packages.pytorch-lightning

2 packages built:
python37Packages.test-tube python38Packages.test-tube
@bcdarwin
Copy link
Member

@bcdarwin bcdarwin commented Jun 17, 2020

Hi @tbenst -- any chance of updating this? If not I'll make a new PR.

@tbenst
Copy link
Contributor Author

@tbenst tbenst commented Jun 17, 2020

Hi @bcdarwin, would welcome the help! Please feel free to push to this PR or make a new PR, I believe the package is fully functional, and I've used it, but getting the tests running will require downloading pre-trained models & data ahead of time, and possibly patching the test code to avoid a download. If you want to chat realtime about this, feel free to ping me on the nix-data slack channel.

description = "PyTorch Lightning is the lightweight PyTorch wrapper for ML researchers. Scale your models. Write less boilerplate";
homepage = https://github.com/williamFalcon/pytorch-lightning;
license = licenses.asl20;
# maintainers = [ maintainers. ];

This comment has been minimized.

@bcdarwin

bcdarwin Jun 17, 2020
Member

maintainers should be set


meta = with lib; {
description = "PyTorch Lightning is the lightweight PyTorch wrapper for ML researchers. Scale your models. Write less boilerplate";
homepage = https://github.com/williamFalcon/pytorch-lightning;

This comment has been minimized.

@bcdarwin

bcdarwin Jun 17, 2020
Member

homepage URLs should be quoted now (in both packages)

@bcdarwin
Copy link
Member

@bcdarwin bcdarwin commented Jun 17, 2020

I added a couple cosmetic comments inline, but I guess the main thing is updating the package (0.7.6 is out) so that it can work with Pytorch 1.5.

@tbenst
Copy link
Contributor Author

@tbenst tbenst commented Jun 17, 2020

@bcdarwin, yes, updating will require adding several packages to nixpkgs: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/environment.yml. In particular, trains, Neptune, wandb, and comet ML, although as I did in 0.5.3.2 we can potentially remove imports that are unused (I maintain mlflow but don’t have interest in the others). But than you for raising this point, doesn’t make sense to add tests to this old version.

In fact, I might propose that we skip tests on this package for now if others are ok with it, as the maintenance burden is high given the rate of releases, change in testing environment, and high utilization of network to download pre-trained models and datasets. Or if you or someone else wants to take a stab at getting tests working, I’m happy to help but I don’t have bandwidth to take on right now

@bcdarwin bcdarwin mentioned this pull request Aug 20, 2020
3 of 7 tasks complete
@bcdarwin
Copy link
Member

@bcdarwin bcdarwin commented Aug 21, 2020

Pytorch-lightning was merged in #95827 -- probably still worth it to merge test-tube.

@tbenst
Copy link
Contributor Author

@tbenst tbenst commented Aug 21, 2020

Thanks for pushing that over the finish line! I know it’s pretty simple to get us to merge test tube at this point but I don’t have any bandwidth right now

@bcdarwin bcdarwin mentioned this pull request Aug 23, 2020
3 of 8 tasks complete
@bcdarwin
Copy link
Member

@bcdarwin bcdarwin commented Aug 23, 2020

Closing in favour of #96119.

@bcdarwin bcdarwin closed this Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.