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

Python: Fix Interactive Inspection #265

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Sep 28, 2022

When autocompleting the ImpactX() Python class, IPython uses Jedi to look into the getters of properties.

Jedi does execute properties and in general is not very careful to avoid code execution.

By default, the ParmParse::get() we use would throw an amrex::Abort(), which exits the interpreter.

We could and should use amrex.throw_exception = 1 and amrex.signal_handling = 0 to make this a std::runtime_error. Unfortunately, this is still not sufficient to catch it, because C++ exception handling across shared libraries needs extra care.

Thus, we just do a helper function, save some lines of code, and throw directly from ImpactX's Python module.

Thanks to @n01r for spotting this originally.
Follow-up to #260 and #223

@ax3l ax3l added bug Something isn't working bug: affects latest release Bug also exists in latest release version component: python Python bindings labels Sep 28, 2022
@ax3l ax3l requested a review from n01r September 28, 2022 03:08
@ax3l ax3l marked this pull request as ready for review September 28, 2022 03:08
@ax3l ax3l force-pushed the fix-py-jedi-getter-prop branch from 45f1a26 to f0dc6d2 Compare September 28, 2022 03:08
void init_ImpactX(py::module& m)
namespace detail
{
/** Helper Function for Property Getters
Copy link
Member Author

Choose a reason for hiding this comment

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

We can check in the future if we find a reasonable location to set default parameters without setting them multiple times throughout the code.

Maybe we need a ReadParameters-like function called set_default_parameters after all... :-S

When autocompleting the `ImpactX()` Python class, Ipython
uses Jedi to look into the getters of properties. By default,
the `ParmParse::get()` we use would throw an `amrex::Abort()`,
which exits the interpreter.

We could and should use `amrex.throw_exception = 1` and
`amrex.signal_handling = 0` to make this a `std::runtime_error`.
Unfortunately, this is still not sufficient to catch it, because
C++ exception handlign accross shared libraries needs extra care.

Thus, we just do a helper function, save some lines of code, and
throw directly from ImpactX's Python module.
Copy link
Member

@n01r n01r left a comment

Choose a reason for hiding this comment

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

Looks great! :)

@n01r
Copy link
Member

n01r commented Sep 28, 2022

Also doesn't crash on Mac, anymore. ✨

@ax3l ax3l merged commit 3b52052 into ECP-WarpX:development Sep 29, 2022
@ax3l ax3l deleted the fix-py-jedi-getter-prop branch September 29, 2022 17:26
@ax3l ax3l requested a review from RTSandberg September 29, 2022 17:26
Copy link
Member

@RTSandberg RTSandberg left a comment

Choose a reason for hiding this comment

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

This looks good to me and works fine on my Mac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: affects latest release Bug also exists in latest release version bug Something isn't working component: python Python bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants