Add python bindings #35

danpf · 2019-10-15T00:16:21Z

this may not ever be merged, but here I recreate the basics of mmtf-py using this library only.

it is currently significantly faster than the mmtf-py implementation.

time to load a single mmtf file 1000x
cpp bare 0.29s
this library 0.44s
python og 4.34s

the only 'question' that is really left to solve is how to handle the templating of the mapdecoder. But I think that will not be too difficult. Once i get some time to look at it.

not to get ahead of myself -- but TODO:

tests for round tripping (implement pytest CI as well)
add mapDecoder bindings
add python comparator classes + tests
add to pip
tests for bindings

gtauriello

I like where this is going. Thanks for the efforts. The TODO-list seems reasonable to me.

I had some concerns originally of whether the binding should be an external tool in a separate repository which simply uses mmtf-cpp. But actually, the way you are implementing it is very nicely separated anyways and it makes sense the way it is included here. So as long as everything stays in the separate "bindings" folder and is only included if requested in cmake, it's all good.

.gitmodules

gtauriello · 2019-10-15T13:51:47Z

CMakeLists.txt

+	# set(PYTHON_INCLUDE_DIR_A python_include_A)
+    # set(PYTHON_INCLUDE_DIR_B python_include_B)
+
+	target_include_directories(mmtf_py PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include)


Picky observation: there's a mix of tabs and spaces here. It would be preferred to stick to one style at least within the same file.

danpf · 2019-10-15T23:24:38Z

@gtauriello thanks for your comments! I think it will be a relatively minor addition to the library when it's all said and done, hopefully totaling in at under 1000 lines (of course there's a lot of python init boilerplate that baloons that), but it's all quite simple.

danpf · 2020-03-26T05:19:27Z

@gtauriello and @speleo3 could you review this when you get a chance? I think this is ready for a 1-4 week trial run in master, and then once we are happy with it I can publish it to pip.

what are your thoughts?

speleo3

Would be good to split this into several pull requests

submodule reorganization
const API changes
fix for Segmentation fault with -march=native #36
actual python bindings feature

Can you add a README for the Python usage? It's not obvious what the suggested usage pattern is, the tests are the only hints.

The wrapping looks tedious and very explicit, I assume there is no way to do it automated? I hope this complexity is no burden for the long-term maintainability of mmtf-cpp.

speleo3 · 2020-03-26T08:21:15Z

.gitmodules

+[submodule "submodules/msgpack-c"]
+	path = submodules/msgpack-c
+	url = https://github.com/msgpack/msgpack-c.git
+[submodule "submodules/Catch2"]


Catch2 listed twice?

I will fix this once the other two PRs are merged

speleo3 · 2020-03-26T08:22:30Z

.gitmodules

+[submodule "submodules/--force"]
+	path = submodules/--force
+	url = https://github.com/msgpack/msgpack-c.git


submodules/--force? Looks like an accident

speleo3 · 2020-03-26T08:25:41Z

.travis.yml

      addons: *linux64
    - os: linux
      compiler: clang
      addons: *linux64
    - os: linux
      compiler: gcc
-      env: ARCH=x86 CMAKE_EXTRA=-DHAVE_LIBM=/lib32/libm.so.6
+      env: ARCH=x86 CMAKE_EXTRA=-DHAVE_LIBM=/lib32/libm.so.6 TEST_COMMAND=$TRAVIS_BUILD_DIR/ci/build_and_run_tests.sh


TEST_COMMAND redundant with global value?

The TEST_COMMAND environment variable is a way to run two different tests. I could not find a better way to run two different test suites. If you have any examples of repos that do this in a nicer way, I would be happy to learn a more efficient way and refactor this.

in this case TEST_COMMAND is not redundant because the env: block is overwriting the global value.

speleo3 · 2020-03-26T08:29:40Z

.travis.yml

+      compiler: gcc
+      addons: *linux64cpp17py36
+      dist: bionic
+      env: TEST_COMMAND=$TRAVIS_BUILD_DIR/ci/build_and_run_python_tests.sh CC=gcc


maybe add a global TEST_COMMAND_PYTHON and use

env : TEST_COMMAND=$TEST_COMMAND_PYTHON

