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

Basic setup for Python interface using SWIG #216

Merged
merged 18 commits into from
Aug 17, 2021

Conversation

WagnerMarcos
Copy link
Contributor

@WagnerMarcos WagnerMarcos commented Jul 27, 2021

Signed-off-by: Marcos Wagner [email protected]

🎉 New feature

Summary

Basic setup for a Python interface using SWIG to address #210 . (Related to #101) This creates a python module with the cpp classes with an associated .i file. To achieve dual interface output with Python and Ruby, the ruby.i and the python.i used at compilation had to be moved into separate subdirectories, to avoid collisions between the auxiliary files created by SWIG for Python and Ruby.

Test it

To test this interface

  • Build ign-math with colcon
  • Run export PYTHONPATH=/ws/install/lib/python
  • Open a Python interactive session running python3.
  • Inside of the interactive session run import ignition.math.
  • Library inside python ready to use.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Jul 27, 2021
@francocipollone
Copy link
Contributor

Some of the changes I've pushed:

  • Install path was modified to install math.py in install folder which simplifies PYTHONPATH setting up. 3e31a3a
  • Module was renamed from pymath to math. 3e31a3a
  • Examples were added for Angle, Vector2, and Vector3 py classes. f0ad4df
  • Fixes ruby_TEST after moving ruby.i to a subfolder fab308e
  • The configuration using Swig 3(bionic) and Swig 4(focal) is slightly different. Added support for both, however if we want to target focal it would be a bit simpler. 5fa9264

@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #216 (e6373fb) into ign-math6 (9519ce7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-math6     #216   +/-   ##
==========================================
  Coverage      99.21%   99.21%           
==========================================
  Files             65       65           
  Lines           6089     6089           
==========================================
  Hits            6041     6041           
  Misses            48       48           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9519ce7...e6373fb. Read the comment docs.

@WagnerMarcos WagnerMarcos force-pushed the wagnermarcos/python_interface branch from bca2353 to 4d09fe6 Compare July 29, 2021 14:55
LolaSegura and others added 2 commits July 29, 2021 16:56
…on test files to some of the classes that had an associated .i file.

Signed-off-by: LolaSegura <[email protected]>
@WagnerMarcos
Copy link
Contributor Author

WagnerMarcos commented Jul 30, 2021

The interface has been implemented for Python using SWIG for the files that already had Ruby implementation. At the moment we have pushed tests for these files, to match the Ruby implementation.

Something to note is that when running add_test() for Python in the CMakeList.txt the --gtest_output=xml argument was not added to match the Ruby add_test(), because it was not being able to run it. Ruby is using the test-result folder inside the build directory as output for the xml produced, but no files are created when the colcon test cmd is run. Even after these two cases, running colcon test-result outputs the correct amount of passed, errors, failures and skipped tests (including the Python and Ruby ones)

Copy link
Contributor

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

Looks good, I added some comments.

examples/vector2_example.py Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/GaussMarkovProcess_TEST.py Outdated Show resolved Hide resolved
src/GaussMarkovProcess_TEST.py Outdated Show resolved Hide resolved
src/Rand_TEST.py Outdated Show resolved Hide resolved
src/Vector2_TEST.py Outdated Show resolved Hide resolved
src/Vector3_TEST.py Outdated Show resolved Hide resolved
src/Vector2_TEST.py Outdated Show resolved Hide resolved
src/Vector2_TEST.py Outdated Show resolved Hide resolved
src/python_TEST.py Show resolved Hide resolved
@francocipollone francocipollone marked this pull request as ready for review July 30, 2021 20:59
Signed-off-by: LolaSegura <[email protected]>
Copy link
Contributor

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

LGTM, I left some minor comments.

src/Vector4_TEST.py Outdated Show resolved Hide resolved
src/Vector4_TEST.py Outdated Show resolved Hide resolved
src/Vector2_TEST.py Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
@francocipollone
Copy link
Contributor

I've just left some minor comments however It is in a good shape to be reviewed @chapulina.
One comment, there is no a defined style for python in the ignition contribution, so let us know if we must align to a different style.

Signed-off-by: LolaSegura <[email protected]>
@scpeters
Copy link
Member

I think the following will fix the ABI checker: gazebo-tooling/release-tools#497

src/CMakeLists.txt Outdated Show resolved Hide resolved

set(CMAKE_SWIG_OUTDIR "${CMAKE_BINARY_DIR}/lib/python")
if(CMAKE_VERSION VERSION_GREATER 3.8.0)
SWIG_ADD_LIBRARY(${SWIG_PY_LIB} LANGUAGE python SOURCES python/python.i ${sources})
Copy link
Contributor

Choose a reason for hiding this comment

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

We are recompiling sources instead of just linking, for both python and ruby bindings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Solved at 0ce31c6

Copy link
Contributor

Choose a reason for hiding this comment

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

It fails now, it's like now it can't find libignition-math6.so but I couldn't replicate this locally, locally it works. Will check

Copy link
Member

Choose a reason for hiding this comment

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

I can reproduce the problem on my machine. I'm looking into it

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

the test failure in the GitHub actions occur because make test is run without make install. We created the FAKE_INSTALL target in the test folder to support testing without make install. I've updated the python tests to use FAKE_INSTALL_PREFIX in 3e9109a. It required moving the definition of the FAKE_INSTALL_PREFIX variable to the root CMakeLists.txt in order to be accessed by both the src and test folders, which seems acceptable to me

import ignition.math


class TestVector3(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

nit: rename TestVector3 since this is a more general test

Copy link
Contributor

Choose a reason for hiding this comment

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

Done. e6373fb

@scpeters
Copy link
Member

looks like the only remaining CI issue is that 9e2dafc needs to be signed @francocipollone

francocipollone and others added 6 commits August 17, 2021 13:19
Co-authored-by: Louise Poubel <[email protected]>

Signed-off-by: Franco Cipollone <[email protected]>
Signed-off-by: Franco Cipollone <[email protected]>
Move the variable definition to the root CMakeLists.txt

Signed-off-by: Steve Peters <[email protected]>
Point to fake install lib folder.

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Franco Cipollone <[email protected]>
@francocipollone
Copy link
Contributor

francocipollone commented Aug 17, 2021

the test failure in the GitHub actions occur because make test is run without make install. We created the FAKE_INSTALL target in the test folder to support testing without make install. I've updated the python tests to use FAKE_INSTALL_PREFIX in 3e9109a. It required moving the definition of the FAKE_INSTALL_PREFIX variable to the root CMakeLists.txt in order to be accessed by both the src and test folders, which seems acceptable to me

I see, thanks @scpeters for both the patch and the explanation.


looks like the only remaining CI issue is that 9e2dafc needs to be signed @francocipollone

Fixed!

a1 = ignition.math.Angle(1.5707)
a2 = ignition.math.Angle(0.7854)
print("a1 = {} radians, {} degrees\n".format(a1.Radian(), a1.Degree()))
print("a2 = {} radians, {} degrees\n".format(a2.Radian(), a2.Degree()))
Copy link
Member

Choose a reason for hiding this comment

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

nit for the future (not this pull request): I find f"" strings are more readable than using .format()

for example:

print(f"a2 = {a2.Radian()} radians, {a2.Degree(} degrees\n")

@scpeters scpeters merged commit 2330061 into ign-math6 Aug 17, 2021
@scpeters scpeters deleted the wagnermarcos/python_interface branch August 17, 2021 19:07
@chapulina
Copy link
Contributor

chapulina commented Aug 17, 2021

I just pulled the latest ign-math6 branch and got into a weird compile error:

set_target_properties Can not find target to add properties to: _pymath

Full log

[0.020s] Invoking command in '/home/chapulina/dev_bionic/ws_fortress/build/ignition-math6': /usr/bin/cmake /home/chapulina/dev_bionic/ws_fortress/src/ign-math -DBUILD_TESTING=true -DCMAKE_INSTALL_PREFIX=/home/chapulina/dev_bionic/ws_fortress/install
[0.062s] -- The C compiler identification is GNU 8.4.0
[0.102s] -- The CXX compiler identification is GNU 8.4.0
[0.111s] -- Detecting C compiler ABI info
[0.150s] -- Detecting C compiler ABI info - done
[0.159s] -- Check for working C compiler: /usr/bin/cc - skipped
[0.159s] -- Detecting C compile features
[0.159s] -- Detecting C compile features - done
[0.161s] -- Detecting CXX compiler ABI info
[0.200s] -- Detecting CXX compiler ABI info - done
[0.209s] -- Check for working CXX compiler: /usr/bin/c++ - skipped
[0.209s] -- Detecting CXX compile features
[0.210s] -- Detecting CXX compile features - done
[0.215s] -- ignition-math6 version 6.8.0
[0.215s] -- Operating system is Linux
[0.218s] -- Found CPack generators: DEB
[0.223s] -- Looking for eigen3 - found
[0.223s] 
[0.232s] -- Searching for swig - found.
[0.696s] -- Searching for Ruby - found.
[0.704s] -- Searching for Python - found.
[1.374s] -- 
[1.375s] -- Searching for host SSE information
[1.413s] -- SSE2 found
[1.413s] -- SSE3 found
[1.413s] -- SSE4.1 found
[1.413s] -- SSE4.2 found
[1.422s] -- Configuring library: ignition-math6
[1.423s] -- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY
[1.472s] -- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY - Success
[1.472s] -- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY
[1.520s] -- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY - Success
[1.520s] -- Performing Test COMPILER_HAS_DEPRECATED_ATTR
[1.570s] -- Performing Test COMPILER_HAS_DEPRECATED_ATTR - Success
[1.572s] -- Adding 43 UNIT tests
[1.593s] -- Adding 6 UNIT tests
[1.614s] �[31mCMake Error at src/CMakeLists.txt:120 (set_target_properties):
[1.614s]   set_target_properties Can not find target to add properties to: _pymath
[1.614s] 
[1.614s] �[0m
[1.614s] �[31mCMake Error at src/CMakeLists.txt:126 (target_compile_options):
[1.614s]   Cannot specify compile options for target "_pymath" which is not built by
[1.614s]   this project.
[1.615s] 
[1.615s] �[0m
[1.615s] �[31mCMake Error at src/CMakeLists.txt:131 (install):
[1.615s]   install TARGETS given target "_pymath" which does not exist.
[1.615s] 
[1.615s] �[0m
[1.632s] -- Adding 1 INTEGRATION tests
[1.648s] -- No tests have been specified for PERFORMANCE
[1.663s] -- No tests have been specified for REGRESSION
[1.681s] -- Configuring library: ignition-math6-eigen3
[1.682s] -- Adding 1 UNIT tests
[1.720s] -- Adding codecheck target
[1.720s] -- Build configuration successful
[1.720s] -- Build type: RelWithDebInfo
[1.721s] -- Install prefix: /home/chapulina/dev_bionic/ws_fortress/install
[1.722s] -- Looking for ronn to generate manpages - found
[1.759s] -- Found Doxygen: /usr/bin/doxygen (found version "1.8.13") found components: doxygen missing components: dot
[1.801s] -- Configuring incomplete, errors occurred!
[1.801s] See also "/home/chapulina/dev_bionic/ws_fortress/build/ignition-math6/CMakeFiles/CMakeOutput.log".
[1.802s] See also "/home/chapulina/dev_bionic/ws_fortress/build/ignition-math6/CMakeFiles/CMakeError.log".
[1.807s] Invoked command in '/home/chapulina/dev_bionic/ws_fortress/build/ignition-math6' returned '1': /usr/bin/cmake /home/chapulina/dev_bionic/ws_fortress/src/ign-math -DBUILD_TESTING=true -DCMAKE_INSTALL_PREFIX=/home/chapulina/dev_bionic/ws_fortress/install

I'm not sure yet how to fix my workspace. Looking into it

Update

Solved for now with sudo apt purge .*swig.*

@scpeters
Copy link
Member

scpeters commented Aug 17, 2021

I just pulled the latest ign-math6 branch and got into a weird compile error:

set_target_properties Can not find target to add properties to: _pymath

Full log
I'm not sure yet how to fix my workspace. Looking into it

Update

Solved for now with sudo apt purge .*swig.*

what OS and swig version do you have?

we should consider reverting this if others can reproduce the problem

@chapulina
Copy link
Contributor

what OS and swig version do you have?

This happened on Bionic, swig/bionic 3.0.12-1 amd64. I see this is the same version installed in the successful CI build. I can try to reproduce in a clean container later.

@francocipollone
Copy link
Contributor

I couldn't reproduce the error in bionic with the same swig version. I'm running the container with ign-rocker.

Solved for now with sudo apt purge .swig.

So removing swig and installing it again did the trick? That's interesting.

@chapulina
Copy link
Contributor

So removing swig and installing it again did the trick?

Not exactly 😅 Removing SWIG just allowed me to skip that entire block of code, so I didn't run into the problem and could compile the core library. Reinstalling SWIG brings back the issue to me.


My problem is the CMake version, trying a few things here: #223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants