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

python312Packages.dgl: init at 1.1.3 #319786

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+269 −0
Draft

python312Packages.dgl: init at 1.1.3 #319786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90f09ab
maintainers: add dtsykunov
dtsykunov Jun 6, 2024
dc015d6
python312Packages.dgl: init at 1.1.3
dtsykunov Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions maintainers/maintainer-list.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5619,6 +5619,12 @@
github = "dsymbol";
githubId = 88138099;
};
dtsykunov = {
name = "Dmitrii Tsykunov";
email = "[email protected]";
github = "dtsykunov";
githubId = 73191463;
};
dtzWill = {
email = "[email protected]";
github = "dtzWill";
Expand Down
261 changes: 261 additions & 0 deletions pkgs/development/python-modules/dgl/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,261 @@
{
buildPythonPackage,
cmake,
cudaPackages,
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
cudaPackages,
config,
cudaPackages,
cudaSupport ? config.cudaSupport,

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

cython,
fetchFromGitHub,
gtest,
keras,
lib,
networkx,
numpy,
pandas,
pip,
psutil,
pydantic,
pytest,
python,
requests,
scikit-learn,
scipy,
# 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,
Comment on lines +21 to +24
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
# 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.

tensorflow,
torch,
tqdm,
}:
let
pname = "dgl";
version = "1.1.3";
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@dtsykunov dtsykunov Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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


# All of these tests fail because they use network.
pytorchDisabledTests = [
"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"
Comment on lines +35 to +48
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
"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

];

commonDisabledTests = [
"test_actor"
"test_add_node_property_split"
"test_add_nodepred_split"
"test_adj[idtype0]"
"test_adj[idtype1]"
Comment on lines +55 to +56
Copy link
Member

Choose a reason for hiding this comment

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

ditto

"test_as_graphpred"
"test_as_graphpred_ogb"
"test_as_graphpred_reprocess"
"test_as_linkpred"
"test_as_linkpred_ogb"
"test_as_nodepred1"
"test_as_nodepred2"
"test_as_nodepred_ogb"
"test_chameleon"
"test_citation_graph"
"test_cluster"
"test_cornell"
"test_explain_syn"
"test_fakenews"
"test_flickr"
"test_fraud"
"test_gather_mm_idx_b[dtype1-0.02-256]"
"test_gin"
"test_global_uniform_negative_sampling[int32]"
"test_global_uniform_negative_sampling[int64]"
"test_gnn_benchmark"
"test_pattern"
"test_reddit"
"test_squirrel"
"test_texas"
"test_tudataset_regression"
"test_wiki_cs"
"test_wisconsin"
"test_zinc"
];

distributedDisabledTests = [
"test_dataloader[edge-0-3]"
"test_dataloader[edge-4-3]"
"test_dataloader[node-0-3]"
"test_dataloader[node-4-3]"
"test_dist_dataloader[1-False-0-3]"
"test_dist_dataloader[1-False-4-3]"
"test_dist_dataloader[1-True-0-3]"
"test_dist_dataloader[1-True-4-3]"
"test_dist_emb_server_client"
"test_dist_optim_server_client"
"test_kv_multi_role"
"test_kv_store"
"test_multi_client[socket]"
"test_multi_client[tensorpipe]"
"test_multi_client_connect[socket]"
"test_multi_client_connect[tensorpipe]"
"test_multi_thread_rpc[socket]"
"test_multi_thread_rpc[tensorpipe]"
"test_multiple_dist_dataloaders[edge-0-1]"
"test_multiple_dist_dataloaders[edge-0-4]"
"test_multiple_dist_dataloaders[edge-1-1]"
"test_multiple_dist_dataloaders[edge-1-4]"
"test_multiple_dist_dataloaders[edge-4-1]"
"test_multiple_dist_dataloaders[edge-4-4]"
"test_multiple_dist_dataloaders[node-0-1]"
"test_multiple_dist_dataloaders[node-0-4]"
"test_multiple_dist_dataloaders[node-1-1]"
"test_multiple_dist_dataloaders[node-1-4]"
"test_multiple_dist_dataloaders[node-4-1]"
"test_multiple_dist_dataloaders[node-4-4]"
"test_neg_dataloader[0-3]"
"test_neg_dataloader[4-3]"
"test_rpc[tensorpipe]"
"test_rpc_find_edges_shuffle[1]"
"test_rpc_find_edges_shuffle[2]"
"test_rpc_get_degree_shuffle[1]"
"test_rpc_get_degree_shuffle[2]"
"test_rpc_in_subgraph"
"test_rpc_sampling_shuffle[1]"
"test_rpc_sampling_shuffle[2]"
"test_rpc_timeout[socket]"
"test_rpc_timeout[tensorpipe]"
"test_server_client"
"test_standalone"
"test_standalone_etype_sampling"
"test_standalone_sampling"
Comment on lines +89 to +134
Copy link
Member

Choose a reason for hiding this comment

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

ditto

];

disabledTestString = testsList: "not (${lib.strings.concatStringsSep " or " testsList})";
in

buildPythonPackage {
inherit pname version;

format = "other";

src = fetchFromGitHub {
owner = "dmlc";
repo = "dgl";
rev = "v${version}";
hash = "sha256-FQxfyHPvqI1x4b3byCrTAf11El2MNOXtb5/jx66Yve4=";
fetchSubmodules = true;
};

dependencies =
[
networkx
numpy
pandas
psutil
pydantic
requests
scikit-learn
scipy
tqdm
]
++ lib.optionals withTorch [ torch ]
++ lib.optionals withTensorflow [
keras
tensorflow
Comment on lines +164 to +168
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
]
++ lib.optionals withTorch [ torch ]
++ lib.optionals withTensorflow [
keras
tensorflow

I don't think it requires either torch or tensorflow.
https://github.com/dmlc/dgl/blob/2a1ac588d5060908b8ff6dd71de5e13a51216fe2/python/setup.py#L222-L231

The user should be able to use python3.withPackages (ps: with ps; [ dgl torch ])

];

nativeCheckInputs = [ pytest ];

env = {
# error: 'uintptr_t' in namespace 'std' does not name a type
CXXFLAGS = "-include cstdint";

# error: ‘iadjwgt’ may be used uninitialized
NIX_CFLAGS_COMPILE = "-Wno-error=maybe-uninitialized";
};

cmakeFlags = [
"-DBUILD_CPP_TEST=ON"
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
"-DBUILD_CPP_TEST=ON"
lib.cmakeBool "BUILD_CPP_TEST" doCheck;

Tests should be able to be turned off.

"-DBUILD_SPARSE=OFF"
"-DUSE_LIBXSMM=OFF"
];

nativeBuildInputs = [
cmake
cython
pip
];

buildInputs = [
cudaPackages.cudatoolkit
Copy link
Member

@natsukium natsukium Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please use cudaPackages.nvcc, cudaPackages.cuda_cudart, and so on.
ref. #232501

e.g.

  nativeBuildInputs = lib.optionals cudaSupport [ cudaPackages.nvcc ];

  buildInputs = lib.optionals cudaSupport (with cudaPackages; [
    cuda_cudart
    ...
  ]);

gtest
Copy link
Member

Choose a reason for hiding this comment

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

Since gtest is a check dependency, it should go to checkInputs.

];

buildPhase = ''
runHook preBuild

make -j $NIX_BUILD_CORES

runHook postBuild
'';

Comment on lines +198 to +205
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
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

installPhase = ''
runHook preInstall

mkdir -p $out

pushd ../python
python3 -m pip install . --prefix=$out --no-index
python3 setup.py build_ext --inplace
popd

runHook postInstall
'';

doCheck = true;

Comment on lines +219 to +220
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
doCheck = true;

buildPythonPackage function has doCheck = true; by default.

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
'';
Comment on lines +221 to +251
Copy link
Member

Choose a reason for hiding this comment

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

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


meta = with lib; {
description = ''
Python package built to ease deep learning on graph, on top of existing DL frameworks.
'';
Comment on lines +254 to +256
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
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

homepage = "https://www.dgl.ai/";
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
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.

maintainers = [ maintainers.dtsykunov ];
license = licenses.asl20;
};
}
2 changes: 2 additions & 0 deletions pkgs/top-level/python-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2998,6 +2998,8 @@ self: super: with self; {

dfdiskcache = callPackage ../development/python-modules/dfdiskcache { };

dgl = callPackage ../development/python-modules/dgl { };

diagrams = callPackage ../development/python-modules/diagrams { };

diceware = callPackage ../development/python-modules/diceware { };
Expand Down