netcdf4:init at 1.5.1.2 #67338

mamueller · 2019-08-23T12:23:23Z

#67337
Generalized the expression to be able to create a version with parallel
IO. If the function is called with netcdf-mpi, hdf5-mpi and mpi4py
it now produces a version that can access netcdf4 files in parallel.
I tested this with a small python script that uses parallel IO.
This should probably go to the original testsuite of the package which
is disabled in the nix expression I started with.
So I did not touch this part.
The patch is necessary since the original setup.py does not find
the parallel version of the netcdf although it is there since it
inspects the header file with a pattern that is propably only present in
the newest version. I made a pullrequest to the upstream maintainers to
generelize the pattern so that the patch will no longer be needed.

Motivation for this change

Things done

Notify maintainers

cc @

lsix · 2019-08-24T21:31:32Z

Looks good to me (overall), I'll give it a try when possible (it might take time).

Did you try to see if the tests can be re-enabled?

lsix · 2019-08-24T21:37:27Z

The commits should be renamed:

netcdf4:init at 1.5.1.2 -> pythonPackages.netcdf4: enable parallel IO support or something like that.

I would also say that your second commit (introducing netcdffortran-mpi) could deserve its own PR, since it is independent (about another package, not dependent on the python netcdf binding).

lsix · 2019-08-24T21:38:32Z

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

+      inherit mpi;
+    }; 
+    meta = with stdenv.lib; {
+      description = "Interface to netCDF library (versions 3 and 4) with support for parallel IO";


I do not think the description should change from the previous implementation.

lsix · 2019-08-24T21:40:39Z

And by the way, thanks for your first PR to nixpkgs and welcome in the community !

FRidh

Please undo the indentation change that is not needed.

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

veprbl · 2019-08-27T17:20:49Z

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

-  version = "1.5.1.2";
+, numpy, zlib, netcdf, hdf5, curl, libjpeg, cython, cftime 
+,mpi4py ? null, openssh ? null}:
+assert (mpi4py!= null) -> 


Suggested change

assert (mpi4py!= null) ->

assert (mpi4py != null) ->

veprbl · 2019-08-27T17:21:04Z

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


-  disabled = isPyPy;
+let 
+  mpiSupport = (mpi4py!=null);


Suggested change

mpiSupport = (mpi4py!=null);

mpiSupport = (mpi4py != null);

veprbl · 2019-12-27T21:27:22Z

@mamueller Are you interested in finishing this?

mamueller · 2019-12-31T14:52:20Z

What would have to be done? Am Fr., 27. Dez. 2019 um 22:27 Uhr schrieb Dmitry Kalinkin < notifications@github.com>:

…

@mamueller <https://github.com/mamueller> Are you interested in finishing this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#67338?email_source=notifications&email_token=AAIRGPPAGBP2X6IYY27ADRTQ2ZXLXA5CNFSM4IO7CKDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHXZFCQ#issuecomment-569348746>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIRGPN4UL5KECRHVS55R7DQ2ZXLXANCNFSM4IO7CKDA> .

veprbl · 2019-12-31T17:55:24Z

@mamueller You need to rebase your changes on top of our master branch to fix the merge conflict. Since we already updated to version 1.5.3, you should not need your parallel4Detection.patch anymore [1]. Also, you would need to address the review comments above.

[1] Unidata/netcdf4-python@43ba3e5

mamueller · 2020-01-02T18:34:36Z

I am a little under pressure just now. So I would not mind if I would not have to do it right now. (At the moment I am not working on any nix related stuff, so its a bit awkward to sell to my boss, while when I made the change we really needed it ...) Am Di., 31. Dez. 2019 um 18:55 Uhr schrieb Dmitry Kalinkin < notifications@github.com>:

…

@mamueller <https://github.com/mamueller> You need to rebase your changes on top of our master branch to fix the merge conflict. Since we already updated to version 1.5.3, you should not need your parallel4Detection.patch anymore [1]. Also, you would need to address the review comments above. ***@***.*** <Unidata/netcdf4-python@43ba3e5> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#67338?email_source=notifications&email_token=AAIRGPN7JJIN7BJZWIMHJDTQ3OBQ3A5CNFSM4IO7CKDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH4P6JA#issuecomment-569966372>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIRGPMD5LUQ4MNHPDWM3GLQ3OBQ3ANCNFSM4IO7CKDA> .

mamueller · 2020-05-28T10:28:24Z

Sorry for the late response.
I just had a little time to work on it and updated to the new netcdf4 version.
Even (most) tests run now.
With some strange things that I cannot figure out as described in my commit message.
I am not sure how to address the request by FRidh, because I am still (very) new to nix and not too familiar with the stdenv. I think it will be a one liner.
Hopefully the pullrequest goes through now.
Cheers and thanks for the friendly welcome.
Markus

Now also the tests run except one that is skipped by the patch. The behavior of the tests is puzzeling to me in two ways: 1. If I drop into nix-shell (even with --pure ) and call genericBuild all tests run but if I use nix-build the test fails. (If you look at the test you see that it tries to load a remote url. I have no clue why this works in the shell and not in nix-build) 2. A similar thing happens with py.test: It works in the nix-shell but fails with nix-build but here the test suite does not even start. I used unittest instead which leads to only one test failing (which is now skipped by the patch)

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

veprbl

@mamueller Yes there are a few things you missed from previous review. This is looking better (style-wise), but there are still few rough edges to fix. Please see my comments below.

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

veprbl · 2020-05-29T16:31:31Z

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


-  checkInputs = [ pytest ];
+    #patches=[./skipDapTest.patch];


Please remove this.

pkgs/top-level/all-packages.nix

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

veprbl · 2020-06-11T17:59:04Z

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

+  HDF5_DIR= lib.getDev hdf5;
+  NETCDF4_DIR="${netcdf}";
+  CURL_DIR="${curl.dev}";
+  JPEG_DIR="${libjpeg.dev}";


Changes to these four lines are not needed.

Thanks for your time. I learned a bit about nix (I can now use inherit properly ;-) and also see the point of lib.optionals..)
I have committed all the suggestions and added the remaining outdated ones manually.
I have some remaining questions:

I am sorry for having created such a lot of style problems.
Is there a linter (or at least a style guide)?
I am using vim with vim-nix
For python code I use the python language server integration which is very verbose
Is there something similar available for nix?

I am a bit unsure about the workflow used to deal with pull requests.
( I normally work in a very small group at a research institute where 99.999 % of git usage consists of pull and push and even branches are an exception..., so we are not very professional users..)
I know what rebasing means and did it twice already for this branch, but I do not know for instance if you would prefer my (many tiny) commits to be squashed into one or if I would make things harder by it...)
So any suggestions about using git professionally are also welcome.

Thanks again.

@mamueller
Regarding your questions:

There are couple linters that could help. nixpkgs-fmt is one of the most promising ones. There are several other ones: https://github.com/nix-community/nixpkgs-fmt#formatters
Note that if will format whole file at the time, which may hinder the review process if you mix functional changes with style changes. A typical solution to that is to format the original file in a first commit and then introduce functional changes in subsequent separate commits. It is a bit too late to do that in this PR, but you could try this in the future.

Usually commits are supposed to have one specific idea for a change that is well described in the commit message. So if a PR contains several separate changes, we usually ask for them to be in separate commits. In case of this PR there is only one change (adding mpi support to pythonPackages.netcdf4), so we might just squash everything on merge for you. If you are unsure what to do, look around at other peoples contributions, see what they do.

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

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

FRidh · 2020-06-12T13:19:48Z

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

-    py.test test/tst_*.py
+    cd test
+    rm tst_dap.py # does not fail in the nix-shell  
+    python -m unittest tst_*.py


${python.interpreter} instead of python. This is a prerequisite for supporting non-CPython interpreters such as PyPy.

veprbl

Noticed there were couple whitespace issues. Also, reposting some previous comments that were not addressed.

veprbl · 2020-06-12T16:18:15Z

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

 buildPythonPackage rec {
  pname = "netCDF4";
  version = "1.5.3";

  disabled = isPyPy;
-
+  


This change is not needed

veprbl · 2020-06-12T16:18:26Z

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


  buildInputs = [
-    cython
+    mpi 


Suggested change

mpi

mpi

veprbl · 2020-06-12T16:19:05Z

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

-  ];
+  ] ++ lib.optionals mpiSupport [mpi4py mpi openssh];
+
+  #patches=[./skipDapTest.patch];


Please remove this

veprbl · 2020-06-12T16:20:05Z

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

-  NETCDF4_DIR=netcdf;
-  CURL_DIR=curl.dev;
-  JPEG_DIR=libjpeg.dev;
+  NETCDF4_DIR="${netcdf}";


Suggested change

NETCDF4_DIR="${netcdf}";

NETCDF4_DIR=netcdf;

veprbl · 2020-06-12T16:20:19Z

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

-  CURL_DIR=curl.dev;
-  JPEG_DIR=libjpeg.dev;
+  NETCDF4_DIR="${netcdf}";
+  CURL_DIR="${curl.dev}";


Suggested change

CURL_DIR="${curl.dev}";

CURL_DIR=curl.dev;

veprbl · 2020-06-12T16:20:30Z

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

-  JPEG_DIR=libjpeg.dev;
+  NETCDF4_DIR="${netcdf}";
+  CURL_DIR="${curl.dev}";
+  JPEG_DIR="${libjpeg.dev}";


Suggested change

JPEG_DIR="${libjpeg.dev}";

JPEG_DIR=libjpeg.dev;

FRidh · 2020-07-04T11:10:43Z

closes #67340.

jonringer

assuming other reviews are addressed

https://github.com/NixOS/nixpkgs/pull/67338
1 package built:
netcdffortran-mpi

cc @bhipple for mpi conventions

bhipple · 2020-07-04T18:22:40Z

pkgs/top-level/all-packages.nix

+  netcdffortran-mpi = appendToName "mpi" (netcdffortran.override {
+    hdf5 = hdf5-mpi;
+    netcdf = netcdf-mpi;
+  });


We haven't done anything with mpi, but the direction we went with blas/lapack was to have 1 variable at the top level specifying your variant, and then an overlay can -- in one place -- consistently override everything to be the right flavor.

This ensures you don't end up with two incoherent flavors of something like numpy in your stack. CC @matthewbauer not sure if we should do something similar for mpi.

ah, i did get them mixed up. But I think we should have similar conventions between blas and mpi.

stale · 2021-06-03T02:36:05Z

I marked this as stale due to inactivity. → More info

stale · 2022-04-16T03:16:07Z

I marked this as stale due to inactivity. → More info

mamueller requested a review from FRidh as a code owner August 23, 2019 12:23

ofborg bot added 6.topic: python 8.has: clean-up 10.rebuild-darwin: 0 10.rebuild-linux: 0 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Aug 23, 2019

lsix reviewed Aug 24, 2019

View reviewed changes

FRidh requested changes Aug 27, 2019

View reviewed changes

pkgs/development/python-modules/netcdf4/default.nix Outdated Show resolved Hide resolved

pkgs/development/python-modules/netcdf4/default.nix Outdated Show resolved Hide resolved

pkgs/development/python-modules/netcdf4/default.nix Outdated Show resolved Hide resolved

veprbl reviewed Aug 27, 2019

View reviewed changes

veprbl added the 2.status: merge conflict label Sep 11, 2019

mamueller requested a review from jonringer as a code owner May 28, 2020 09:43

mamueller force-pushed the master branch from 0aa77a0 to a7288b4 Compare May 28, 2020 10:18

mamueller requested a review from FRidh May 28, 2020 10:19

