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.

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

petsc: 3.8.4 -> 3.10.3 #55358

Closed
wants to merge 11 commits into from
Closed

petsc: 3.8.4 -> 3.10.3 #55358

wants to merge 11 commits into from

Conversation

@jamtrott
Copy link
Contributor

@jamtrott jamtrott commented Feb 6, 2019

These changes upgrade PETSc to the most recent release and add several useful linear solver backends, including HYPRE, MUMPS, scalapack, and SPAI. In addition, the Python wrapper package petsc4py is built.

This has been tested on Mac OS X 10.13.6 (High Sierra).

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@srhb srhb left a comment

Evaluation currently fails due to some misses in the meta sections. Can you fix these up? :)

@jamtrott
Copy link
Contributor Author

@jamtrott jamtrott commented Feb 8, 2019

Evaluation currently fails due to some misses in the meta sections. Can you fix these up? :)

Hi Sarah. Thank you for reviewing!

I fixed those mistakes and have pushed some new commits. I'm not sure whether it was necessary to create a commit for each separate package, but they can always be squashed before merging anyway.

@srhb
Copy link
Contributor

@srhb srhb commented Feb 8, 2019

Ideally I'd like this to be squashed down to one commit per package, would you mind doing that? :)

@jamtrott
Copy link
Contributor Author

@jamtrott jamtrott commented Feb 8, 2019

Ideally I'd like this to be squashed down to one commit per package, would you mind doing that? :)

No problem at all. What would be the correct way of doing so? Do I close this pull request and submit a new one? Or is it possible to overwrite the branch with a forced push?

@srhb
Copy link
Contributor

@srhb srhb commented Feb 8, 2019

@jamtrott The easiest way is to locally undo your changes with a soft reset, eg git reset master -- this will leave your changed files but uncommitted. You can now piecemeal commit each logical package, and finally force push your branch, which will update this PR. :)

Copy link
Member

@dotlambda dotlambda left a comment

Please add the pythonPackages. prefix to the respective commits.

meta = {
description = "Python bindings for PETSc, the Portable, Extensible Toolkit for Scientific Computation";
homepage = https://bitbucket.org/petsc/petsc4py;
license = stdenv.lib.licenses.bsd3;
Copy link
Member

@dotlambda dotlambda Feb 9, 2019

Choose a reason for hiding this comment

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

Suggested change
license = stdenv.lib.licenses.bsd3;
license = lib.licenses.bsd3;

@@ -0,0 +1,43 @@
{ stdenv
Copy link
Member

@dotlambda dotlambda Feb 9, 2019

Choose a reason for hiding this comment

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

Suggested change
{ stdenv
{ lib

openmpi
openssh
mpi4py
];
Copy link
Member

@dotlambda dotlambda Feb 9, 2019

Choose a reason for hiding this comment

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

Some of those should probably be propagated. Why is python there?

Copy link
Contributor Author

@jamtrott jamtrott Feb 10, 2019

Choose a reason for hiding this comment

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

Hi @dotlambda, thank you for your comments. I admit that I'm not sure about buildInputs versus propagatedBuildInputs. Do you have any suggestions or guidelines for determining which packages should be propagated?

The python package is not needed, and I have removed it from the list.

Copy link
Member

@dotlambda dotlambda Feb 10, 2019

Choose a reason for hiding this comment

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

See https://nixos.org/nixpkgs/manual/#reference:

If something is exclusively a build-time dependency, use buildInputs; if it is (also) a runtime dependency, use propagatedBuildInputs.

Copy link
Member

@dotlambda dotlambda Feb 21, 2019

Choose a reason for hiding this comment

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

See above for cython.
I don't find mpi4py in their setup.py.
numpy is in install_requires and should thus be propagated.
What do you infer the dependency on openssh from?

Copy link
Contributor Author

@jamtrott jamtrott Jun 17, 2019

Choose a reason for hiding this comment

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

Sorry for taking so long before getting back to this. Anyway, I've pushed another couple of commits that address the issues you raise.

Both cython and numpy should be fixed now.

The dependency on openssh is needed to avoid tests from failing when using openmpi. This looks to me like a common pattern, and you will find the same in, for example, pkgs/development/python-modules/mpi4py/default.nix.

For reference, the test failures without openssh look something like this:

running install tests
running test
--------------------------------------------------------------------------
The value of the MCA parameter "plm_rsh_agent" was set to a path
that could not be found:

  plm_rsh_agent: ssh : rsh

Please either unset the parameter, or check that the path is correct
--------------------------------------------------------------------------
[srl-login1:13710] [[INVALID],INVALID] FORCE-TERMINATE AT Not found:-13 - error plm_rsh_component.c(327)
[srl-login1:13710] *** Process received signal ***
[srl-login1:13710] Signal: Segmentation fault (11)
[srl-login1:13710] Signal code: Address not mapped (1)
[srl-login1:13710] Failing at address: (nil)
[srl-login1:13710] [ 0] /nix/store/fivq0nbggp4y8mhy3ixprqd7qyn1hy2j-glibc-2.27/lib/libpthread.so.0(+0x11ed0)[0x1555532aeed0]
[srl-login1:13710] *** End of error message ***
[srl-login1:13707] [[INVALID],INVALID] ORTE_ERROR_LOG: Unable to start a daemon on the local node in file ess_singleton_module.c at line 532
[srl-login1:13707] [[INVALID],INVALID] ORTE_ERROR_LOG: Unable to start a daemon on the local node in file ess_singleton_module.c at line 166
--------------------------------------------------------------------------
It looks like orte_init failed for some reason; your parallel process is
likely to abort.  There are many reasons that a parallel process can
fail during orte_init; some of which are due to configuration or
environment problems.  This failure appears to be an internal failure;
here's some additional information (which may only be relevant to an
Open MPI developer):

  orte_ess_init failed
  --> Returned value Unable to start a daemon on the local node (-127) instead of ORTE_SUCCESS
--------------------------------------------------------------------------
--------------------------------------------------------------------------
It looks like MPI_INIT failed for some reason; your parallel process is
likely to abort.  There are many reasons that a parallel process can
fail during MPI_INIT; some of which are due to configuration or environment
problems.  This failure appears to be an internal failure; here's some
additional information (which may only be relevant to an Open MPI
developer):

  ompi_mpi_init: ompi_rte_init failed
  --> Returned "Unable to start a daemon on the local node" (-127) instead of "Success" (0)
--------------------------------------------------------------------------
*** An error occurred in MPI_Init_thread
*** on a NULL communicator
*** MPI_ERRORS_ARE_FATAL (processes in this communicator will now abort,
***    and potentially your MPI job)
[srl-login1:13707] Local abort before MPI_INIT completed completed successfully, but am not able to aggregate error messages, and not able to guarantee that all other processes were killed!
builder for '/nix/store/5h917aj1lxgxj4vpq1h5zn3ncpnarr32-python2.7-petsc4py-3.10.1.drv' failed with exit code 1
error: build of '/nix/store/5h917aj1lxgxj4vpq1h5zn3ncpnarr32-python2.7-petsc4py-3.10.1.drv' failed

jamtrott added 2 commits Feb 10, 2019
This prevents issues when using multiple cython-based packages
together (for example, mpi4py and petsc4py) due to code that has been
generated with incompatible cython versions.

Also, update homepage.
@jamtrott
Copy link
Contributor Author

@jamtrott jamtrott commented Feb 18, 2019

@srhb, @dotlambda, would you agree that the latest revisions address the issues you have raised?


buildInputs = [ mpi openssh ];
buildInputs = [ cython mpi openssh ];
Copy link
Member

@dotlambda dotlambda Feb 21, 2019

Choose a reason for hiding this comment

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

I guess cython belongs into nativeBuildInputs because it's listed in setup_requires: https://bitbucket.org/mpi4py/mpi4py/src/0d17284a209ebcc7af362e7d110d518308d43299/setup.py#setup.py-464.
See also the new explanation not yet available in the published manual: https://github.com/NixOS/nixpkgs/blob/b4acd97/doc/languages-frameworks/python.section.md#buildpythonpackage-parameters

Copy link
Contributor Author

@jamtrott jamtrott Jun 17, 2019

Choose a reason for hiding this comment

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

Thanks, this should be fixed now.


PETSC_DIR = "${petsc}";

setupPyBuildFlags = [ "build_src --force" ];
Copy link
Member

@dotlambda dotlambda Feb 21, 2019

Choose a reason for hiding this comment

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

You should add the same comment here.

Copy link
Contributor Author

@jamtrott jamtrott Jun 17, 2019

Choose a reason for hiding this comment

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

Done.

openmpi
openssh
mpi4py
];
Copy link
Member

@dotlambda dotlambda Feb 21, 2019

Choose a reason for hiding this comment

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

See above for cython.
I don't find mpi4py in their setup.py.
numpy is in install_requires and should thus be propagated.
What do you infer the dependency on openssh from?


propagatedBuildInputs = [
petsc
openmpi
Copy link
Member

@dotlambda dotlambda Feb 21, 2019

Choose a reason for hiding this comment

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

I don't find openmpi in ther setup.py.
petsc can probably be dropped from here since you specify PETSC_DIR.

Copy link
Contributor Author

@jamtrott jamtrott Jun 17, 2019

Choose a reason for hiding this comment

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

I moved openmpi to nativeBuildInputs and removed petsc. The package doesn't build without openmpi.

jamtrott added 2 commits Jun 17, 2019
Remove mpi4py and make other packages nativeBuildInputs,
except numpy, which is propagated.
@tkonolige
Copy link
Contributor

@tkonolige tkonolige commented Jul 11, 2019

Would it be possible to parametrize these packages by which MPI implementation they use instead of hardcoding openmpi?

It would also be nice to have debugging be optional.

Also, the PETSc developers recommend compiling with -O3.

@FRidh
Copy link
Member

@FRidh FRidh commented Feb 9, 2020

Would be good to see the merge conflicts resolved, because this seems otherwise ready.

@stale
Copy link

@stale stale bot commented Aug 7, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

@stale
Copy link

@stale stale bot commented Jun 4, 2021

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

@stale stale bot added the 2.status: stale label Jun 4, 2021
@jamtrott jamtrott closed this Jun 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants