Fix segfault when importing _libwarpx without initializing WarpX #2580

KZhu-ME · 2021-11-18T19:46:10Z

When doing data analysis, we sometimes need to import modules that use and import _libwarpx, which causes a segfault when it tries to call finalize() when no WarpX simulation was initialized. This PR adds a check to see whether WarpX was actually initialized before calling finalize().

lgtm-com · 2021-11-18T20:28:19Z

This pull request introduces 1 alert when merging e8894d1 into 1182fee - view on LGTM.com

new alerts:

1 for Unused local variable

dpgrote

This looks good - thanks!

peterscherpelz

One style suggestion :)

Python/pywarpx/_libwarpx.py

peterscherpelz

@KZhu-ME It turns out that while python will read from module-level variables if there's no local variable in scope, writing a variable within a function is by default always a local variable. This global call seems to fix things for me.
cf https://stackoverflow.com/questions/7060711/accessing-module-level-variables-from-within-a-function-in-the-module

Python/pywarpx/_libwarpx.py

Co-authored-by: Peter Scherpelz <31747262+peterscherpelz@users.noreply.github.com>

ax3l · 2021-11-19T23:27:10Z

Python/pywarpx/_libwarpx.py

@@ -334,7 +336,7 @@ def finalize(finalize_mpi=1):
    the end of your script.

    '''
-    if warpx_initialized == True:
+    if warpx_initialized:


Would this be needed here, too?

Suggested change

if warpx_initialized:

global warpx_initialized

if warpx_initialized:

Ah maybe not, because it's global anyway. (Is it good style though to mark it anyway?)

Yeah, the Stack Overflow link above says that since functions will access module variables after checking local variables for read operations, python doesn't require the global call here. The problem is the default for write is to create the local variable, not modify the module var.

I'm indifferent on a style choice to specify it or not.

Got ya, agreed.

Style-wise: I think since we generally try to avoid globals and this is kind of an exception for a module global, I would slightly prefer to add it here, just to be explicit and to avoid confusion.

ax3l

General question: could we express this also as a member of the libwarpx object instead?

Seems cleaner to me.

dpgrote · 2021-11-21T00:53:45Z

I should have seen that right away, needing to use global. I've seen this type of thing many times.

BTW, there is a possible deeper solution here. The warpx_finalize routine is only calling the routine WarpX::ResetInstance which is deleting the WarpX instance. Could that delete be put inside an if test?

if (m_instance) {
delete m_instance;
}

removing earlier approval to indicate open follow-ups.

peterscherpelz · 2021-11-22T17:45:39Z

General question: could we express this also as a member of the libwarpx object instead?

@ax3l I guess this one's a matter of preference - my instinct is that since libwarpx is specifically a ctypes.CDLL object, it's good to not assign it custom variables that aren't normally part of that object type. But syntactically yes it can work.

peterscherpelz · 2021-11-22T17:51:44Z

I should have seen that right away, needing to use global. I've seen this type of thing many times.

BTW, there is a possible deeper solution here. The warpx_finalize routine is only calling the routine WarpX::ResetInstance which is deleting the WarpX instance. Could that delete be put inside an if test?

if (m_instance) {
delete m_instance;
}

@dpgrote It's actually not the WarpX finalize call but the AMReX finalize call that seems to cause the major difficulties, including MPI finalization. Since that's a much more complicated function, figuring out how to make it more robust against not-yet-initialized AMReX seems like a bigger project.

PhilMiller · 2021-11-22T19:01:48Z

I should have seen that right away, needing to use global. I've seen this type of thing many times.

BTW, there is a possible deeper solution here. The warpx_finalize routine is only calling the routine WarpX::ResetInstance which is deleting the WarpX instance. Could that delete be put inside an if test?

if (m_instance) {
delete m_instance;
}

The C++ definition of delete accepts null pointer values, and is a no-op in that case.

dpgrote · 2021-11-22T20:13:06Z

Yikes, I missed the call to amrex finalize. That's what I get for trying to do some work on the weekend.

…x_libwarpx_import

…WarpX into kzhu/fix_libwarpx_import

lgtm-com · 2021-11-24T20:22:43Z

This pull request introduces 1 alert and fixes 1 when merging 081fd0d into 6a3f6d4 - view on LGTM.com

new alerts:

1 for Syntax error

fixed alerts:

1 for 'import *' may pollute namespace

ax3l

LGTM, thank you!

ax3l · 2021-11-30T16:41:08Z

Examples/Tests/restart/PICMI_inputs_runtime_component_analyze.py

@@ -97,6 +97,7 @@
    if arg.startswith("amr.restart"):
        restart_file_name = arg.split("=")[1]
        sim.amr_restart = restart_file_name
+        sys.argv.remove(arg)


@KZhu-ME can you comment why this change was needed? :)
Seems unrelated.

This CI test errored out because with the message that there was an unrecognized command line argument amr.restart. I also encountered this error locally, and have ran into this issue in the past when trying to add arguments not recognized by the warpx parser. Not sure why it only started failing now for this PR though...

* Fix Init of Vector Members (ECP-WarpX#2595) Fix default init of `Vector` member variables. The old construct is not valid C++. https://stackoverflow.com/a/11491003/2719194 * C++17: Work-Around NVCC gatherParticles (ECP-WarpX#2596) The `noexcept` lambda does not compile in C++17 mode due to an NVCC compiler bug, at least in NVCC 11.3.109. Compiles in C++14 mode with the same compiler. * requirements.txt - PICMI development version (ECP-WarpX#2588) Document in `requirements.txt` on how to install a pre-release version of PICMI. * CONTRIBUTING: Update/Modernize (ECP-WarpX#2600) * CONTRIBUTING: Update/Modernize - Add GitHub account setup - Add local git setup - Modernize for CMake workflows * Apply suggestions by Edoardo Co-authored-by: Edoardo Zoni <59625522+EZoni@users.noreply.github.com> * Replaced duplicated current deposition documentation (ECP-WarpX#2604) * Throwing a warning if particle_shape>1 with EB (ECP-WarpX#2592) * Aborting if particle_shape!=1 with EB * Throw warning instead of aborting * Checking at runtime if EB is initialized * Added missing preprocessor directive * Ignoring an unused variable * Fix typo * Improve style Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja> * Fix segfault when importing _libwarpx without initializing WarpX (ECP-WarpX#2580) * Added check for if warpx was initialized when calling finalize * Renamed to be warpx_initialized * Fixed reference to global variable Co-authored-by: Peter Scherpelz <31747262+peterscherpelz@users.noreply.github.com> * Changed global variable to member of libwarpx * Fixed syntax errors * Remove custom arg from argv to avoid parmparse error Co-authored-by: Peter Scherpelz <31747262+peterscherpelz@users.noreply.github.com> * Added parallel pragma to ApplyBoundaryConditions (ECP-WarpX#2612) * Note that CCache 4.2 introduced large CUDA improvements (ECP-WarpX#2606) * Dimensionality Docs: Default (ECP-WarpX#2609) Just adds the note that 3D is the default geometry. * AMReX: Weekly Update (ECP-WarpX#2613) * MergeBuffersForPlotfile: Barrier (ECP-WarpX#2608) Make sure that all MPI ranks are in sync, i.e., have closed the files that they wrote, before trying to merge them. * Fix installation location for libraries (ECP-WarpX#2583) During configuration the installation location for libraries is given by dumping the cmake variable `CMAKE_INSTALL_LIBDIR`. This commit adjusts the installation of WarpX libraries (WarpX_LIB=ON) to respect this setting. Co-authored-by: Rolf Pfeffertal <tropf@users.noreply.github.com> * Release 21.12 (ECP-WarpX#2614) * AMReX: 21.12 * PICSAR: 21.12 * WarpX: 21.12 * div(E,B) Cleaning Options for PSATD (ECP-WarpX#2403) * Implement div(E)/div(B) Cleaning with Standard PSATD * Cleaning * Update Benchmark * Add Nodal Synchronization of F,G * OneStep_multiJ: Nodal Syncs, Damp PML * OneStep_multiJ: Push PSATD Fields in PML * div Cleaning Defaults (Domain v. PML) * Include Fix of ECP-WarpX#2429 until Merged * Reset Benchmark of Langmuir_multi_psatd_div_cleaning * Multi-J: Remove PML Support * Include Fix of ECP-WarpX#2474 Until Merged * Exchange All Guard Cells for F,G * Fix Defaults * Update Test, Reset Benchmark * Fix Defaults * Cleaning * Default update_with_rho=1 if do_dive_cleaning=1 * Update CI Test pml_psatd_dive_divb_cleaning * Replace Warning with Abort * Add 2D Langmuir Test w/ MR & PSATD (ECP-WarpX#2605) * Add 2D Langmuir Test w/ MR & PSATD * Add Missing Compile String * Fix out-of-bound in Inverse FFT of F,G (ECP-WarpX#2619) * Mention that the potentail should be constant inside EB (ECP-WarpX#2618) * Mention that the potentail should be constant inside EB * Update text * Replace AMREX_SPACEDIM: Boundary & Parallelization (ECP-WarpX#2620) * AMREX_SPACEDIM : Boundary Conditions * AMREX_SPACEDIM : Parallelization * Fix compilation * Update Source/Parallelization/WarpXComm_K.H * Fix out-of-bound in the initialization of EB (ECP-WarpX#2607) * Call FillBoundary when initializing EB * Avoid out-of-bound * Bug fix * Apply suggestions from code review * update version number Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja> Co-authored-by: Edoardo Zoni <59625522+EZoni@users.noreply.github.com> Co-authored-by: Remi Lehe <remi.lehe@normalesup.org> Co-authored-by: Lorenzo Giacomel <47607756+lgiacome@users.noreply.github.com> Co-authored-by: Kevin Z. Zhu <86268612+KZhu-ME@users.noreply.github.com> Co-authored-by: Peter Scherpelz <31747262+peterscherpelz@users.noreply.github.com> Co-authored-by: David Grote <grote1@llnl.gov> Co-authored-by: Phil Miller <phil@intensecomputing.com> Co-authored-by: s9105947 <80697868+s9105947@users.noreply.github.com> Co-authored-by: Rolf Pfeffertal <tropf@users.noreply.github.com> Co-authored-by: Prabhat Kumar <89051199+prkkumar@users.noreply.github.com>

KZhu-ME added 2 commits November 18, 2021 11:41

Added check for if warpx was initialized when calling finalize

763bb5c

Renamed to be warpx_initialized

e8894d1

ax3l requested review from ax3l and dpgrote November 19, 2021 00:42

ax3l self-assigned this Nov 19, 2021

ax3l added the component: Python Python layer label Nov 19, 2021

dpgrote previously approved these changes Nov 19, 2021

View reviewed changes

peterscherpelz reviewed Nov 19, 2021

View reviewed changes

Python/pywarpx/_libwarpx.py Outdated Show resolved Hide resolved

peterscherpelz reviewed Nov 19, 2021

View reviewed changes

Python/pywarpx/_libwarpx.py Outdated Show resolved Hide resolved

Fixed reference to global variable

57d2602

Co-authored-by: Peter Scherpelz <31747262+peterscherpelz@users.noreply.github.com>

ax3l reviewed Nov 19, 2021

View reviewed changes

KZhu-ME added 3 commits November 24, 2021 11:39

Merge branch 'development' of github.com:ECP-WarpX/WarpX into kzhu/fi…

0879f8e

…x_libwarpx_import

Changed global variable to member of libwarpx

20e81a4

Merge branch 'kzhu/fix_libwarpx_import' of github.com:ModernElectron/…

081fd0d

…WarpX into kzhu/fix_libwarpx_import

KZhu-ME added 2 commits November 29, 2021 09:05

Fixed syntax errors

a51e2dc

Remove custom arg from argv to avoid parmparse error

c774e6f

ax3l mentioned this pull request Nov 30, 2021

CONTRIBUTING: Update/Modernize #2600

Merged

ax3l approved these changes Nov 30, 2021

View reviewed changes

ax3l reviewed Nov 30, 2021

View reviewed changes

ax3l merged commit 11a5bf5 into ECP-WarpX:development Nov 30, 2021

ax3l mentioned this pull request Nov 30, 2021

Add a function returning whether amrex::Initialize has been called or not. AMReX-Codes/amrex#2501

Merged

5 tasks

KZhu-ME deleted the kzhu/fix_libwarpx_import branch March 22, 2022 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix segfault when importing _libwarpx without initializing WarpX #2580

Fix segfault when importing _libwarpx without initializing WarpX #2580

KZhu-ME commented Nov 18, 2021

lgtm-com bot commented Nov 18, 2021

dpgrote left a comment

peterscherpelz left a comment

peterscherpelz left a comment

ax3l Nov 19, 2021 •

edited

Loading

peterscherpelz Nov 19, 2021

ax3l Nov 24, 2021 •

edited

Loading

ax3l left a comment

dpgrote commented Nov 21, 2021 •

edited

Loading

peterscherpelz commented Nov 22, 2021 •

edited

Loading

peterscherpelz commented Nov 22, 2021

PhilMiller commented Nov 22, 2021

dpgrote commented Nov 22, 2021

lgtm-com bot commented Nov 24, 2021

ax3l left a comment

ax3l Nov 30, 2021

KZhu-ME Nov 30, 2021

	if warpx_initialized:
	global warpx_initialized
	if warpx_initialized:

Fix segfault when importing _libwarpx without initializing WarpX #2580

Fix segfault when importing _libwarpx without initializing WarpX #2580

Conversation

KZhu-ME commented Nov 18, 2021

lgtm-com bot commented Nov 18, 2021

dpgrote left a comment

Choose a reason for hiding this comment

peterscherpelz left a comment

Choose a reason for hiding this comment

peterscherpelz left a comment

Choose a reason for hiding this comment

ax3l Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

peterscherpelz Nov 19, 2021

Choose a reason for hiding this comment

ax3l Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

ax3l left a comment

Choose a reason for hiding this comment

dpgrote commented Nov 21, 2021 • edited Loading

peterscherpelz commented Nov 22, 2021 • edited Loading

peterscherpelz commented Nov 22, 2021

PhilMiller commented Nov 22, 2021

dpgrote commented Nov 22, 2021

lgtm-com bot commented Nov 24, 2021

ax3l left a comment

Choose a reason for hiding this comment

ax3l Nov 30, 2021

Choose a reason for hiding this comment

KZhu-ME Nov 30, 2021

Choose a reason for hiding this comment

ax3l Nov 19, 2021 •

edited

Loading

ax3l Nov 24, 2021 •

edited

Loading

dpgrote commented Nov 21, 2021 •

edited

Loading

peterscherpelz commented Nov 22, 2021 •

edited

Loading