ofborg bot added 10.rebuild-darwin: 1 10.rebuild-linux: 1 and removed 2.status: merge conflict labels May 28, 2020

mamueller and others added 4 commits June 8, 2020 18:25

Update pkgs/development/python-modules/netcdf4/default.nix

af62f87

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

hopefully addressed all the comments

60b0d15

removed the added indentation

0ece1a3

mamueller force-pushed the master branch from 18d7975 to 0ece1a3 Compare June 8, 2020 16:26

added the desired quotation marks

99aab5f

veprbl requested changes Jun 11, 2020

View reviewed changes

mamueller and others added 7 commits June 12, 2020 11:37

Update pkgs/top-level/all-packages.nix

76123af

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

Update pkgs/development/python-modules/netcdf4/default.nix

da6c475

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

Update pkgs/development/python-modules/netcdf4/default.nix

6d5943d

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

Update pkgs/development/python-modules/netcdf4/default.nix

84c3b30

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

Update pkgs/development/python-modules/netcdf4/default.nix

9649e5c

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

Update pkgs/development/python-modules/netcdf4/default.nix

2c49463

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

Incorporated all the remaining changes

7aca1fc

FRidh reviewed Jun 12, 2020

View reviewed changes

veprbl reviewed Jun 12, 2020

View reviewed changes

This was linked to issues Jul 4, 2020

netcdffortran-mpi #67340

Open

no parallel version of netcdf4_py #67337

Open

jonringer approved these changes Jul 4, 2020

View reviewed changes

bhipple reviewed Jul 4, 2020

View reviewed changes

SuperSandro2000 marked this pull request as draft November 29, 2020 01:40

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 3, 2021

veprbl added the 9.needs: reporter feedback label Oct 3, 2021

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2021

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 16, 2022

AndersonTorres closed this Jun 16, 2022

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netcdf4:init at 1.5.1.2 #67338

netcdf4:init at 1.5.1.2 #67338

mamueller commented Aug 23, 2019 •

edited

lsix commented Aug 24, 2019 •

edited

lsix commented Aug 24, 2019

lsix Aug 24, 2019

lsix commented Aug 24, 2019

FRidh left a comment

veprbl Aug 27, 2019

veprbl Aug 27, 2019

veprbl commented Dec 27, 2019

mamueller commented Dec 31, 2019 via email

veprbl commented Dec 31, 2019 •

edited

mamueller commented Jan 2, 2020 via email

mamueller commented May 28, 2020

veprbl left a comment

veprbl May 29, 2020

veprbl Jun 11, 2020

mamueller Jun 12, 2020

veprbl Jun 12, 2020

FRidh Jun 12, 2020

veprbl left a comment

veprbl Jun 12, 2020

veprbl Jun 12, 2020

veprbl Jun 12, 2020

veprbl Jun 12, 2020

veprbl Jun 12, 2020

veprbl Jun 12, 2020

FRidh commented Jul 4, 2020

jonringer left a comment

bhipple Jul 4, 2020

jonringer Jul 4, 2020

stale bot commented Jun 3, 2021

stale bot commented Apr 16, 2022

netcdf4:init at 1.5.1.2 #67338

netcdf4:init at 1.5.1.2 #67338

Conversation

mamueller commented Aug 23, 2019 • edited

Motivation for this change

Things done

Notify maintainers

lsix commented Aug 24, 2019 • edited

lsix commented Aug 24, 2019

Choose a reason for hiding this comment

lsix commented Aug 24, 2019

FRidh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veprbl commented Dec 27, 2019

mamueller commented Dec 31, 2019 via email

veprbl commented Dec 31, 2019 • edited

mamueller commented Jan 2, 2020 via email

mamueller commented May 28, 2020

veprbl left a 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

veprbl left a 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

FRidh commented Jul 4, 2020

jonringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 3, 2021

stale bot commented Apr 16, 2022

mamueller commented Aug 23, 2019 •

edited

lsix commented Aug 24, 2019 •

edited

veprbl commented Dec 31, 2019 •

edited