Skip to content
Snippets Groups Projects

[feature][Python] Add default reference-element containers to the library.

Open Robert K requested to merge feature/referenceelements-in-pythonlib into master

This is done for dim <= 3 to reduce compile times.

Edited by Carsten Gräser

Merge request reports

Merge request pipeline #68565 failed

Merge request pipeline failed for 0d89de64

Approval is optional
Merge blocked: 1 check failed
Merge conflicts must be resolved.

Merge details

  • The source branch is 52 commits behind the target branch.
  • 1 commit and 1 merge commit will be added to master.
  • Source branch will be deleted.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • @andreas.dedner: Could you take a look? It's not pretty but it works without changing much of the workflow.

  • Robert K added 1 commit

    added 1 commit

    • 8612d4aa - [feature][Python] Add default reference containers to the library for

    Compare with previous version

  • @andreas.dedner: Any objections of this being merged?

  • Haven't tested yet give me until the weekend

  • @andreas.dedner: Trust me, it's working :-D

  • Robert K added 3 commits

    added 3 commits

    Compare with previous version

  • Robert K unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Robert K added 21 commits

    added 21 commits

    Compare with previous version

  • @samuel.burbulla, @andreas.dedner: This MR pre-compiles the reference modules for the common reference elements, i.e. dim <= 3. I think it is a good idea. It is certainly a speedup and the usage does not change. I works with the pip installed version. So everything seems fine. Any thoughts or objections?

  • Robert K mentioned in merge request dune-common!942 (closed)

    mentioned in merge request dune-common!942 (closed)

  • I don't really think this is necessary - I would rather get rid of those libraries all together because they cause problems with shared/dynamic library choices and the rpath. Also, the advantage is really not clear to me, especially in this case. How much time are you saving here? The reference elements compile really fast on my machine especially compared to the real elephant in the room - the compile time of the grid itself.

  • I didn't know about the shared/dynamic library choices and the rpath problem. I also don't like in this approach here that it represents a mixed approach. There are several benefits though, the module loading becomes instant (no hash necessary) and for the grids the same approach is possible. A branch in alugrid that demonstrates this feature exists already. In general for things that only exists once (like RefElem(dim) we should not employ the complicated create code, compile, load machinery. This makes the code slow. Also, when things are compile as default modules one can use the -jcores option to speed things up. This is not really possible with the current approach (would be nice though).

    One could think about putting the current _dune-module.cc as pre-compiled modules in dune-py that are compiled when dune-py is first setup.

    Edited by Robert K
  • I've thought about ways to move those libraries into dune-py but I wouldn't want the setup of dune-py be a major computational task either. If you allow any module to add 'predefined' targets to dune-py (which I'm sure is possible) then one would be compiling e.g. alugrid each time dune-py is setup just because one has a dune-alugrid module, i.e., independent of actually wanting to use it.

    I would also have to convince myself that the runtime advantage is really that great, e.g., with respect to forming the hash - I would have to check. Being able to parallel build in dune-py would be nice but I don't see any way of doing that which would really bring much advantage. I tested a few things some time ago but couldn't get it to work, i.e., the additional complexity of the approach was never worth the observed gain.

    Btw: the issue with the static libraries is that these libraries are compiled in the build directory and then moved into the site-package. They will for example depend on libdunecommon.so for example which is again moved somewhere else when installed. There might be a solution for this but we haven't found it yet which is the reason why everything is done with static libraries at the moment, e.g., in the pip installation.

  • Robert K added 1 commit

    added 1 commit

    • f52ec03c - [feature][Python] Add default reference containers to the library for

    Compare with previous version

  • Robert K added 38 commits

    added 38 commits

    Compare with previous version

  • Samuel Burbulla mentioned in merge request !185 (closed)

    mentioned in merge request !185 (closed)

  • I like this idea a lot and I think this really improves compilation time and usability of the python bindings.

    The similar thing that I proposed in !185 (closed) can be dropped in favor of @robert.kloefkorn's implementation. I would like to merge this after merging the cmake-overhaul branches.

    Having separate modules for each reference element can be exploited by parallel compilation (which is the default for installed packages because of the Ninja backend). So I would not prefer putting all of them into a single lib.

  • Carsten Gräser changed title from [feature][Python] Add default reference containers to the library. to [feature][Python] Add default reference-element containers to the library.

    changed title from [feature][Python] Add default reference containers to the library. to [feature][Python] Add default reference-element containers to the library.

  • Robert K added 5 commits

    added 5 commits

    Compare with previous version

  • added python label

  • Robert K added 10 commits

    added 10 commits

    Compare with previous version

  • In this case the precompile approach is okay with me, although I'm still not too convinced of the advantages.

    I did some quick tests (each time first running make clean):

    1. make:
      • master 13.41user 1.42system 0:14.20elapsed 104%CPU
      • new 73.95user 5.71system 1:17.71elapsed 102%CPU
    2. make -j8:
      • master 13.70user 1.28system 0:13.33elapsed 112%CPU
      • new 74.63user 4.83system 0:22.48elapsed 353%CPU So if one uses parallel make there is a bit of increase in the make time for dune-geometry - without using -j the increase is considerable. In this case I think it's not an issue but we have to be careful with this approach to not increase module configure time too much.
  • Sure, there is some increase, but only once. I frequently run into the problem that somehow the cmake files get updated and then everything is recompiled. This approach it at least taking the ReferenceElements out of this equation. To me this is a step in the right direction.

  • Robert K added 88 commits

    added 88 commits

    Compare with previous version

  • What is the status of this MR? Any progress? It is not updated and not merged since 2y. Currently, there are merge conflicts. And the spdx headers are missing. Will not be part of 2.10.

Please register or sign in to reply
Loading