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

ENH: Require Lapack 3.4.0 #8670

Merged
merged 23 commits into from
Jun 1, 2018
Merged

Conversation

insertinterestingnamehere
Copy link
Member

This is a reboot of #6051.

It bumps the required LAPACK version to 3.4.0 and updates the Cython BLAS/LAPACK wrappers accordingly.

@insertinterestingnamehere
Copy link
Member Author

Thus far this just implements this for Windows and Linux. I'm putting this up now to see how the CI tests go. I haven't removed the various custom accelerate wrappers or updated the CI scripts yet, so, for the moment, failures on OSX are expected.

@ilayn
Copy link
Member

ilayn commented Apr 3, 2018

Most of the errors are about missing qr_insertwhich calls ?qptr and ?rptr routines. But the rest is surprisingly happy (apart from PEP8 details) !

@rgommers rgommers added enhancement A new feature or improvement scipy.linalg labels Apr 4, 2018
@rgommers
Copy link
Member

rgommers commented Apr 4, 2018

Nice and compact:)

Could possibly still be included in 1.1.x if it gets merged within about one week.

from LAPACK 3.1.0 to 3.6.0. It also provides some of the
in LAPACK 3.4.0 except for ``zcgesv`` since its interface is not consistent
from LAPACK 3.4.0 to 3.6.0. It also provides some of the
from LAPACK 3.4.0 to 3.6.0. It also provides some of the
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate line

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Thanks.

return name

def fix_line_length(subroutine):
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this would be simpler if we integrated the line-wrapping into the code that generates the subroutine (fort_subroutine_wrapper). It would at least remove the need to do fiddly parsing here.

Copy link
Member

Choose a reason for hiding this comment

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

Also do we really need line wrapping? Nobody is editing this file anyways

Copy link
Member Author

Choose a reason for hiding this comment

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

The line wrapping is needed to get around the maximum column limitation in Fortran 77. Including it in the original code generation is a good idea though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so after looking at this a bit more, it turns out that the xblas functions were the only things hitting this corner case, so we could just pull this logic out entirely for LAPACK 3.4.0. That said, I don't really want to deal with this breaking again when doing some future update, so I'm pushing a version that handles the max line length issue by just vertically stacking the arguments in the generated Fortran subroutines. That greatly simplifies the logic for generating the wrapper but should also avoid potential trouble with line length later.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a better solution indeed.

@@ -689,7 +727,8 @@ def make_all(blas_signature_file="cython_blas_signatures.txt",

comments = ["This file was generated by _generate_pyx.py.\n",
"Do not edit this file directly.\n"]
ccomment = ''.join(['/* ' + line.rstrip() + ' */\n' for line in comments]) + '\n'
ccomment = ''.join(['/* ' + line.rstrip() + ' */\n'
for line in comments]) + '\n'
Copy link
Member

Choose a reason for hiding this comment

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

This produces a double newline after the last element. Not sure if that's intended.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yah, that's not needed. I'll remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, looking at the generated files, the extra newline does help readability even though it's not strictly necessary, so I'll leave it there.

@insertinterestingnamehere
Copy link
Member Author

Yah, I'm amazed that it built and linked at all on OSX. For some reason the unavailable symbols aren't detected till load time. It must only be listing the first missing symbol it finds though, since it's only showing one missing symbol but many new routines were added between 3.2.1 and 3.4.0.

@insertinterestingnamehere
Copy link
Member Author

1.1.x is definitely my goal, though that really depends on how deep the OSX rabbithole is.

@@ -58,7 +62,7 @@ def sigs_from_dir(directory, outfile, manual_wrappers=None, exclusions=None):
exclusions += [get_sig_name(l) for l in manual_wrappers.split('\n')]
signatures = []
for filename in files:
name = filename.split('\\')[-1][:-2]
name = os.path.basename(filename)[:-2]
Copy link
Member

Choose a reason for hiding this comment

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

Is the [:-2] stripping a file extension? If so, use splitext.

return name + dims[name]
if ('x' in name or 'y' in name):
return name + special_cases.get(funcname[1:], dict()).get(name, '(n)')
name += special_cases.get(funcname[1:], dims).get(name, dims.get(name, ''))
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit hairy. How about:

special_cases = defaultdict(dict, ladiv=...)

and then in process_fortran_name:

special = special_cases[funcname[1:]]
if 'x' in name or 'y' in name:
    suffix = special.get(name, '(n)')
else:
    suffix = special.get(name, '')
return name + suffix

included = ['zladiv', 'zdotu', 'zdotc']
excluded = ['chla_transtype', 'clanhf', 'slansf']
if (name[0] in 'cs' and name not in excluded) or name in included:
return "w" + name
Copy link
Member

Choose a reason for hiding this comment

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

Use single-quoted 'w' for consistency.

@@ -645,7 +661,8 @@ def split_signature(sig):

def filter_lines(lines):
lines = [line.strip() for line in lines
if line != '\n' and line[0] != '#']
if line not in ['\n', '\r\n']
and line[0] != '#']
Copy link
Member

Choose a reason for hiding this comment

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

I'd split this into two passes:

lines = [line for line in map(str.strip, lines) if line and not line.startswith('#')]

