dgasmith / gau2grid

Fast computation of a gaussian and its derivative on a grid.
https://gau2grid.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
29 stars 17 forks source link

gau2grid not working on any other architectures than x86_64 #25

Closed susilehtola closed 6 years ago

susilehtola commented 6 years ago

gau2grid has been approved in Fedora, but the builds fail on every other architecture than x86_64.

https://koji.fedoraproject.org/koji/taskinfo?taskID=29866555

It appears there are at least two separate issues in the code.

First, on i686 and armv7hl I get

[ 14%] Generating gau2grid.h, gau2grid_phi.c, gau2grid_deriv1.c, gau2grid_deriv2.c, gau2grid_spherical.c, gau2grid_helper.c
/usr/bin/python3.7 -c "import sys;                                      sys.path.append('/builddir/build/BUILD/gau2grid-1.2.0');                                      import gau2grid as gg;                                      gg.c_gen.generate_c_gau2grid(8, path='/builddir/build/BUILD/gau2grid-1.2.0/objdir', cartesian_order='row', spherical_order='gaussian')"
Traceback (most recent call last):
  File "/usr/lib/python3.7/site-packages/numpy/lib/format.py", line 527, in _read_array_header
    dtype = numpy.dtype(d['descr'])
TypeError: data type "<f16" not understood
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/builddir/build/BUILD/gau2grid-1.2.0/gau2grid/__init__.py", line 5, in <module>
    from . import RSH
  File "/builddir/build/BUILD/gau2grid-1.2.0/gau2grid/RSH.py", line 43, in <module>
    _load_saved_rsh_coefs()
  File "/builddir/build/BUILD/gau2grid-1.2.0/gau2grid/RSH.py", line 29, in _load_saved_rsh_coefs
    coefs = rsh_data[key_name + "coeffs"]
  File "/usr/lib/python3.7/site-packages/numpy/lib/npyio.py", line 251, in __getitem__
    pickle_kwargs=self.pickle_kwargs)
  File "/usr/lib/python3.7/site-packages/numpy/lib/format.py", line 642, in read_array
    shape, fortran_order, dtype = _read_array_header(fp, version)
  File "/usr/lib/python3.7/site-packages/numpy/lib/format.py", line 530, in _read_array_header
    raise ValueError(msg % (d['descr'],))
ValueError: descr is not a valid dtype descriptor: '<f16'

Second, on ppc64le, aarch64 and s390x I get

[ 28%] Building C object CMakeFiles/gg.dir/gau2grid_phi.c.o
/usr/bin/cc -Dgg_EXPORTS  -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mcpu=power8 -mtune=power8 -fasynchronous-unwind-tables -fstack-clash-protection -DNDEBUG -fPIC   -std=c99 -o CMakeFiles/gg.dir/gau2grid_phi.c.o   -c /builddir/build/BUILD/gau2grid-1.2.0/objdir/gau2grid_phi.c
/builddir/build/BUILD/gau2grid-1.2.0/objdir/gau2grid_phi.c:13:10: fatal error: mm_malloc.h: No such file or directory
 #include <mm_malloc.h>
          ^~~~~~~~~~~~~
compilation terminated.
dgasmith commented 6 years ago

On the first one it looks like they do not support long double types which we can work around.

For the second one I guess there is no aligned memory calls for these architectures which could be fun. Is there a way to determine x86 architecture in headers? I believe we would need to change all of our pragma's and align calls.

susilehtola commented 6 years ago

The mm_malloc issue should be solvable just by switching to using the C11 standard library

void *aligned_alloc( size_t alignment, size_t size );

function.

susilehtola commented 6 years ago

Psi4 anyway requires C++11, so that shouldn't be a problem, right?

dgasmith commented 6 years ago

It shouldn't be an issue for Psi4, but several other programs use it as well now. Let me check with them.

dgasmith commented 6 years ago

Looks like for MSVC we will need _aligned_malloc as seen here.

susilehtola commented 6 years ago

Of course, you could try to conditionalize: check which one is available and use that.

It shouldn't be an issue for Psi4, but several other programs use it as well now. Let me check with them.

Which ones?

dgasmith commented 6 years ago

Yes, adding additional conditionals is the straightforward option.

Avogadro and Chronus are both looking into adding the library AFAIK.

susilehtola commented 6 years ago

This is still holding up the Psi4 update in Fedora.

dgasmith commented 6 years ago

I will try to get to it this weekend.

yurivict commented 6 years ago

Same in the FreeBSD port: it only succeeds on amd64.

susilehtola commented 6 years ago

Any news? It'd be nice to get this fixed so I could build gau2grid and update Psi4 from the ancient revision currently on Fedora.

dgasmith commented 6 years ago

Please try a build from the current master. The linked PR should have fixed this.

susilehtola commented 6 years ago

But the linked PR hasn't been merged..?

susilehtola commented 6 years ago

And then there's the long double issue on some architectures.

dgasmith commented 6 years ago

Sure merged, long double should have been fixed in #26.

susilehtola commented 6 years ago

It builds! Good to go.

dgasmith commented 6 years ago

Great! Thanks for checking. @loriab Should I mint a new release?

loriab commented 6 years ago

Not for my sake, but Susi and Yuri would probably prefer to pull from a tag than a sha.

susilehtola commented 6 years ago

Well, I already had to use the github commit hash to build on Fedora, so not for my sake either...

mbanck commented 6 years ago

I think a new (minor?) release would be warranted, after all portability goes up from non-existing to pretty good I think?

Also (i) those two PRs are rather on the big side, especially the changes in rsh_coeffs.* and (ii) now that this issue is closed (but not solved in a released), it is a bit unclear what is going on portability-wise, i.e. I was just about to submit a new issue that gau2grid is not working on any architecture other than x86-64.

dgasmith commented 6 years ago

The changes should have allowed architectures other than x86-64. Do you have errors on ARM or similar that we can look into.

@loriab Any objections to a release?

loriab commented 6 years ago

No objections -- I was just about to vote for a release.

mbanck commented 6 years ago

Well I only tried building 1.2.0 so far cause nothing indicated there might be problems. I've now uploaded git master to Debian so we'll see - still I think a release would be worthwhile if there are no other blockers.

There are testsuite failures on ppc64el, but I'll file those in a separate issue.

dgasmith commented 6 years ago

Release minted. Please do open a new issue for ppc64el and other architecture issues. Non x86 is new to me, but happy to try to broaden this project's targets.

mbanck commented 6 years ago

Current master turned the autobuilders mostly green, in particular the ppc64el tests passed as well: https://buildd.debian.org/status/package.php?p=gau2grid - the remaining issues are in greyed-out non-release architectures, so not a big deal.