And is CC needed? Looks redundant with compiler: gcc

I just tested this, and apparently... yes it is required for some reason, even though you would think otherwise..

CMakeLists.txt

include/mmtf/binary_decoder.hpp

speleo3 · 2020-03-26T09:03:35Z

include/mmtf/structure_data.hpp

@@ -163,7 +163,7 @@ struct StructureData {
  std::string                              title;
  std::string                              depositionDate;
  std::string                              releaseDate;
-  std::vector<std::vector<float> >         ncsOperatorList;
+  std::vector<std::vector<float>>          ncsOperatorList;


This is C++11 syntax. As far as I know this project is still C++03 (but you have my vote for dropping that).

I'm guessing you meant to comment this somewhere else?

speleo3 · 2020-03-26T09:15:10Z

setup.py

+        self.sourcedir = os.path.abspath(sourcedir)
+
+
+class CMakeBuild(build_ext):


What are the advantages of using cmake here? Can't setuptools build an extension directly?

I suppose we could, but we already have cmake baked into this repo and it was relatively simple to set it up to use cmake. If you are aware of some pitfalls of using this type of workflow (setup.py -> cmake) I'm open to changing it, but I found this method relatively painleses.

speleo3 · 2020-03-26T09:17:55Z

setup.py

+    long_description=open("README.md").read(),
+    packages=find_packages("python_src", exclude=["tests", "python_src/tests"]),
+    package_dir={"": "python_src"},
+    ext_modules=[CMakeExtension("mmtf_cppy/")],


The name mmtf_cppy is a bit odd. How about dropping the y?

it's possible we could just kill the python implementation with this one too and just use mmtf. but that would make them incompatible with eachother

danpf · 2020-03-26T12:28:20Z

@speleo3 Thank you for taking the time to review! I'll split this out into 3 requests and then I can address your concerns

gtauriello · 2020-03-26T13:05:30Z

Thanks to both of you for your efforts here and sorry if I've been quiet lately. At SWISS-MODEL, we are currently involved in some modeling efforts for SARS-CoV-2 which makes it hard for me to look at this code. That being said, I'll try to contribute what I can...

danpf · 2020-03-26T13:37:26Z

.travis.yml

@@ -1,57 +1,96 @@
+---


I 'linted' this file using yamllint, sorry for the noise

danpf · 2020-03-26T14:30:32Z

Can you add a README for the Python usage? It's not obvious what the suggested usage pattern is, the tests are the only hints.

done

The wrapping looks tedious and very explicit, I assume there is no way to do it automated? I hope this complexity is no burden for the long-term maintainability of mmtf-cpp.

I actually did use https://github.com/RosettaCommons/binder to generate the initial bindings.

but in this case binder is not a great fit because

msgpack complicates things due to its variant typing system
binder bindings were slower because they did not destroy the c++ data, they returned copies/lists.

it was actually very easy to do most of the bindings. char related data was the only difficult thing to use.

danpf · 2020-04-30T18:18:20Z

reminder bump!
(if you're still busy w/ covid stuff don't feel obligated to respond) just making sure this isn't completely forgotten (like i just did for a month)

danpf · 2020-10-21T23:15:37Z

Hi all,
sorry to bump again, but I also forgot about this (and the other PRs) for a few months. Hopefully everyone is healthy and doing well!

gtauriello · 2020-10-22T12:19:51Z

Ciao all. I am so sorry that I left this pending for so long. I now merged the other 2 PRs and did a 3rd one for the segfault issue (as @speleo3 had suggested).

Also: honestly, I simply don't find time for this project any more. Can one of you @danpf or @speleo3 take it over? I think I can pass admin rights to one (or both) of you for this...

@danpf for the code itself:
I think all 5 TODO items at the start of this PR are done, right?
Once the other PR is merged (I need a review there), you will have to merge master into this branch to resolve merge conflicts.
Other than that it seems to work just fine...

Comments on the README:

can you add some text that "pip install -r requirements.txt" is needed before "pip install ." (also can't the requirements be handled by pip somehow?)
can you add a short description on what the example code is expected to do?

gtauriello · 2020-10-22T12:32:47Z

@danpf One more thing for after the PR and master merges. Can you add an item to the CHANGELOG about the python bindings?

danpf added 7 commits September 12, 2019 19:20

almost there

082afe4

remove excess

a8a924e

done with reader

4b33819

temporary stuff

111b734

updated to have encode work

caeb577

setup for pip

eddb760

remove extra

8e7f475

gtauriello reviewed Oct 15, 2019

View reviewed changes

danpf added 21 commits October 20, 2019 02:40

make pybind11 https

dc91554

binary enc changes

1cb3597

merged

c940bd3

cmakelists to spaces

86cbd91

checkpoint

037eb69

fixes

60103bc

removed mmtf_t.py

5dacd2d

build ci

2ec61b4

lowercase bionic

bae25d3

try2

4d54664

debug travis

a65d69a

setup gcc7

9143d1a

add to env for ever matrix

8a0f1a4

attempt 01

6701c19

more updates

daadb33

fixed exe

7bb120f

upagain

1692246

up

b10e6a3

update langauge

7a2c67e

hopefully done

cf92b45

remove user call

ded948f

danpf added 8 commits March 25, 2020 01:38

fix submodule loc

c0158a6

last try

1ab4052

fixes

674aea1

donezo

2394da2

fix cpp tests

7aab1a3

remove pyc

efcb14a

update gitignore

83ea2be

fix pathing

4761465

speleo3 reviewed Mar 26, 2020

View reviewed changes

danpf added 4 commits March 26, 2020 06:15

change travis python addon name

95c81d1

get rid of CC in travis check

e3bf246

attempt substitute python command

9464880

lint .travis

10e2148

danpf commented Mar 26, 2020

View reviewed changes

.travis.yml

@@ -1,57 +1,96 @@

---

Copy link

Collaborator Author

danpf Mar 26, 2020

Choose a reason for hiding this comment

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

I 'linted' this file using yamllint, sorry for the noise

danpf added 4 commits March 26, 2020 06:38

fix extra DNDEBUG setting

433810e

add CC back

22d8b68

update readme with python information

5ac8042

update

a24420c

gtauriello mentioned this pull request Oct 22, 2020

Fix segmentation fault with -march=native #40

Merged

danpf added 3 commits November 14, 2023 14:02

merge with master

6f2aea1

update mmtf_spec

c740b14

update to Catch2 v3.4.0 (for m1 silicon support)

65c8345

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add python bindings #35

Add python bindings #35

danpf commented Oct 15, 2019 •

edited

Loading

gtauriello left a comment

gtauriello Oct 15, 2019

danpf commented Oct 15, 2019

danpf commented Mar 26, 2020

speleo3 left a comment

speleo3 Mar 26, 2020

danpf Mar 26, 2020

speleo3 Mar 26, 2020

speleo3 Mar 26, 2020

danpf Mar 26, 2020

speleo3 Mar 26, 2020

danpf Mar 26, 2020

speleo3 Mar 26, 2020

danpf Mar 26, 2020

speleo3 Mar 26, 2020

danpf Mar 26, 2020

speleo3 Mar 26, 2020

danpf Mar 26, 2020

danpf commented Mar 26, 2020

gtauriello commented Mar 26, 2020

danpf Mar 26, 2020

danpf commented Mar 26, 2020

danpf commented Apr 30, 2020

danpf commented Oct 21, 2020

gtauriello commented Oct 22, 2020

gtauriello commented Oct 22, 2020

		self.sourcedir = os.path.abspath(sourcedir)


		class CMakeBuild(build_ext):

Add python bindings #35

Are you sure you want to change the base?

Add python bindings #35

Conversation

danpf commented Oct 15, 2019 • edited Loading

gtauriello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpf commented Oct 15, 2019

danpf commented Mar 26, 2020

speleo3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpf commented Mar 26, 2020

gtauriello commented Mar 26, 2020

Choose a reason for hiding this comment

danpf commented Mar 26, 2020

danpf commented Apr 30, 2020

danpf commented Oct 21, 2020

gtauriello commented Oct 22, 2020

gtauriello commented Oct 22, 2020

danpf commented Oct 15, 2019 •

edited

Loading