@pv pv added this to the 1.2.0 milestone Apr 12, 2018
@insertinterestingnamehere
Copy link
Member Author

WRT the OSX CI tests, for some reason I was unable to get the build to pick up any LAPACK other than the one numpy is built with. Specifying a different BLAS in the site.cfg file didn't help and I'm not aware of any other way to do that.

Given that limitation, for the CI tests, I do have a script to download an openblas binary and then build both numpy and scipy against that, but it ended up being easier to just drop the whole thing into a miniconda install and then use the openblas linked numpy available there. Any preference in general? The miniconda route requires less actual compilation, but it does download a bit more. It would also make it easier to test against MKL if we wanted to add that.

@matthew-brett
Copy link
Contributor

In case it is at all useful, here is how I built Scipy with OpenBLAS a while back : https://github.com/matthew-brett/scipy-wheels/tree/with-openblas-on-osx . The mechanism could be simplified a bit now because of changes in our wheel building.

I have some vague memory that you have to put the site.cfg in your home directory, maybe because it is numpy doing the processing:

https://github.com/scipy/scipy/blob/master/site.cfg.example#L7

but please correct me if I'm wrong about that.

@insertinterestingnamehere
Copy link
Member Author

Okay, I managed to get scipy to link against something other than what was used to build numpy. I think the issue I ran up against was just a peculiarity of how I had set things up to test locally. The expected workflow works in the CI environment, but not with a numpy pulled in from the default conda channel. Either way, it's working now.

Now that it's linking against the newer BLAS, I've found that the gfortran ABI changed between gcc 6 and gcc 7, so I shouldn't have the downloaded libopenblas load libgfortran ABI version 4 since it was built with ABI version 3. Would it be preferable to use a newer binary or to install an older gcc/gfortran from homebrew for the CI tests?

@ilayn
Copy link
Member

ilayn commented Apr 21, 2018

OSX build didn't like the solution apparently.

@insertinterestingnamehere
Copy link
Member Author

Sorry I wasn't clear. Currently it succeeds in building against the new library, but the test runs fail because it is looking for a different version of libgfortran than is provided by the latest homebrew. I could just use install_name_tool to change libopenblas to use the newer version, but the reason the gfortran devs changed the shared object version number was because of ABI incompatibilities in libgfortran between gfortran 7 and previous versions. To get around this, I need to either pin the CI tests to use an older gfortran so the older version of libgfortran is available or I can use a binary of OpenBLAS that was built with the newer gfortran.

So, does anyone have a binary of OpenBLAS built with gcc7 handy? If so, great. Either way, I'll look in to using an older gfortran from homebrew for the time being.

introduced after LAPACK 3.2.1. Functions introduced after that version
that would otherwise need ABI wrappers for clapack ABI compatibility are
not supported by Accelerate at all. That's the only place where the
clapack abi is used, so ABI wrappers for these routines are not needed.
For detection of BLAS/LAPACK, use our own copies lapack_opt_info and
blas_opt_info which ignore OSX Accelerate.
The top level setup.py file now checks that a BLAS/LAPACK installation
has been found, so it is no longer necessary to check for that when
linking against BLAS and LAPACK in the lower level setup.py files.
@insertinterestingnamehere
Copy link
Member Author

To what extent do we want to discuss the site.cfg machinery in the docs here? Should that be included at all in the top-level INSTALL file? What about the OSX-specific page in the docs? The Windows page does not mention it, but the Linux one does. It's not really an OS-specific detail though.

------------------------

You will also need to install a library providing the BLAS and LAPACK
interfaces. ATLAS, OpenBLAS, and MKL all work. OpenBLAS can be installed
Copy link
Member

Choose a reason for hiding this comment

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

How about changing the order to "OpenBLAS, MKL and ATLAS"? ATLAS is the worst of the three, and the most difficult to obtain.

Another sentence to add is something like "if you're using numpy from the Anaconda default channel, it will use MKL, and SciPy will then be built against that same MKL install".

@matthew-brett
Copy link
Contributor

While I'm here - I wanted to point to https://github.com/MacPython/numpy-wheels/blob/master/on_openblas.rst - it describes the Travis-CI / Appveyor build machinery for OpenBLAS, which is a bit disorganized at the moment.

@ilayn
Copy link
Member

ilayn commented Jun 1, 2018

I think build instructions can be developed independent from this PR. As we have done with Building on Windows we can make another for OSX environment separately to be more focused on building.

@insertinterestingnamehere
Copy link
Member Author

Yah, I'd rather not let documentation concerns hold up getting the other changes in to master. The longer the non-doc changes are in master, the more time we have to see if they cause trouble for anyone.

Should we merge the doc edits here as is? I can remove them if needed.

@matthew-brett
Copy link
Contributor

Sorry, my link was just an FYI, not a suggestion for the documentation.

Copy link
Member

@pv pv left a comment

Choose a reason for hiding this comment

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

I don't have further comments, looks good to go from my side. I agree polishing the build instructions should be done separately.

No need to remove the documentation changes made here, though, I think.

@rgommers
Copy link
Member

rgommers commented Jun 1, 2018

LGTM too - let's leave doc changes and possibly other little tweaks to future PRs. Merging this so people will start trying it out.

Thanks a lot @insertinterestingnamehere, all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement scipy.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants