Skip to content

parmetis: use github/KarypisLab/ParMETIS as source#329102

Merged
emilazy merged 2 commits intoNixOS:masterfrom
helsinki-systems:fix/parmetis-archive-org
Aug 21, 2024
Merged

parmetis: use github/KarypisLab/ParMETIS as source#329102
emilazy merged 2 commits intoNixOS:masterfrom
helsinki-systems:fix/parmetis-archive-org

Conversation

@cheriimoya
Copy link
Copy Markdown
Contributor

@cheriimoya cheriimoya commented Jul 22, 2024

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 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.

Add a 👍 reaction to pull requests you find important.

@cheriimoya cheriimoya force-pushed the fix/parmetis-archive-org branch from 115ddf6 to 5b6822d Compare July 22, 2024 09:01
@emilazy
Copy link
Copy Markdown
Member

emilazy commented Jul 22, 2024

Perhaps we should source this from https://github.com/KarypisLab/ParMETIS instead. There is a commit corresponding to 4.0.3.

@SuperSandro2000 SuperSandro2000 changed the title Fix/parmetis archive org parmetis: use archive.org as source and others Jul 22, 2024
Comment on lines 21 to 24
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
enableParallelBuilding = true;

the cmake setup hook already sets this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to include ninja in nativeBuildInputs for faster builds, though, if it works.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

unsure, when building without this commit:

$ time nix-build -A parmetis
nix-build -A parmetis  1.04s user 0.38s system 3% cpu 46.465 total

when building with this commit:

$ time nix-build -A parmetis
nix-build -A parmetis  1.09s user 0.36s system 13% cpu 11.118 total

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it might just be Ninja that defaults to parallel builds? I would suggest adding ninja next to cmake in nativeBuildInputs, which should be faster than even a parallel Make build.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

error: builder for '/nix/store/br8kfshh9y3jgkx7gw5f2rkx3392nzwr-parmetis-4.0.3.drv' failed with exit code 1;
       last 10 log lines:
       > -- Looking for execinfo.h - found
       > -- Looking for getline
       > -- Looking for getline - found
       > -- checking for thread-local storage - found
       > -- Configuring done (3.2s)
       > -- Generating done (0.0s)
       > -- Build files have been written to: /build/parmetis-4.0.3/build/Linux-x86_64
       > Running phase: buildPhase
       > build flags: -j12
       > ninja: error: loading 'build.ninja': No such file or directory
       For full logs, run 'nix log /nix/store/br8kfshh9y3jgkx7gw5f2rkx3392nzwr-parmetis-4.0.3.drv'.
nix-build -A parmetis  1.05s user 0.42s system 27% cpu 5.408 total

@emilazy
Copy link
Copy Markdown
Member

emilazy commented Jul 22, 2024

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jul 22, 2024
@ofborg ofborg bot requested a review from costrouc July 22, 2024 10:16
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jul 22, 2024
@cheriimoya
Copy link
Copy Markdown
Contributor Author

Okay, so I went through many hoops now and the commits I'm pushing now is what I came up with.
I tested it with a pflotran simulation and the output of the simulation didn't change after switching to this version of parmetis, which is needed for petsc, which is needed for pflotran.
Execution time changed in a negligible way.
I don't know much about C, so somebody better double check what I did 😅


Result of nixpkgs-review run on x86_64-linux 1

8 packages built:
  • getdp
  • parmetis
  • petsc
  • precice
  • python311Packages.pyprecice
  • python311Packages.pyprecice.dist
  • python312Packages.pyprecice
  • python312Packages.pyprecice.dist

@cheriimoya cheriimoya force-pushed the fix/parmetis-archive-org branch from 7bd8ef5 to fbff98c Compare July 24, 2024 08:43
@cheriimoya cheriimoya requested a review from emilazy July 24, 2024 08:44
@cheriimoya cheriimoya changed the title parmetis: use archive.org as source and others parmetis: use petsc parmetis as source Jul 24, 2024
@emilazy
Copy link
Copy Markdown
Member

emilazy commented Jul 26, 2024

Sorry for the back and forth on this. I didn’t realize that this package is licensed under very restrictive terms. It’s not clear to me that the licence permits derivative works at all. It seems like PETSc is developed by a US government research agency, so I guess it hinges on whether “freely used” permits distributing modified versions.

Does your pflotran simulation work with the upstream GitHub commit that supposedly corresponds to 4.0.3? If so, it may be a more conservative choice to use that version instead.

@cheriimoya
Copy link
Copy Markdown
Contributor Author

cheriimoya commented Jul 29, 2024 via email

@cheriimoya cheriimoya force-pushed the fix/parmetis-archive-org branch 2 times, most recently from 0e05e78 to 8ea28bb Compare August 6, 2024 09:55
@cheriimoya
Copy link
Copy Markdown
Contributor Author

Tested the latest commit and the simulation runs through, producing the correct results as expected. This is now using the metis package source as input, so maybe we could think about also switching the metis source.

@cheriimoya cheriimoya changed the title parmetis: use petsc parmetis as source parmetis: use github/KarypisLab/ParMETIS as source Aug 6, 2024
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
description = "MPI-based parallel library that implements a variety of algorithms for partitioning unstructured graphs, meshes, and for computing fill-reducing orderings of sparse matrices";
description = "MPI-based parallel library that implements a variety of algorithms";

thats a bit long

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I’d prefer to cut “that implements a variety of algorithms” so that some description of what the package actually does is retained…

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We also have longDescription.

description = "Parallel Graph Partitioning and Fill-reducing Matrix Ordering";
longDescription = ''
  MPI-based parallel library that implements a variety of algorithms for
  partitioning unstructured graphs, meshes, and for computing fill-reducing
  orderings of sparse matrices.
  The algorithms implemented in ParMETIS are based on the multilevel
  recursive-bisection, multilevel k-way, and multi-constraint partitioning
  schemes
'';

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I added it:)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
enableParallelBuilding = true;

already done by cmake

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are you sure?
If i remove the line it seems like the build takes more time:

# command used: `time nix-build . -A parmetis`
# with `enableParallelBuilding = true;`
nix-build . -A parmetis  1.68s user 0.34s system 8% cpu 22.698 total
# vs without `enableParallelBuilding = true;`
nix-build . -A parmetis  1.67s user 0.35s system 3% cpu 57.441 total

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe this is a result of running make in the configurePhase?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You probably want [ cmake ninja ].

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think you mentioned this before:
image

#329102 (comment)

Or am i using this incorrectly? I just added it to the nativeBuildInputs

Copy link
Copy Markdown
Member

@emilazy emilazy Aug 6, 2024

Choose a reason for hiding this comment

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

Uh, sorry, I completely forgot, I was just reacting to the parallel build stuff… it should just work with CMake builds, but I guess there’s something funky going on here. I think you’d have to drop the make stuff in configurePhase and just let the CMake hook run naturally, with any required cmakeFlags corresponding to the ones set in the upstream Makefile.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tested several different approaches: Running the tar xf during preConfigure and not touching the configurePhase, commenting out make in the configurePhase, but nothing worked. How bad would it be to just set enableParallelBuilding = true;? I can also remove the line and we can settle for only building with one thread if the option is that bad...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When setting the cmakeFlags to

cmakeFlags = [
  "-DGKLIB_PATH=metis/GKlib"
  "-DMETIS_PATH=metis" 
];

The build is running in parallel as expected, but then the build fails with

parmetis> [ 55%] Building C object libparmetis/CMakeFiles/parmetis.dir/balancemylink.c.o
parmetis> [ 55%] Building C object libparmetis/CMakeFiles/parmetis.dir/akwayfm.c.o
parmetis> [ 56%] Building C object libparmetis/CMakeFiles/parmetis.dir/comm.c.o
parmetis> In file included from /build/source/libparmetis/balancemylink.c:14:
parmetis> /build/source/libparmetis/./parmetislib.h:23:10: fatal error: ../metis/libmetis/gklib_defs.h: No such file or directory
parmetis>    23 | #include "../metis/libmetis/gklib_defs.h"
parmetis>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
parmetis> compilation terminated.
parmetis> In file included from /build/source/libparmetis/akwayfm.c:14:
parmetis> /build/source/libparmetis/./parmetislib.h:23:10: fatal error: ../metis/libmetis/gklib_defs.h: No such file or directory
parmetis>    23 | #include "../metis/libmetis/gklib_defs.h"
parmetis>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
parmetis> make[2]: *** [libparmetis/CMakeFiles/parmetis.dir/build.make:104: libparmetis/CMakeFiles/parmetis.dir/balancemylink.c.o] Error 1
parmetis> make[2]: *** Waiting for unfinished jobs....
parmetis> compilation terminated.
parmetis> make[2]: *** [libparmetis/CMakeFiles/parmetis.dir/build.make:76: libparmetis/CMakeFiles/parmetis.dir/akwayfm.c.o] Error 1
parmetis> In file included from /build/source/libparmetis/ametis.c:15:
parmetis> /build/source/libparmetis/./parmetislib.h:23:10: fatal error: ../metis/libmetis/gklib_defs.h: No such file or directory
parmetis>    23 | #include "../metis/libmetis/gklib_defs.h"
parmetis>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
parmetis> compilation terminated.
parmetis> make[2]: *** [libparmetis/CMakeFiles/parmetis.dir/build.make:90: libparmetis/CMakeFiles/parmetis.dir/ametis.c.o] Error 1
parmetis> In file included from /build/source/libparmetis/comm.c:11:
parmetis> /build/source/libparmetis/./parmetislib.h:23:10: fatal error: ../metis/libmetis/gklib_defs.h: No such file or directory
parmetis>    23 | #include "../metis/libmetis/gklib_defs.h"
parmetis>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
parmetis> compilation terminated.
parmetis> make[2]: *** [libparmetis/CMakeFiles/parmetis.dir/build.make:118: libparmetis/CMakeFiles/parmetis.dir/comm.c.o] Error 1
parmetis> make[1]: *** [CMakeFiles/Makefile2:184: libparmetis/CMakeFiles/parmetis.dir/all] Error 2
parmetis> make: *** [Makefile:136: all] Error 2

I'd prefer to simply use the commits I provided.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It’s totally fine to set enableParallelBuilding = true;, in my opinion. But it is indeed the overridden configurePhase that is the issue here, because CMake’s includes this:

    if ! [[ -v enableParallelBuilding ]]; then
        enableParallelBuilding=1
        echo "cmake: enabled parallel building"
    fi

I think the issue you’re running into there might be because you’re using a relative path whereas the Makefile explicitly makes it absolute, though.

@cheriimoya cheriimoya force-pushed the fix/parmetis-archive-org branch from 8ea28bb to 8ce6a3c Compare August 6, 2024 12:32
@cheriimoya cheriimoya force-pushed the fix/parmetis-archive-org branch from 8ce6a3c to a468bfc Compare August 19, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants