-
Notifications
You must be signed in to change notification settings - Fork 26
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
Enable building a functional Sire on ppc64le architecture #315
Conversation
Thanks @ptosco - this looks fantastic and much appreciated. On a quick look I am very much in favour of merging (particularly as you’ve fixed a long standing bug that caused many unit test failures - feel a little embarrassed about that!). I’m on holiday until next Tuesday so can’t review this until then. I’m happy for @lohedges to merge this tomorrow or Monday if he’s happy with it, as it looks great. |
Hi @ptosco. Many thanks for this, it looks great. I'll try to test local builds on Linux and macOS today and will accept when I'm done. Obviously, I won't be able to test on ppc64le mysef, but I'm happy to trust that this is working. I'll also check for any knock-on effects to BioSimSpace. Thanks again, your efforts are always greatly appreciated. |
All good on Linux on macOS over here so I'm happy to merge. Next to deal with any issues with the pipelines build. (It's not been run in a while so I'm hoping that there are no new conda-related issues.) |
Huzzah, the build worked without issue 👍 Given the intrascale looping speedups, the stable sorting of potential terms and this pull, it's probably time to think about 2020.1.0 releases for Sire and BioSimSpace. @ptosco: are there any other pull requests that you have in the pipeline? If so, I'll hold off until you submit them. Otherwise I can probably get a new release sorted next week. Cheers. |
@lohedges Hi Lester, no, not really, I think that's all for now. Thank you for the super-quick merge! |
Just a quick update. I still plan on making 2020.1.0 releases for Sire and BioSimSpace. @ptosco, I was also planning on updating MiniConda to Python 3.8 and upgrading the Conda dependencies to their latest versions. They way I normally choose suitable versions is to go to the conda-forge page for each dependency, then choose the most recent version that is available for all operating systems that we support. If there is a known bug in a particular version, then I'll roll back the version for all operating systems. Is there anything that I should be aware of for the Windows build, e.g. certain versions of particular dependencies not working. Also, do I only need to care about the version for |
@lohedges I am not aware of particular Windows issues with Python 3.8 - as long as the dependencies are available on conda-forge (sometimes the Windows packages lag behind a bit). And, |
Hmmm, it turns out that there's no easy way to do this. OpenMM hasn't been built for Python 3.8 (apart from in conda-forge testing) so we're stuck with 3.7 for now. Also, when updating dependencies for Sire I need to be careful that they're compatible with all dependencies of BioSimSpace, particularly rdkit. Wish there was an easier way. I'll push an update once I've got a working environment. If you could then test on Windows that would be great. If all is okay, I'll go ahead and create a new release. |
This PR implements a few tweaks to get Sire to build and pass all unit tests on a
ppc64le
architecture.More details:
build/build_sire.py
: replacedos.system
withsubprocess.run
in This avoids that a new shell is spawned to run the subprocess and allows keeping the environment of the calling shell. I had already committed similar changes to unit tests with the same purpose.compile_sire.sh
: addedppc64le
supportcorelib/CMakeLists.txt
: addedppc64le
support; added support forDebug
builds; avoid attempting to compilecpuid
onppc64le
platforms where it is not supportedcorelib/build/cmake/FindVector.cmake
: removed unusedSSE4
flagcorelib/src/apps/testmulti/speedtest.cpp
: avoid that the build fails if_mm_alloc
and_mm_free
are not defined because no intrinsics headers were includedcorelib/src/libs/SireBase/cpuid.cpp
: added support for getting the number of CPUs with native platform-specific methods in the absence oflibcpuid
corelib/src/libs/SireMaths/multi*.h
: all of these files used the!
instead of the~
operator for bitwisenot
, which is wrong and leads to 20/50 unit tests failing inSireMM
. This has gone unnoticed so far as only happens when a non-SSE/non-AVX build is done, which never happens these days onx86
platforms. I also replaced the nested loop with a simpler and more efficient construct. Now all tests pass.wrapper/CMakeLists.txt
: added a comment