python312Packages.dgl: init at 1.1.3 #319786

dtsykunov · 2024-06-14T09:12:17Z

Description of changes

Python package built to ease deep learning on graph, on top of existing DL frameworks.

https://www.dgl.ai/
https://github.com/dmlc/dgl/

Things done

Add a 👍 reaction to pull requests you find important.

drupol · 2024-06-16T20:33:01Z

pkgs/development/python-modules/dgl/default.nix

@@ -0,0 +1,261 @@
+{ buildPythonPackage


Please reformat this new file with nixfmt-rfc-style.

drupol · 2024-06-16T20:33:18Z

pkgs/development/python-modules/dgl/default.nix

+    owner = "dmlc";
+    repo = pname;
+    rev = "v${version}";
+    sha256 = "sha256-FQxfyHPvqI1x4b3byCrTAf11El2MNOXtb5/jx66Yve4=";


Suggested change

sha256 = "sha256-FQxfyHPvqI1x4b3byCrTAf11El2MNOXtb5/jx66Yve4=";

hash = "sha256-FQxfyHPvqI1x4b3byCrTAf11El2MNOXtb5/jx66Yve4=";

drupol · 2024-06-16T20:33:28Z

pkgs/development/python-modules/dgl/default.nix

+
+  src = fetchFromGitHub {
+    owner = "dmlc";
+    repo = pname;


Please don't use this.

repo = pname; is nice for DRY but creates a binding that goes too far.

See further information about this here: nix-community/nixpkgs-lint#21

drupol · 2024-06-16T20:33:42Z

pkgs/development/python-modules/dgl/default.nix

+    ./dlpack_convert.patch
+  ];
+
+  propagatedBuildInputs = [


Suggested change

propagatedBuildInputs = [

dependencies = [

drupol · 2024-06-16T20:33:52Z

pkgs/development/python-modules/dgl/default.nix

+  };
+
+  patches = [
+    ./dlpack_convert.patch


Each patch in nixpkgs applied to the upstream source should be documented. Out-of-tree patches, fetched using fetchpatch, are preferred.

In each patch comment, please explain the purpose of the patch and link to the relevant upstream issue if possible. If the patch has been merged upstream but is not yet part of the released version, please note the version number or date in the comment such that a future maintainer updating the nix expression will know whether the patch has been incorporated upstream and can thus be removed from nixpkgs.

Furthermore, please use a stable URL for the patch. Rather than, for example, linking to a GitHub pull request of the form https://github.com/owner/repo/pull/pr_number.patch, which would change every time a commit is added or the PR is force-pushed, link to a specific commit patch in the form https://github.com/owner/repo/commit/sha.patch.

Here are two good examples of patch comments:

mkDerivation { … patches = [ # Ensure RStudio compiles against R 4.0.0. # Should be removed next 1.2.X RStudio update or possibly 1.3.X. (fetchpatch { url = "https://github.com/rstudio/rstudio/commit/3fb2397c2f208bb8ace0bbaf269481ccb96b5b20.patch"; sha256 = "0qpgjy6aash0fc0xbns42cwpj3nsw49nkbzwyq8az01xwg81g0f3"; }) ]; }

mkDerivation { … patches = [ # Allow building with bison 3.7 # PR at https://github.com/GoldenCheetah/GoldenCheetah/pull/3590 (fetchpatch { url = "https://github.com/GoldenCheetah/GoldenCheetah/commit/e1f42f8b3340eb4695ad73be764332e75b7bce90.patch"; sha256 = "1h0y9vfji5jngqcpzxna5nnawxs77i1lrj44w8a72j0ah0sznivb"; }) ]; }

Unfortunately, there is no patch for this in the repo for dgl. However, I was able to get rid of this patch by providing a CXXFLAGS environment variable to the build.

dtsykunov · 2024-06-17T20:57:05Z

@drupol Thank you very much for the review. I believe I addressed all the notes.
Please, take a look again when you get the chance.

natsukium · 2024-06-18T13:11:45Z

pkgs/development/python-modules/dgl/default.nix

+}:
+let
+  pname = "dgl";
+  version = "1.1.3";


Why do you adopt 1.1.3?
2.2.1 has already been released.
https://github.com/dmlc/dgl/releases/tag/v2.2.1

Because starting from version 2.0.0 dgl requires torchdata package which isn't present in nixpkgs.

I'll go ahead and try to revive this PR:
#187779

natsukium · 2024-06-18T13:24:08Z

pkgs/development/python-modules/dgl/default.nix

+{
+  buildPythonPackage,
+  cmake,
+  cudaPackages,


Suggested change

cudaPackages,

config,

cudaPackages,

cudaSupport ? config.cudaSupport,

cudaSupport should be optional.
This package does not necessarily require CUDA.

natsukium · 2024-06-18T13:30:50Z

pkgs/development/python-modules/dgl/default.nix

+  # tensorflow-build and torch use different grpcio, tensorboard and protobuf versions
+  # so they can't be used at the same time
+  withTorch ? true,
+  withTensorflow ? false,


Suggested change

# tensorflow-build and torch use different grpcio, tensorboard and protobuf versions

# so they can't be used at the same time

withTorch ? true,

withTensorflow ? false,

In the python library, we do not allow such arguments since it is easy to conflict PYTHONPATH with different derivatives.

natsukium · 2024-06-18T13:33:01Z

pkgs/development/python-modules/dgl/default.nix

+    "test_cluster_gcn[0]"
+    "test_cluster_gcn[4]"
+    "test_dataloader_worker_init_fn"
+    "test_graph_dataloader[16]"
+    "test_graph_dataloader[None]"
+    "test_saint[edge-0]"
+    "test_saint[edge-4]"
+    "test_saint[node-0]"
+    "test_saint[node-4]"
+    "test_saint[walk-0]"
+    "test_saint[walk-4]"
+    "test_shadow[0]"
+    "test_shadow[4]"
+    "test_sparse_opt"


Suggested change

"test_cluster_gcn[0]"

"test_cluster_gcn[4]"

"test_dataloader_worker_init_fn"

"test_graph_dataloader[16]"

"test_graph_dataloader[None]"

"test_saint[edge-0]"

"test_saint[edge-4]"

"test_saint[node-0]"

"test_saint[node-4]"

"test_saint[walk-0]"

"test_saint[walk-4]"

"test_shadow[0]"

"test_shadow[4]"

"test_sparse_opt"

"test_cluster_gcn"

"test_dataloader_worker_init_fn"

"test_graph_dataloader"

"test_saint"

"test_shadow"

"test_sparse_opt"

It is not necessary to list all of the parameters.

You can use the -k command line option to specify an expression which implements a substring match on the test names instead of the exact match on markers that -m provides.
https://docs.pytest.org/en/7.1.x/example/markers.html#using-k-expr-to-select-tests-based-on-their-name

natsukium · 2024-06-18T13:34:52Z

pkgs/development/python-modules/dgl/default.nix

+    "test_adj[idtype0]"
+    "test_adj[idtype1]"


natsukium · 2024-06-18T13:53:32Z

pkgs/development/python-modules/dgl/default.nix

+  buildPhase = ''
+    runHook preBuild
+
+    make -j $NIX_BUILD_CORES
+
+    runHook postBuild
+  '';
+


Suggested change

buildPhase = ''

runHook preBuild

make -j $NIX_BUILD_CORES

runHook postBuild

'';

By default, parallel building is enabled as CMake supports parallel building almost everywhere.
https://nixos.org/manual/nixpkgs/unstable/#cmake

natsukium · 2024-06-18T13:56:45Z

pkgs/development/python-modules/dgl/default.nix

+  doCheck = true;
+


Suggested change

doCheck = true;

buildPythonPackage function has doCheck = true; by default.

natsukium · 2024-06-18T13:58:57Z

pkgs/development/python-modules/dgl/default.nix

+  checkPhase =
+    ''
+      runHook preCheck
+
+      export HOME="$(mktemp -d)"
+
+      cd ..
+
+      ./build/runUnitTests
+
+      export PYTHONPATH="$PYTHONPATH:$out/${python.sitePackages}:$src/tests"
+      export PATH="$PATH:$out/bin"
+    ''
+    + lib.optionalString withTorch ''
+      export DGLBACKEND=pytorch
+      pytest tests/python/pytorch/ -k '${disabledTestString pytorchDisabledTests}' \
+       --ignore=tests/python/pytorch/sparse/
+      pytest tests/python/common/ -k '${disabledTestString commonDisabledTests}'
+
+      pytest tests/distributed/ -k '${disabledTestString distributedDisabledTests}'
+    ''
+    +
+      lib.optionalString (withTensorflow && withTorch) # should be withTensorflow, but you need 'import torch' to run these
+        ''
+          export DGLBACKEND=tensorflow
+          pytest tests/python/tensorflow/
+          pytest tests/python/common/ -k '${disabledTestString commonDisabledTests}'
+        ''
+    + ''
+      runHook postCheck
+    '';


Can we use pytestCheckHook instead?
https://nixos.org/manual/nixpkgs/unstable/#using-pytestcheckhook

natsukium · 2024-06-18T14:00:39Z

pkgs/development/python-modules/dgl/default.nix

+    description = ''
+      Python package built to ease deep learning on graph, on top of existing DL frameworks.
+    '';


Suggested change

description = ''

Python package built to ease deep learning on graph, on top of existing DL frameworks.

'';

description = "Python package built to ease deep learning on graph, on top of existing DL frameworks";

Please remove the trailing period.
https://nixos.org/manual/nixpkgs/unstable/#var-meta-description

natsukium · 2024-06-18T14:01:47Z

pkgs/development/python-modules/dgl/default.nix

+    description = ''
+      Python package built to ease deep learning on graph, on top of existing DL frameworks.
+    '';
+    homepage = "https://www.dgl.ai/";


Suggested change

homepage = "https://www.dgl.ai/";

homepage = "https://www.dgl.ai/";

changelog = "https://github.com/dmlc/dgl/releases/tag/${src.rev}"

Adding meta.changelog should be useful for future updates.

github-actions bot added 6.topic: python 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Jun 14, 2024

NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 14, 2024

ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jun 14, 2024

drupol requested changes Jun 16, 2024

View reviewed changes

dtsykunov added 2 commits June 17, 2024 22:53

maintainers: add dtsykunov

90f09ab

python312Packages.dgl: init at 1.1.3

dc015d6

dtsykunov force-pushed the init_dgl branch from 33929ff to dc015d6 Compare June 17, 2024 20:53

dtsykunov requested a review from drupol June 17, 2024 20:58

natsukium requested changes Jun 18, 2024

View reviewed changes

FliegendeWurst added the awaiting_changes (old Marvin label, do not use) label Dec 1, 2024

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025

SigmaSquadron removed the awaiting_changes (old Marvin label, do not use) label Jan 5, 2025

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 5, 2025

donovanglover marked this pull request as draft February 7, 2025 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python312Packages.dgl: init at 1.1.3 #319786

python312Packages.dgl: init at 1.1.3 #319786

dtsykunov commented Jun 14, 2024

drupol Jun 16, 2024

drupol Jun 16, 2024

drupol Jun 16, 2024

drupol Jun 16, 2024

drupol Jun 16, 2024

dtsykunov Jun 17, 2024

dtsykunov commented Jun 17, 2024

natsukium Jun 18, 2024

dtsykunov Jun 19, 2024 •

edited

Loading

natsukium Jun 18, 2024

natsukium Jun 18, 2024

natsukium Jun 18, 2024

natsukium Jun 18, 2024

natsukium Jun 18, 2024

natsukium Jun 18, 2024

natsukium Jun 18, 2024

natsukium Jun 18, 2024

natsukium Jun 18, 2024

	sha256 = "sha256-FQxfyHPvqI1x4b3byCrTAf11El2MNOXtb5/jx66Yve4=";
	hash = "sha256-FQxfyHPvqI1x4b3byCrTAf11El2MNOXtb5/jx66Yve4=";

	homepage = "https://www.dgl.ai/";
	homepage = "https://www.dgl.ai/";
	changelog = "https://github.com/dmlc/dgl/releases/tag/${src.rev}"

python312Packages.dgl: init at 1.1.3 #319786

Are you sure you want to change the base?

python312Packages.dgl: init at 1.1.3 #319786

Conversation

dtsykunov commented Jun 14, 2024

Description of changes

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtsykunov commented Jun 17, 2024

Choose a reason for hiding this comment

dtsykunov Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtsykunov Jun 19, 2024 •

edited

Loading