Fixes to new convolution code by astrofrog · Pull Request #8035 · astropy/astropy

astrofrog · 2018-10-30T08:50:42Z

This fixes #8032

If developers updated to the latest master version of astropy and were in develop mode, and did not rebuild extensions, they would get:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/bsipocz/munka/devel/astropy/astropy/convolution/__init__.py", line 10, in <module>
    from .convolve import convolve, convolve_fft, interpolate_replace_nans, convolve_models
  File "/Users/bsipocz/munka/devel/astropy/astropy/convolution/convolve.py", line 28, in <module>
    lib_path = glob.glob(os.path.join(os.path.dirname(__file__), 'lib_convolve*'))[0]
IndexError: list index out of range

so this PR makes the error message cleaner. For anyone curious, you can reproduce this error with:

python setup.py build_ext --inplace
rm astropy/convolution/lib_convolve.cpython-37m-darwin.so 
python -c 'import astropy.convolution'

Secondly it seems that importing faulthandler doesn't always work (see #8032) - so we should just not import it by default (it's easy for developers to import it in test scripts they are using, or in pytest using pytest-faulthandler).

cc @jamienoss

…e found

astropy-bot · 2018-10-30T08:50:46Z

Hi there @astrofrog 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

pllim · 2018-10-30T13:57:46Z

From a glance, it seems like there is no "slow Python" fallback if C-extension fails to build?

Perhaps a future PR, but is it possible to have that C-extension as optional at all? That is, will some of the convolution functions still run without a C-extension?

jamienoss · 2018-10-30T14:07:27Z

@astrofrog Thanks for making the changes.

@pllim

That is, will some of the convolution functions still run without a C-extension?

No, it's always been dependent on the C extension.

Perhaps a future PR, but is it possible to have that C-extension as optional at all?

