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

added pythonPackages.tensorboardX #52939

Merged
merged 3 commits into from Jun 30, 2020
Merged

added pythonPackages.tensorboardX #52939

merged 3 commits into from Jun 30, 2020

Conversation

akamaus
Copy link
Contributor

@akamaus akamaus commented Dec 26, 2018

Motivation for this change

Added an expression for tensorboardX, a standalone library for logging data for subsequent visualization by tensorboard.

Things done

@akamaus akamaus requested a review from FRidh as a code owner December 26, 2018 19:07
FRidh
FRidh requested changes Jan 3, 2019
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Please use the full PR template, it's there for a reason.

maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/development/python-modules/tensorboardX/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/tensorboardX/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/tensorboardX/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/tensorboardX/default.nix Outdated Show resolved Hide resolved
@akamaus
Copy link
Contributor Author

akamaus commented Jan 4, 2019

@FRidh all done

FRidh
FRidh requested changes Jan 4, 2019
View reviewed changes
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
@FRidh
Copy link
Member

FRidh commented Jan 4, 2019

@GrahamcOfBorg build python3.pkgs.tensorboardX

@akamaus
Copy link
Contributor Author

akamaus commented Jan 7, 2019

@FRidh renamed to lowercase.

@mmahut
Copy link
Member

mmahut commented Aug 13, 2019

Are there any updates on this pull request, please?

@FRidh
Copy link
Member

FRidh commented Aug 13, 2019

@GrahamcOfBorg eval

FRidh
FRidh reviewed Aug 13, 2019
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Looks ok

@akamaus akamaus force-pushed the tensorboardX branch 8 times, most recently from 9488c0a to 53c410f Compare August 21, 2019 20:52
@akamaus
Copy link
Contributor Author

akamaus commented Aug 23, 2019

@FRidh, please have a look.

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@akamaus akamaus requested a review from jonringer as a code owner June 24, 2020 09:38
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 24, 2020
@akamaus akamaus requested a review from FRidh June 24, 2020 09:39
@akamaus
Copy link
Contributor Author

akamaus commented Jun 24, 2020

Updated PR to latest tensorboardX-2.0, apparently, previosly failed tests were fixed upstream, so now they all pass (besides two, one for Visdom integration which is missing from nixpkgs and one trying download something with wget. All builds and works nicely for me.

jonringer
jonringer reviewed Jun 24, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

I think it also makes sense to squash all of the tensorboard commits, and have just a single python3Packages.tensorboardx: init at 2.0.

From the perspective master branch, the meaningful change is that a package was added, the intermediate bumps are not very consequential (at least to me).

maintainers/maintainer-list.nix Show resolved Hide resolved
@Mic92
Copy link
Member

Mic92 commented Jun 24, 2020

I think @mweinelt has an interest in getting this merged.

@akamaus
Copy link
Contributor Author

akamaus commented Jun 26, 2020

@Mic92 commits squashed, githubId added.

mweinelt
mweinelt requested changes Jun 26, 2020
View reviewed changes
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

That's a great start, I only added some style remarks.

pkgs/development/python-modules/tensorboardx/default.nix Outdated
@@ -0,0 +1,30 @@
{boto3, buildPythonPackage, crc32c, fetchFromGitHub, lib, matplotlib, moto, numpy, pillow, pytorch, protobuf, six, pytest, tensorflow-tensorboard, torchvision }:
Copy link
Member

Choose a reason for hiding this comment

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

Can you wrap this line? It is quite a bit too long to properly read on GitHub.

pkgs/development/python-modules/tensorboardx/default.nix Outdated

meta = {
description = "Library for writing tensorboard-compatible logs";
homepage = https://github.com/lanpa/tensorboard-pytorch;
Copy link
Member

Choose a reason for hiding this comment

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

The URL needs to be quoted as per RFC 45: https://github.com/NixOS/rfcs/blob/master/rfcs/0045-deprecate-url-syntax.md

pkgs/development/python-modules/tensorboardx/default.nix Outdated
substituteInPlace tests/test_onnx_graph.py --replace test_onnx_graph _skip_test_onnx_graph
'';

meta = {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = {
meta = with lib; {

And then cut off lib. on license, maintainers and platforms.

pkgs/development/python-modules/tensorboardx/default.nix Outdated
postPatch=''
substituteInPlace tests/test_visdom.py --replace test_TorchVis _skip_test_TorchVis
substituteInPlace tests/test_onnx_graph.py --replace test_onnx_graph _skip_test_onnx_graph
'';
Copy link
Member

Choose a reason for hiding this comment

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

The indentation here seems off.

@akamaus
Copy link
Contributor Author

akamaus commented Jun 28, 2020

@mweinelt, thanks for tips for fixing style. All done.

@Mic92
Copy link
Member

Mic92 commented Jun 30, 2020

@GrahamcOfBorg build python3.pkgs.tensorboardX

@Mic92
Copy link
Member

Mic92 commented Jun 30, 2020

@GrahamcOfBorg build python3.pkgs.tensorboardx

@Mic92 Mic92 merged commit 74cb17f into NixOS:master Jun 30, 2020
@akamaus akamaus deleted the tensorboardX branch June 30, 2020 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmahut mmahut mmahut left review comments

@jonringer jonringer jonringer left review comments

@mweinelt mweinelt mweinelt requested changes

@FRidh FRidh Awaiting requested review from FRidh

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@akamaus @FRidh @mmahut @Mic92 @mweinelt @jonringer @GrahamcOfBorg