cellml / libcellml

Repository for libCellML development.
https://libcellml.org
Apache License 2.0
17 stars 21 forks source link

Adding initial Python bindings #161

Closed MichaelClerx closed 6 years ago

MichaelClerx commented 7 years ago

From Andre/Hugh:

The idea is to have a "bindings/python" folder which would contain the SWIG config/interface files for generating python bindings for the libCellML API. This would also need to be added to the CMake build system so that the bindings can be built. Hugh has found some issues previous with other projects where CMake won't "notice" changes in the API header files and trigger the bindings to be remade when the API changes, but there are ways to work around that on the CI system and still a chance the CMake community will respond to his issues there.

Swig interfaces (and Python tests):

Useful links:

MichaelClerx commented 7 years ago

I came across this yesterday: https://www.softwariness.com/articles/api-design-for-swig/

Seems like we're on the right track

MichaelClerx commented 7 years ago

Ok, workaround for SWIG 3.0.2 seems to work, but now there's a segfault in there somewhere! Possibly still because of the lack of enum class support? Ran tests plenty of time locally and never had a segfault...

@hsorby Any luck on the windows front?

MichaelClerx commented 7 years ago

Need to move this forward!

I can try finding an Ubuntu 14.04 machine and seeing if I get the same segfault as our test machine. If so I can start debugging. (Since it's a segfault, I'm guessing its a C++ issue or a C++/Python issue that, for whatever reason, reliably surfaces on Ubuntu 14.04. Could also just be a swig bug though, in which case we'll just need to update to Ubuntu 16).

But not sure I could find a windows machine, install VB and debug it!

Thoughts? @hsorby @agarny @nickerso @jonc125 ?

If people want to review this PR they can start before we fix the final bugs :-)

hsorby commented 7 years ago

I'll try and get around to looking at this later today.

agarny commented 7 years ago

My take on this: I would simply upgrade to Ubuntu 16.04 LTS.

MichaelClerx commented 7 years ago

Just tested on another machine to double check: It compiles and passes all tests on Ubuntu 16.04 LTS!

MichaelClerx commented 7 years ago

And tested on a virtual machine with Ubuntu 14.04 LTS --> Get the same SEGFAULT! An update definitely seems in order!

MichaelClerx commented 7 years ago

Thanks to @hsorby 's work the segfaults are fixed (I also discovered you could create them on Fedora, but those are fixed too!).

Now it's only the windows linking issue that's stopping this from passing all tests!

MichaelClerx commented 7 years ago

@agarny @nickerso Anyone good at fixing windows issues?

hsorby commented 7 years ago

I can have a look at this I have a windows machine somewhere.

agarny commented 7 years ago

A couple of links (which I hope are correct) to help those who want/need to know more about the issues at hand (it would be nice if such links were always provided: makes it easier to check things, if needed/wanted):

Thanks to @hsorby 's work the segfaults are fixed (I also discovered you could create them on Fedora, but those are fixed too!).

Does this refer to commit 4b9a7b5?

@agarny @nickerso Anyone good at fixing windows issues?

I imagine you are talking about http://autotest.bioeng.auckland.ac.nz/libcellml-buildbot/builders/Unit%20Tests%20Builder%20Windows%208%2064%20Bit/builds/546/steps/compile/logs/stdio?

Regarding the Windows build, I tried to build libCellML on my Windows machine, but among the required libraries, I was missing libxml2. From http://xmlsoft.org/, I thought I could get some from https://www.zlatkovic.com/libxml.en.html, but unless I am mistaken they are for MinGW. So, I ended up using the version provided by the libSBML guys (a copy of which can be found here), but to no avail since when building the develop branch of libCellML, I am getting loads of unresolved external symbol errors at link time.

Still, a quick search for __imp__invalid_parameter_noinfo_noreturn gave me this , which might be worth investigating. I mean, I get the feeling that it might be somewhat related to @hsorby's work in commit 4b9a7b5?

Otherwise, with regards to my libxml2 issue, I checked http://libcellml.readthedocs.io/en/latest/dev_building.html and that made me think that I wish we had a section on prerequisites to build libCellML. That section would mention where we can get those prerequisites. This would potentially save people time...

