Skip to content

Mliap bug 3204#3388

Merged
akohlmey merged 7 commits into
lammps:developfrom
chemshift:mliap-bug-3204
Aug 10, 2022
Merged

Mliap bug 3204#3388
akohlmey merged 7 commits into
lammps:developfrom
chemshift:mliap-bug-3204

Conversation

@chemshift

@chemshift chemshift commented Aug 8, 2022

Copy link
Copy Markdown
Contributor

Summary

"import lammps.mliap" now succeeds with the python distributions from conda-forge
Tested on Linux and Windows with an Anaconda Environment and python=3.10.5 from conda-forge.

Related Issue(s)

fixes #3204

Author(s)

Dionysios Sema, MIT | dsema@mit.edu

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Backward Compatibility

No other features affected.

Post Submission Checklist

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system

@akohlmey

akohlmey commented Aug 8, 2022

Copy link
Copy Markdown
Member

@lubbersnick can you please check this out?

@akohlmey

akohlmey commented Aug 8, 2022

Copy link
Copy Markdown
Member

@chemshift thanks for your submission. I think part of the solution is that you are using Python 3.10.5. There have been issues with sub-interpreters for multiple 3.10.x versions that also affected several other software packages. The one where I was affected was "magically" corrected after python was updated to 3.10.5.

@chemshift

chemshift commented Aug 9, 2022

Copy link
Copy Markdown
Contributor Author

Hi @akohlmey. I also tested this with python=3.10.0, 3.9.13 and 3.8.10 (which is an older distribution) and all of these versions work on Linux. I think this has more to do with how python is compiled and distributed in the repos.

I asked @rohskopf to test this and it didn't work on his Mac (python v3.10.5), but was good on his Linux machine. Unfortunately, Mac users will have to compile python from source. If the repos provide appropriate builds in the future though, this code should work for all OSes.

The main problem is actually in lines 11-12:

if pylib.endswith(".a"):
        pylib.strip(".a") + ".so"
        pylib = ctypes.CDLL(library)

"pylib" doesn't change the name of "library" so it is still trying to load the static library. If the above is replaced with:

if library.endswith(".a"):
        library = library.strip(".a") + ".so"
        pylib = ctypes.CDLL(library)

it fixes the problem on Linux. The reason I changed the other parts of the code is because the key: 'INSTSONAME' doesn't exist on Windows, and doing it this way loads the mliap package for Windows too.

@akohlmey akohlmey requested a review from athomps August 9, 2022 18:18
@akohlmey akohlmey self-assigned this Aug 9, 2022
@akohlmey akohlmey added this to the Stable Release Spring 2023 milestone Aug 9, 2022

@athomps athomps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I approve

@lubbersnick

Copy link
Copy Markdown
Contributor

I tried to start a review but I am not sure if it is showing up here for others since I didn't see it from a non-logged in computer. My concern is with the blanket Except clause which doesn't handle all errors. Reposting:

I think this should be raising an OSError with a specific error message from whatever exception is caught here, no?

Something like:

except Exception as e:
    raise OSError("Unable to locate python shared library") from e

The current code doesn't do anything with the OSError.

An alternative would be to omit this blanket except clause.

@chemshift what do you think?

@chemshift

chemshift commented Aug 10, 2022

Copy link
Copy Markdown
Contributor Author

@lubbersnick Addressed.
The except clause should be there, since if the library is not found, it will raise an OSError anyway.

@akohlmey akohlmey merged commit b565d10 into lammps:develop Aug 10, 2022
@chemshift chemshift deleted the mliap-bug-3204 branch August 11, 2022 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] python->LAMMPS->python patterns broken on some interpreters.

4 participants