pyamrex icon indicating copy to clipboard operation
pyamrex copied to clipboard

segmentation fault if AMReX is not initialized

Open BenWibking opened this issue 1 year ago • 13 comments
trafficstars

The Python interpreter segfaults if a function is called without initializing AMReX:

$  pyamrex git:(add-plotfiledata) ✗ python
Python 3.12.3 (main, Apr  9 2024, 08:09:14) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import amrex.space3d as amr
>>> amr.PlotFileData("./tests/projz04000/")
[1]    94346 segmentation fault  python

whereas this works fine:

$ pyamrex git:(add-plotfiledata) ✗ python
Python 3.12.3 (main, Apr  9 2024, 08:09:14) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import amrex.space3d as amr
>>> amr.initialize([])
Initializing AMReX (24.05)...
MPI initialized with 1 MPI processes
MPI initialized with thread support level 0
AMReX (24.05) initialized
<amrex.space3d.amrex_3d_pybind.AMReX object at 0x10127d270>
>>> amr.PlotFileData("./tests/projz04000/")
<amrex.space3d.amrex_3d_pybind.PlotFileData object at 0x10127ddb0>
>>> amr.finalize()
AMReX (24.05) finalized

BenWibking avatar May 31 '24 18:05 BenWibking

Discovered via this PR: https://github.com/AMReX-Codes/pyamrex/pull/320

BenWibking avatar May 31 '24 18:05 BenWibking

It's not surprising that a lot of amrex function won't work without amrex::Initialize called.

WeiqunZhang avatar May 31 '24 19:05 WeiqunZhang

No, but I don't think it should segfault. This is very surprising (bad) behavior for a Python program. Python scripts should never segfault.

BenWibking avatar May 31 '24 19:05 BenWibking

Can amrex be initialized when the module is imported?

BenWibking avatar Jun 01 '24 14:06 BenWibking

Hm, I am a bit hesitant to add a lot if if amrex initialized checks in pyAMReX. But if you add them upstream then we can just C++ throw and this will (should) propagate as a runtime exception to pyAMReX.

ax3l avatar Jun 04 '24 16:06 ax3l

Can amrex be initialized when the module is imported?

mpi4py style? Possibly... But there is a lot of runtime stuff people might want to pass to init

ax3l avatar Jun 04 '24 16:06 ax3l

Can amrex be initialized when the module is imported?

mpi4py style? Possibly... But there is a lot of runtime stuff people might want to pass to init

Is it possible to re-init after the first (default) init?

BenWibking avatar Jun 04 '24 17:06 BenWibking

Yes, for pyAMReX, we added fixes in AMReX that make sure we can finalize and reinit many times in a row in the same process.

I always considered import-time side-effects as an anti-pattern in Python. Like this (Sphinx RTD): https://github.com/AMReX-Codes/pyamrex/blob/c61add4fd44ad4ec69b131865375d76c68f12b29/docs/source/conf.py#L31-L32 or this (mpi4py): https://github.com/AMReX-Codes/pyamrex/blob/c61add4fd44ad4ec69b131865375d76c68f12b29/tests/conftest.py#L19-L21 I mean this is even confusing to linter tools...

I wonder if we can find another solution :thinking:

ax3l avatar Aug 14 '24 20:08 ax3l

An alternative could be to only allow access to pyAMReX functions with a content manager...

Something like

import amrex.space3d as amr

with amr.Initialized() as amx:
    amx.PlotFileData("./tests/projz04000/")

ax3l avatar Aug 14 '24 21:08 ax3l

That might work.

BenWibking avatar Aug 14 '24 21:08 BenWibking

What would be a good syntax?

from amrex.space3d import amr_initialize

with amr_initialize() as amr:
    amr.PlotFileData("./tests/projz04000/")

ax3l avatar Aug 15 '24 06:08 ax3l

Maybe something like:

from amrex.space3d import AmrContext:

with AmrContext(mpi_comm=None) as amr:
    amr.PlotFileData("./tests/projz04000/")

or

from amrex.space3d import AmrContext:

with AmrContext() as amr:
    amr.PlotFileData("./tests/projz04000/")

I think calling it initialize or something like that is confusing, since it actually returns a context object. On the other hand, calling it context doesn't have a counterpart in the C++ API. But if it's object-oriented context management anyway, then I think it makes more sense to use a constructor for a context object.

BenWibking avatar Aug 15 '24 14:08 BenWibking

Sounds good. Other names could be ActiveAmr or Activate or so if we want to avoid context.

ax3l avatar Sep 06 '24 18:09 ax3l