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

fix mmseqs, add foldseek #1143

Closed
wants to merge 1 commit into from
Closed

fix mmseqs, add foldseek #1143

wants to merge 1 commit into from

Conversation

joelb123
Copy link
Contributor

CPU flags aren't respected by the mmseqs cmake, and attempts to set them manually broke all optimizations, so I backed that out.

Version numbers reported by the binaries are now consistent with upstream.

I have added foldseek, a related code, which got released yesterday. I think it will be of broad interest.

I added live builds for mmseqs and foldseek. The repos are moving fast between releases for both.

@Nowa-Ammerlaan
Copy link
Member

The -DHAVE_AVX2 etc options are ignored? Do we have an upstream issue about this?

@Nowa-Ammerlaan Nowa-Ammerlaan self-assigned this Feb 10, 2022
@joelb123
Copy link
Contributor Author

Well, the flag to not do native detection is not ignored, as setting it breaks all the others. CMake reports detection of several other hardware conditions without their own flags (SSE, SSE3, ...) as well. But so far as I can tell, there is no usage of these flags in code, only in the compiler flags. This means that one could bypass the CMake dysfunction through appropriate setting of CFLAGS, if one cared to. The build does reflect the CFLAGS.

Upstream seems to have been busy with foldseek and other AlphaFold-related stuff, probably not on their critical path any more than it is on mine.

@Nowa-Ammerlaan
Copy link
Member

Sorry I'm still a bit confused about what is broken. We set -DNATIVE_ARCH=OFF which causes -DHAVE_AVX2 etc to be ignored? but -DNATIVE_ARCH=OFF itself does behave as intended and causes a generic build?

@joelb123
Copy link
Contributor Author

I just said something incorrect; there are indeed AVX and SSE2 ifdefs in the code, so you can't set the CFLAGS.

What you said seems to be correct as per my (novice) understanding of the CMake output. If -DNATIVE_ARCH=OFF, I don't see any messages about recognition of SSE2 or AVX. Maybe that's okay and the configuration still works, but one would have to do some checks to find out.

Perhaps the best solution for now would be to put a native use flag that is default?

@Nowa-Ammerlaan
Copy link
Member

What you said seems to be correct as per my (novice) understanding of the CMake output. If -DNATIVE_ARCH=OFF, I don't see any messages about recognition of SSE2 or AVX. Maybe that's okay and the configuration still works, but one would have to do some checks to find out.

I'll see if I can investigate this and figure out which configuration produces which build. Might take a while until I can take a proper look at it though because I'm rather busy with real-life work stuff at the moment

Perhaps the best solution for now would be to put a native use flag that is default?

Yes, if the -DHAVE_AVX2 etc options don't work as expected then we can compromise and use -DNATIVE_ARCH=OFF to toggle between generic and native build.

@joelb123
Copy link
Contributor Author

After a bit more digging, it seems that the CPUFLAGS stuff you put in does work properly, and should go back in. But I'll have to test it to be sure.

A generic-only build is pretty worthless as the boost from SSE2/AVX is big.

@Nowa-Ammerlaan
Copy link
Member

After a bit more digging, it seems that the CPUFLAGS stuff you put in does work properly, and should go back in. But I'll have to test it to be sure.

You were right, the flag for avx2 works, but the sse2/sse4 flags aren't properly respected. I'll see if I can find a solution.

Nowa-Ammerlaan added a commit to Nowa-Ammerlaan/MMseqs2 that referenced this pull request Feb 13, 2022
@Nowa-Ammerlaan
Copy link
Member

After a bit more digging, it seems that the CPUFLAGS stuff you put in does work properly, and should go back in. But I'll have to test it to be sure.

You were right, the flag for avx2 works, but the sse2/sse4 flags aren't properly respected. I'll see if I can find a solution.

Fix was easy luckily! CMakeLists.txt should use if instead of elsif. Upstream PR is open here: soedinglab/MMseqs2#533

I've pushed a fixed revision of this ebuild, could you rebase and adjust your changes on top of that please?

gentoo-bot pushed a commit that referenced this pull request Feb 13, 2022
Upstream PR is here: soedinglab/MMseqs2#533
See also: #1143

Package-Manager: Portage-3.0.30, Repoman-3.0.3
Signed-off-by: Andrew Ammerlaan <[email protected]>
Nowa-Ammerlaan added a commit to Nowa-Ammerlaan/MMseqs2 that referenced this pull request Feb 15, 2022
Nowa-Ammerlaan added a commit to Nowa-Ammerlaan/MMseqs2 that referenced this pull request Feb 15, 2022
Nowa-Ammerlaan added a commit to Nowa-Ammerlaan/MMseqs2 that referenced this pull request Feb 15, 2022
@Nowa-Ammerlaan
Copy link
Member

Since the issue with the CPU flags is fixed I think we can close this. Please re-open this if it is still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants