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

Remove CPP and use only C in get neighbors optmization #1916

Merged
merged 2 commits into from Jul 30, 2020

Conversation

chc273
Copy link
Contributor

@chc273 chc273 commented Jul 30, 2020

Summary

Recently, we have seen some platform-dependent errors caused by this cython extension.

The known error is "illegal instructions (core dumped)" which cause crash of python. This error was found on conda installations. In particular, I have encountered issues on ubuntu 18.04.4 LTS (Matt raised this issue to me), and the github action VM macOS-latest, python3.7. Such errors however do not show up if pymatgen is installed using pip or from source. Interestingly, it also does not show up on github action VM macOS-latest, python3.8.

This PR is an attempt to fix the issue. My suspicion is that this is related to C++ compilation since it requires the platform-dependent flags. In this PR, I have removed all CPP-dependent codes and replace them with C only codes.

@mkhorton
Copy link
Member

Many thanks for addressing this @chc273.

For documentary purposes, I'm just noting we also noticed this issue on conda installs via Binder (which runs Ubuntu), and previously we also had some issues with Linux wheels via PyPI on #1868 that I think might be related.

@chc273
Copy link
Contributor Author

chc273 commented Jul 30, 2020

Many thanks for addressing this @chc273.

For documentary purposes, I'm just noting we also noticed this issue on conda installs via Binder (which runs Ubuntu), and previously we also had some issues with Linux wheels via PyPI on #1868 that I think might be related.

Ah, I missed that issue. Thanks for bringing that to my attention... I hope this fix will do the job, otherwise, it could take much more time, since it is not very obvious where the problem is and the problem seems to be very dependent on the installation methods..

@mkhorton
Copy link
Member

I'm pretty confident that a switch to pure-C will fix this.

In that other issue, we were trying a new method of building PyPI wheels, so I'd assumed there was an issue with the build process rather than an issue in the code itself. However, since we're seeing it on conda too, it's clear there must just be a compatibility issue across Linux distributions with the current code.

@mkhorton mkhorton merged commit 7040ea2 into materialsproject:master Jul 30, 2020
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.

None yet

2 participants