Does anything else in Astropy use optional extensions like this? Also, have a missed something, why has this PR (and/or #8032) have anything to do with the extension having build issues?

astrofrog · 2018-10-30T15:04:51Z

@jamienoss - the issue wasn't fundamentally related to the convolution changes, it's just that the way the convolution code now looks for the extension means that @bsipocz got a cryptic error about glob rather than simply having an import of the C extension fail.

keflavich · 2018-10-30T15:16:52Z

This is a bit over my head. I don't want to "approve" it because I don't really know what's going on. Can someone else review more usefully?

pllim

LGTM

bsipocz

I guess it's OK now.

jamienoss · 2018-10-30T17:39:14Z

-if sys.platform.startswith('win'):
-    libConvolve = ctypes.windll.LoadLibrary(lib_path)
+lib_paths = glob.glob(os.path.join(os.path.dirname(__file__), 'lib_convolve*'))
+if len(lib_paths) > 0:


lib_paths is a list, isn't if lib_paths preferred?

Both work and I don't think it matters which one we use here.

jamienoss · 2018-10-30T18:14:59Z

-    libConvolve = ctypes.windll.LoadLibrary(lib_path)
+lib_paths = glob.glob(os.path.join(os.path.dirname(__file__), 'lib_convolve*'))
+if len(lib_paths) > 0:
+    if sys.platform.startswith('win'):


Think we need something like:

... #Search for library lib_paths = glob.glob(os.path.join(root, 'lib_convolve*')) lib_path ='' for path in lib_paths: if not path.endswith('.py'): lib_path = path break if not lib_path: raise Exception("Compiled convolution code is missing, try re-building astropy") #Load found library if sys.platform.startswith('win'): libConvolve = ctypes.windll.LoadLibrary(lib_path) else: libConvolve = ctypes.cdll.LoadLibrary(lib_path) ...

I think the exclusion soln. might be better than trying to be explicit for different OS lib suffixes. Neither are pleasant. Any thoughts anyone?

or

... lib_paths = glob.glob(os.path.join(root, 'lib_convolve*')) filtered_lib_paths = [path for path in lib_paths if not path.endswith('.py')] if not filtered_lib_paths: raise Exception("Compiled convolution code is missing, try re-building astropy") # Use the 1st found lib_path = filtered_lib_paths[0] #Load found library if sys.platform.startswith('win'): libConvolve = ctypes.windll.LoadLibrary(lib_path) else: libConvolve = ctypes.cdll.LoadLibrary(lib_path) ...

We can raise on if len(filtered_lib_paths) > 1?

What about .pyc files? We should make sure we include any kind of similar files. Any other extensions to exclude?

Not that endswith can semantically handle *.

pyc
pyo
pyx
pxd
~~pyd (checking now...)~~ This is the suffix for the windows built library.

~~Should it just go straight into the glob, instead?~~

Actually, I'm not sure if .pyd would kill the windows usage...?

Yep, we can't include .pyd in the exclusion list as this is what the the windows build produces.

pllim · 2018-10-30T18:38:15Z

Interesting CircleCI failure -- probably not related?

Worker 'gw3' crashed while running 'astropy/io/fits/tests/test_util.py::TestUtils::()::test_ignore_sigint'

jamienoss · 2018-10-30T18:42:47Z

@pllim it's not related.

jamienoss · 2018-10-30T18:43:38Z

~~Unless...~~ Nope, unrelated.

saimn · 2018-10-30T22:51:07Z

Numpy has a function to load libraries with ctypes, taking care of the cross-platform differences (numpy.ctypeslib.load_library). We use it for MPDAF without any problem so far (but also without Windows testing/support).
https://git-cral.univ-lyon1.fr/MUSE/mpdaf/blob/master/lib/mpdaf/tools/ctools.py#L50-57
https://docs.scipy.org/doc/numpy/reference/routines.ctypeslib.html#numpy.ctypeslib.load_library
https://github.com/numpy/numpy/blob/master/numpy/ctypeslib.py#L91

jamienoss · 2018-10-30T23:41:55Z

@saimn That sounds awesome! Thanks for the intel. I'll take it for a quick spin on windows tomorrow, but I imagine it works great, probably by design definition.

It explicitly solves the lib ext issue that we're having here.

libname : str
Name of the library, which can have ‘lib’ as a prefix, but without an extension.

jamienoss · 2018-10-31T14:29:19Z

+LIBRARY_PATH = os.path.dirname(__file__)

+try:
+    lib_convolve = load_library("lib_convolve", LIBRARY_PATH)


Awesome, nice and neat.

pllim · 2018-10-31T14:31:23Z

This is okay to merge when you think it is ready. Thanks!

jamienoss · 2018-10-31T14:33:18Z

@astrofrog I opened #8050, just an FYI incase you felt it necessary to add the test to this PR.

jamienoss · 2018-10-31T14:58:59Z

@astrofrog I tested this on windows and it works as it should. Sorry, I wasn't sure what you dev on and it was quick enough for me to test.

astrofrog · 2018-10-31T14:59:14Z

I'll merge this for now, I don't have time to figure out a test, but if you do please open a PR!

jamienoss · 2018-10-31T15:08:26Z

Thanks again @saimn, nicely saved.

Fixes to new convolution code

jamienoss · 2018-11-02T14:37:16Z

@pllim Would you mind quickly adding the convolution label, please, cheers.

astrofrog added 3 commits October 30, 2018 08:47

Don't automatically import faulthandler

e1e414d

Reduce flake8 warnings

cc74f3b

Give more explicit error message if compiled convolution code can't b…

410cab4

…e found

astrofrog added Bug Affects-dev PRs and issues that do not impact an existing Astropy release labels Oct 30, 2018

astrofrog added this to the v3.1 milestone Oct 30, 2018

astrofrog requested a review from bsipocz October 30, 2018 08:50

astrofrog requested a review from keflavich October 30, 2018 08:50

pllim reviewed Oct 30, 2018

View reviewed changes

Comment thread astropy/convolution/convolve.py Outdated

pllim approved these changes Oct 30, 2018

View reviewed changes

nden mentioned this pull request Oct 30, 2018

convolution cannot be imported #8032

Closed

bsipocz approved these changes Oct 30, 2018

View reviewed changes

bsipocz mentioned this pull request Oct 30, 2018

Speed up creation of simple composite units #7649

Merged

jamienoss reviewed Oct 30, 2018

View reviewed changes

Minor cleanup

c61527d

astrofrog force-pushed the convolution-fixes branch from 4de4f28 to c61527d Compare October 30, 2018 18:09

jamienoss reviewed Oct 30, 2018

View reviewed changes

astrofrog force-pushed the convolution-fixes branch from 7062701 to c61527d Compare October 30, 2018 22:31

jamienoss mentioned this pull request Oct 30, 2018

Decide on policy for writing and interfacing with C code #8036

Open

Use load_library

2377b36

jamienoss reviewed Oct 31, 2018

View reviewed changes

astrofrog merged commit f533845 into astropy:master Oct 31, 2018

bsipocz pushed a commit that referenced this pull request Oct 31, 2018

Merge pull request #8035 from astrofrog/convolution-fixes

5b96405

Fixes to new convolution code

astrofrog added the convolution label Nov 2, 2018

bsipocz reviewed Nov 2, 2018

View reviewed changes

Comment thread astropy/convolution/convolve.py

astrofrog deleted the convolution-fixes branch November 14, 2018 15:23

bsipocz mentioned this pull request Dec 29, 2018

[3.1] Convolve problems in some affiliated packages #8312

Closed

Conversation

astrofrog commented Oct 30, 2018

Uh oh!

astropy-bot Bot commented Oct 30, 2018

Uh oh!

pllim commented Oct 30, 2018

Uh oh!

Uh oh!

jamienoss commented Oct 30, 2018

Uh oh!

astrofrog commented Oct 30, 2018

Uh oh!

keflavich commented Oct 30, 2018

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienoss Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienoss Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienoss Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienoss Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienoss Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 30, 2018

Uh oh!

jamienoss commented Oct 30, 2018

Uh oh!

jamienoss commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamienoss commented Oct 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 31, 2018

Uh oh!

jamienoss commented Oct 31, 2018

Uh oh!

jamienoss commented Oct 31, 2018

Uh oh!

astrofrog commented Oct 31, 2018

Uh oh!

jamienoss commented Oct 31, 2018

Uh oh!

jamienoss commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jamienoss Oct 30, 2018 •

edited

Loading

jamienoss Oct 30, 2018 •

edited

Loading

jamienoss Oct 30, 2018 •

edited

Loading

jamienoss Oct 30, 2018 •

edited

Loading

jamienoss Oct 30, 2018 •

edited

Loading

jamienoss commented Oct 30, 2018 •

edited

Loading

saimn commented Oct 30, 2018 •

edited

Loading

jamienoss commented Nov 2, 2018 •

edited

Loading