python3.pkgs.mmcv: init at 1.7.1

[benxiao]

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.05 Release Notes (or backporting 22.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

[kirillrdy]

1.7.1 was released on Jan 3rd, but I wouldn't want to block this MR because of it

[SuperSandro2000]

That's 3 months ago. If we introduce a new package and it is a trivial update, we should do that right away unless there is a reason not to

[benxiao]

updated.

[kirillrdy]

last commit message doesnt meet guidelines,

since this is an init PR, I would just squash all the commits

[kirillrdy]

as per contribution guidelines, description should not start or contain name of the package

[benxiao]

fixed

[bcdarwin]

I don't think the use of torch-bin and torchvision-bin is desirable here. We have torch{With,Without}Cuda and also torch itself can have cuda enabled or not depending on config.enableCuda or overriding torch.

[benxiao]

I might use torch torch vision, and let the user overwrite it in a flake file.

[SuperSandro2000]

If someone might do that, they can always overwrite the torch input to this package

[benxiao]

fixed.

[bcdarwin]

I don't think we should have a single package depending on cuda arch settings like this. We already have e.g. config.cudaCapabilities for configuring what architectures cuda-enabled packages are built for.

[benxiao]

I will try to use torch/default.nix as an example

[benxiao]

done

[bcdarwin]

it shouldn't be necessarily for torch/torchvision to be in nativeBuildInputs and propagatedBuildInputs.

[benxiao]

@bcdarwin torch is needed at runtime. not sure there is another way to do it without propagatedBuildInputs

[ConnorBaker]

Not that I'd consider this a blocker for this PR, but for future work consider using cudaPackages over cudatoolkit. cudaPackages uses redistributables from NVIDIA and allows for much smaller closures because you choose which libraries you need. It also makes it easier to maintain because we then have greater visibility into what the package is using.

For an example, take a look at

nixpkgs/pkgs/development/python-modules/torchvision/default.nix

Lines 17 to 40 in 7f194c7

	inherit (torch) cudaCapabilities cudaPackages cudaSupport;
	inherit (cudaPackages) cudatoolkit cudaFlags cudaVersion;
	# NOTE: torchvision doesn't use cudnn; torch does!
	# For this reason it is not included.
	cuda-common-redist = with cudaPackages; [
	cuda_cccl # <thrust/*>
	libcublas # cublas_v2.h
	libcusolver # cusolverDn.h
	libcusparse # cusparse.h
	];
	cuda-native-redist = symlinkJoin {
	name = "cuda-native-redist-${cudaVersion}";
	paths = with cudaPackages; [
	cuda_cudart # cuda_runtime.h
	cuda_nvcc
	] ++ cuda-common-redist;
	};
	cuda-redist = symlinkJoin {
	name = "cuda-redist-${cudaVersion}";
	paths = cuda-common-redist;
	};

.

I break the dependencies into those needed only at build time, run time, or both (common). Unfortunately, because most packages expect all the CUDA libraries to be in a single directly, we need to use symlinkJoin to group them together.

Also, make sure you're specifying the right C/C++ compilers if you're building with CUDA support, like here: #218265. (Torchvision doesn't yet do that; I've got an open issue here: #221898.)

The compiler provided by stdenv isn't necessarily supported by NVCC, and it's possible to get odd symbol errors during linking if the other derivations were built with a different compiler or language standard. That's why it's important to use backendStdenv.cc.

Torchvision and Torch will probably have other things you might want to look at. If you have any questions, don't hesitate to reach out to me or the other CUDA maintainers on Matrix: https://matrix.to/#/#cuda:nixos.org.

[benxiao]

hey @ConnorBaker , thanks a lot for the advices and examples. thanks @bcdarwin for raising this. I am thinking about using torch.cudaCapabilities as well. but I think there is a valid usecase for people who want to overwrite with torch-bin (doesn't require compilation), but doesn't have passthru for cudaCapabilities. would you suggest that I copy the same pattern from torch/default.nix to get cudaCababilities?

as for the odd symbol error, I am already getting that with opencv4, as it is compiled with stdenv.cc gcc 12, and cudatoolkit uses gcc 11. I will give it another go.

[benxiao]

also. with following nix-shell, the torchvision now fails to build with cuda on latest master @ConnorBaker. i assume gcc 12.2 is from stdenv

with import ../nixpkgs {
  config.allowUnfree = true;
 };
let
  mypthon =
    let
      packageOverrides = self: super: {
        # packagesOverride, will affect all packages that uses the resulted packages as deps
        torch = super.torch.override{ magma = magma-cuda; cudaSupport = true; };
        # opencv4 = super.opencv4.override {
        #   enableBlas = false;
        # };
      };
    in
    python3.override { inherit packageOverrides; self = mypthon; };
in
let
  pythonEnv = mypthon.withPackages (ps: with ps; [ torchvision ]);
in
mkShell {
  packages = [ pythonEnv  ];
}
    


 raise RuntimeError(
RuntimeError: The current installed version of g++ (12.2.0) is greater than the maximum required version by CUDA 11.7 (11.5.0). Please make sure to use an adequate version of g++ (>=6.0.0, <=11.5.0).
/nix/store/c3f4jdwzn8fm9lp72m91ffw524bakp6v-stdenv-linux/setup: line 1593: pop_var_context: head of shell_variables not a function context
        

[ConnorBaker]

but I think there is a valid usecase for people who want to overwrite with torch-bin (doesn't require compilation), but doesn't have passthru for cudaCapabilities.

IIRC, newer torch binaries use CUDA redistributables from PyPi. If they haven't moved over to that yet, then their builder is still using bash scripts to manually copy libraries into their binary :(

There's a hardcoded set of supported capabilities for each version of PyTorch. You can find them here: https://github.com/pytorch/pytorch/blob/49444c3e546bf240bed24a101e747422d1f8a0ee/torch/utils/cpp_extension.py#L1751-L1752.

Because it's a binary distributions and not "built" with Nixpkgs, the CUDA capabilities supported by the binary aren't tied to what we specify in our config.

For that reason I'd recommend you use the same logic torch/default.nix has. (Those versions are actually sourced from that same file!)

as for the odd symbol error, I am already getting that with opencv4, as it is compiled with stdenv.cc gcc 12, and cudatoolkit uses gcc 11. I will give it another go.

I have a PR open which should help with that: #221370.

i assume gcc 12.2 is from stdenv

That's right, it is :(

I know that error well. It's from here: https://github.com/pytorch/pytorch/blob/49444c3e546bf240bed24a101e747422d1f8a0ee/torch/utils/cpp_extension.py#L415. (Related discussion on whether checks like that should be removed when included in Nixpkgs: #221564 (comment).)

That's odd that you're getting it though, since the torchvision derivation should correctly set the CC/CXX environment variables to the right compiler:

nixpkgs/pkgs/development/python-modules/torchvision/default.nix

Lines 65 to 72 in e400f93

	# NOTE: We essentially override the compilers provided by stdenv because we don't have a hook
	# for cudaPackages to swap in compilers supported by NVCC.
	+ lib.optionalString cudaSupport ''
	export CC=${backendStdenv.cc}/bin/cc
	export CXX=${backendStdenv.cc}/bin/c++
	export TORCH_CUDA_ARCH_LIST="${lib.concatStringsSep ";" cudaCapabilities}"
	export FORCE_CUDA=1
	'';

.

With respect to your nix file can you try doing something like the following instead? Substitute whatever capability is appropriate for your device (though using just one does make it a lot faster to rebuild stuff from source!).

with import ../nixpkgs {
  config = {
    allowUnfree = true;
    cudaSupport = true;
    cudaCapabilities = [ "8.6" ];
  };
};

I think it was choosing the wrong compiler because cudaSupport wasn't explicitly set to true.

Let me know how that works!

[benxiao]

@ConnorBaker with your config, I fixed the issue. thanks a bunch.

[benxiao]

@ConnorBaker, your MR on opencv fixed my symbol error.

[ConnorBaker]

Yay! Now it just needs to get merged (somewhat difficult given the large number of rebuilds)!

[SuperSandro2000]

This comment does not justify not using pytestCheckHook at all

Suggested change

	# reason for not using pytestCheckHook is similiar to what
	# is already mentioned in pkgs/development/python-modules/typed-ast
	# test_cnn test_ops really requires gpus to be useful.
	# some of the tests take exceedingly long time.
	# the rest of the tests are disabled due to sandbox env.

[benxiao]

I tried to do

  preCheck = ''
    PYTHONPATH=$out/${python.sitePackages}:$PYTHONPATH
  ''

which seems to work with a lot of packages with c extension, but it didn't work for me.

[SuperSandro2000]

Another common way is to rm the the conflicting source directory but pytestCheckHook does just invoke pytest as you do, it only builds the arguments to it.

[benxiao]

had to find that out the hard way. lol. will add disabled tests like you suggested.

[benxiao]

done

[SuperSandro2000]

Please convert this into disabledTestFiles

[benxiao]

done

[SuperSandro2000]

If someone might do that, they can always overwrite the torch input to this package

[SuperSandro2000]

That's 3 months ago. If we introduce a new package and it is a trivial update, we should do that right away unless there is a reason not to

[bcdarwin]

These should almost certainly be moved to nativeCheckInputs.

[benxiao]

Done

[nixos-discourse]

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

https://discourse.nixos.org/t/tweag-nix-dev-update-46/26872/1

[bcdarwin]

This comment is now out of date.

[benxiao]

it was merged in Nixos:Staging not master. so its still relevant

[SuperSandro2000]

It's currently in staging-next https://nixpk.gs/pr-tracker.html?pr=221370

I would suggest to just leave this open until that is merged which will be very soon.

[benxiao]

#221370 has been merged into master. @SuperSandro2000

[SuperSandro2000]

So you can resolve the comment now?

[benxiao]

removed.

[bcdarwin]

Results of nixpkgs-review on x86_64-NixOS:

2 packages built:
python310Packages.mmcv python310Packages.mmcv.dist

[bcdarwin]

LGTM

[SomeoneSerge]

Results of nixpkgs-review on x86_64-NixOS:

Btw, nixpkgs-review has a --post-result option that uses gh cli to post a comment containing the exact command you used to run it. Makes it easier to reproduce the results

[SuperSandro2000]

@ofborg build python310Packages.mmcv