hsorby commented 7 years ago

I have used the libxml2 library available from:

https://github.com/OpenCMISS-Dependencies/libxml2/

using the develop branch. Thinking about it now I must have some changes pending from my Windows machine to make integration with libCellML easier. I spoke with @nickerso about this and thought we should probably provide binaries so that people may use them easily.

You are correct we should have some notes on pre-requisites like this time for another issue!

MichaelClerx commented 7 years ago

@agarny yes https://github.com/cellml/libcellml/pull/205/commits/2b8b02041f184d8a1a1f2359ee053cdecae6ceed but also https://github.com/cellml/libcellml/pull/205/commits/c3066d03b0458b1850b1c422a53ce1af23bb5e13 which solves the same issue but for the other enums.

And yes it was the windows build for this ticket's PR I was talking about. I completely agree a requirements file is a good idea!

MichaelClerx commented 7 years ago

bump

hsorby commented 7 years ago

I have had a look at the Windows non-linking issue and when I tested on the machine it linked fine. I might look at moving the build slave to use Visual Studio 2015 it is currently using Visual Studio 2013. This will hopefully resolve this issue.

MichaelClerx commented 7 years ago

Brilliant, thanks!

hsorby commented 7 years ago

I think the reason for the non-linking issue is due to a mismatch between building debug libCellML libraries and release Python libraries. This on Windows has previously been a problem but was addressed with the release of the universal C runtime libraries. My thinking is that this was done post-Visual Studio 2013 (I have absolutely no evidence of this) and thus VS 2103 is using the old C runtime libraries which had to be matched. Changing to VS 2015 which links through the UCRT is probably the reason why upgrading fixes this linking issue.

MichaelClerx commented 7 years ago

Sounds plausible! Microsoft has done a lot of Python work recently. Is there a way to manually restart the buildbot tests. or should we do a trivial commit?

hsorby commented 7 years ago

You can re-trigger a message by re-sending a webhook, but don't do it just yet I'm still testing. I'll let you know when I'm done.

hsorby commented 7 years ago

I have updated the build slave to use VS 2015 so if someone with the power would like to resend the appropriate payload we can see if I have fixed the linking issue or created a new one.

MichaelClerx commented 7 years ago

Great! It compiles without warnings or errors now! Thanks!

It doesn't pass any of the tests though, because Test command: NOT_AVAILABLE. Any idea what that is? Python executable not on the path or something?

MichaelClerx commented 7 years ago

Some related issues I'm finding online suggest it could be because we need to specify a release or debug mode? "You should run "ctest -C Debug -V -R itkBMKTestIOTest5" if you compiled in Debug mode and "ctest -C Release -V -R itkBMKTestIOTest5" if you compiled in Release mode." ?

MichaelClerx commented 7 years ago

Someone else had an issue with CMAKE_CONFIGURATION_TYPES; https://github.com/osmcode/libosmium/issues/77

MichaelClerx commented 7 years ago

Here's another one pointing towards build types: https://issues.slicer.org/view.php?id=2347

MichaelClerx commented 7 years ago

I don't know enough about cmake for this! @hsorby ? @agarny ?

agarny commented 7 years ago

I have never used CTest, so not quite sure what to suggest here. Will have a quick look though...

hsorby commented 7 years ago

I know what this is I need to make more changes to the Buildbot configuration files. I'll let you know when this is done so that it can be tested.

hsorby commented 7 years ago

No I was wrong the fix for this should come from the CMake files in libCellML. I will try and address this through #217.

hsorby commented 7 years ago

I think we should remove the __init.i file as this is specific to Python and have a __init.py file that can be configured over to the correct build location instead.

MichaelClerx commented 7 years ago

It's already specific to Python --> It's placed in the bindings/python dir and only used when generating python bindings!

MichaelClerx commented 6 years ago

Any news on this or #217 ?

MichaelClerx commented 6 years ago

Any updates on this or #217 ?

nickerso commented 6 years ago

ping @hsorby

MichaelClerx commented 6 years ago

Hi all! I think it's best to wait till the libxml updates are merged with master, and then rebase this branch (so that it benefits from the new code).

Happy to do that if noone objects.