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

Expose more optional feature booleans, remove Timer from top level dolfinx #3335

Merged
merged 19 commits into from
Sep 11, 2024

Conversation

jhale
Copy link
Member

@jhale jhale commented Aug 6, 2024

This PR removes Timer functionality from the top-level dolfinx module and exposes all of the C++ compile definitions through to Python, including a new dolfinx.has_petsc4py boolean.

@jhale jhale changed the title Expose more optional feature booleans in public API. Expose and move optional feature booleans Aug 6, 2024
@schnellerhase
Copy link
Contributor

We previously missed the HAS_PETSC4PY flag in these exports as well, might be a good place to add that one as well? Also, when this is exported some of the demos (the ones using non linear solvers I think) should probably check for both has_petsc and has_petsc4py, instead of only has_petsc.

@jhale
Copy link
Member Author

jhale commented Aug 7, 2024

These flags are related to the optional features of the C++ library only. Python can perform runtime introspection.

if importlib.util.find_spec("petsc4py") is not None:
    import dolfinx

    if not dolfinx.common.has_petsc:
        print("This demo requires DOLFINx to be compiled with PETSc enabled.")
        exit(0)

    from petsc4py import PETSc

    if PETSc.IntType == np.int64 and MPI.COMM_WORLD.size > 1:
        print("This solver fails with PETSc and 64-bit integers becaude of memory errors in MUMPS.")
        # Note: when PETSc.IntType == np.int32, superlu_dist is used rather
        # than MUMPS and does not trigger memory failures.
        exit(0)
else:
    print("This demo requires petsc4py.")
    exit(0)

The logic here is: OK, I found petsc4py, but did you build DOLFINx with PETSc? If not, then DOLFINx cannot build PETSc data structures anyway, and the demo cannot be run.

@schnellerhase
Copy link
Contributor

Yes, but if I'm not overseeing something here, the cpp may be configured with petsc but the python module build without petsc4py being available, see

#if defined(HAS_PETSC) && defined(HAS_PETSC4PY)
. In which case the flag common.has_petsc will be true, as set by the cpp build, but no nls submodule will be available. This is also independent of the python import of petsc4py working I guess, could have been made available after the python module install.

@jhale
Copy link
Member Author

jhale commented Aug 7, 2024

Right, thanks for explaining - I see your point. It's probably clearest to expose both and the tests in the demos should check if DOLFINx Python was built with petsc4py support (which in turn requires DOLFINx C++ to be built with PETSc support).

@schnellerhase schnellerhase mentioned this pull request Aug 7, 2024
1 task
@mscroggs mscroggs added this to the 0.9.0 milestone Sep 3, 2024
@jhale
Copy link
Member Author

jhale commented Sep 4, 2024

@garth-wells This patch does make the top level namespace cleaner but is an API change. My view is that has_* are not commonly used enough to justify their presence cluttering autocompletion on dolfinx.*. Nevertheless it's not something I feel strongly about. What would you prefer?

@jhale
Copy link
Member Author

jhale commented Sep 4, 2024

In support of keeping has_* in dolfinx is that is consistent with the C++ interface where these are in namespace dolfinx::.

@garth-wells
Copy link
Member

I don't particularly like this change. It adds clutter, but I don't see the benefit. Properties like has_foo are 'properties' of dolfinx, so I prefer things as they are.

@jhale
Copy link
Member Author

jhale commented Sep 6, 2024

I will modify this patch so that we have:

@jhale jhale changed the title Expose and move optional feature booleans Expose more optional feature booleans, remove Timer from top level dolfinx Sep 11, 2024
@jhale jhale added this pull request to the merge queue Sep 11, 2024
Merged via the queue into main with commit 4abcfb8 Sep 11, 2024
27 of 28 checks passed
@jhale jhale deleted the jhale/add-has-scotch branch September 11, 2024 16:07
schnellerhase pushed a commit to schnellerhase/dolfinx that referenced this pull request Sep 11, 2024
…olfinx` (FEniCS#3335)

* Expose more optional feature booleans in public API.

* Update python/test/unit/fem/test_petsc_nonlinear_assembler.py

Co-authored-by: Jørgen Schartum Dokken <[email protected]>

* Top level namespace very cluttered - propose all this to common.

* Fix.

* Fix demos.

* Fix.

* Add has_petsc4py. has_petsc now refers to the state of the C++ library.

* Add has_petsc4py.

* If we cannot find petsc4py module, or DOLFINx was built without petsc4py
support, then exit.

* Check that DOLFINx Python was built with petsc4py.

* Fix logic.

* Ruff format.

* Revert moving feature to booleans to common.

* Ruff format.

---------

Co-authored-by: Jørgen Schartum Dokken <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants