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

Support Swig 4.0 #2976

Merged
merged 21 commits into from
Apr 12, 2021
Merged

Support Swig 4.0 #2976

merged 21 commits into from
Apr 12, 2021

Conversation

aymanhab
Copy link
Member

@aymanhab aymanhab commented Mar 29, 2021

Fixes issue #2524

Brief summary of changes

-Upgrade makefiles to require SWIG 4.0

  • Include doxygen option to pass docs to python, java
  • Update interface files to 4.0
  • Some ifdef/endif in headers due to issues parsing doxygen comments by SWIG
  • Moved methods that traverse componentLists to Model class due to new stricter scoping rules

Testing I've completed

Standard test suite passes, Java bindings work in GUI and comments available in GUI IDE

Looking for feedback on...

How this looks in Matlab and in Python, whether comments are available/helpful

CHANGELOG.md (choose one)

  • no need to update because...
  • updated.

This change is Reviewable

@aymanhab aymanhab changed the title Support Swig 4.0.2 Support Swig 4.0 Apr 8, 2021
aymanhab added 5 commits April 7, 2021 23:02
workaround caching swig
try get source for swig on linux
swig location on linux
@aymanhab aymanhab requested a review from nickbianco April 8, 2021 18:23
@aymanhab
Copy link
Member Author

aymanhab commented Apr 8, 2021

@carmichaelong you may want to take a look/review if @nickbianco is not available

@nickbianco
Copy link
Member

Relavant issue: #2869

@nickbianco
Copy link
Member

nickbianco commented Apr 8, 2021

From dev meeting today: create Python (and maybe Java) versions of osimC3D and add tests for them.

EDIT: We could manually test osimC3D to merge this PR, and then creating a Python version could be a separate PR.

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aymanhab)


Bindings/common.i, line 389 at r1 (raw file):

        std::map<std::string, std::shared_ptr<OpenSim::AbstractDataTable> >;
//%template(StdMapStringTimeSeriesTableVec3)
//        std::map<std::string, std::shared_ptr<OpenSim::TimeSeriesTable_<SimTK::Vec3> > >;

Is this supposed to be commented out?


OpenSim/Tools/InverseKinematicsTool.h, line 111 at r1 (raw file):

#ifndef SWIG
    /** @cond **/ // hide from Doxygen
#endif

Will this get exposed if Doxygen is installed but not SWIG?

@nickbianco
Copy link
Member

The changes look good, just a few minor comments.

Creating a local build now to test osimC3D.

@aymanhab aymanhab requested a review from nickbianco April 9, 2021 16:51
Copy link
Member Author

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nickbianco)


Bindings/common.i, line 389 at r1 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Is this supposed to be commented out?

Yes, this map is used internally and is exposed only by the C3DAdapter class's write method which is never used. If left behind, the code generated by SWIG fails to compile because an Iterator over the Map has to be generated and that was quite difficult to communicate to SWIG and is rather unnecessary.


OpenSim/Tools/InverseKinematicsTool.h, line 111 at r1 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Will this get exposed if Doxygen is installed but not SWIG?

Yes, while running SWIG the block disappears to it can digest comments, if not running SWIG the ifndef/endif is a no-op so Doxygen and any other old code/processor work as expected.

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

Changes LGTM. I tested osimC3D locally and it seems to work fine. However, I'm having trouble getting a Python environment to work. I was able to get an older version of pre-release 4.2 working (with changes to the install process I commented here: #2968), but the same process hasn't worked for this build (I'm working on Mac).

I don't want to hold this up unnecessarily, but I also want to make sure that installing Python still works with the new swig changes.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aymanhab)

Copy link
Member Author

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

Sure, I understand, just keep me posted.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aymanhab)

@nickbianco
Copy link
Member

I'm getting the familiar ImportError issue:

image

A quick Google search suggests that this could be due to circular imports, but nothing in __init__.py has been changed. Not sure what the issue is. I am using an Anaconda environment, but I was able to get the OpenSim 4.2 release working with an Anaconda environment. When installing with 4.2, the 'opensim' package shows up in the list of packages (when you call conda list) in your conda environment. However, with the Mac build I downloaded for this release, it did not appear.

@aymanhab
Copy link
Member Author

aymanhab commented Apr 9, 2021

@nickbianco Can you do plain python 3.7-9 to isolate Anaconda specific issues? The ci build runs python tests so it must be able to import simbody without an issue I'm guessing

@aymanhab
Copy link
Member Author

aymanhab commented Apr 9, 2021

Note that ci was built against python 3.9.2 (just to test) but we could bring it down to 3.7, this could be the difference between this and 4.2 public release which couldn't go above 3.7

@nickbianco
Copy link
Member

@aymanhab that could be the issue. I thought we we're still building against python 3.7.

@nickbianco
Copy link
Member

Okay, I got it to work with an Anaconda environment with Python 3.9.2. However, it only works if I'm in the setup.py directory. I get the following warning when running python setup.py install:

UserWarning: The version specified ('4.3-2021-04-08-0253fd959') is an invalid version, this may not work as expected with newer versions of setuptools, pip, and PyPI. Please see PEP 440 for more details.

In addition, the opensim package still doesn't appear in the conda package list. I'm not sure if there's a conflict with setuptools and the version of Python used in the CI build. We might consider rolling back to an earlier Python version.

@aymanhab
Copy link
Member Author

@nickbianco I reverted osx and windows builds to use python 3.7. The warning about version number is rather benign (https://stackoverflow.com/questions/27493792/python-and-pep-440-how-serious-is-this-warning-about-pep440) and will not happen in official releases because we don't embed build date and hash in release versions.

@nickbianco
Copy link
Member

Thanks @aymanhab, I'll try out the new build.

I know the warning is benign, but it mentions potentially loss of compatibility with setuptools, which we need for python setup.py install. Just something to keep an eye on.

@nickbianco
Copy link
Member

Update: I've tried installing the Python bindings with same Homebrew version of Python 3.7 as the CI uses, but I'm still running into issues. I'm now encountering this error:

ImportError: dlopen(/Applications/opensim-core-4.3-2021-04-10-63168d063/sdk/Python/opensim/_simbody.so, 2): Symbol not found: _PyCMethod_New
  Referenced from: /Applications/opensim-core-4.3-2021-04-10-63168d063/sdk/Python/opensim/_simbody.so
  Expected in: flat namespace
 in /Applications/opensim-core-4.3-2021-04-10-63168d063/sdk/Python/opensim/_simbody.so

I'm there is some issue with Python version compatibility that I'm missing now. This is probably an issue with my environment and not the updates to SWIG. I'm fine with merging this and can sort out the install issues afterwards.

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

@aymanhab Okay I finally got a local build working. I needed to set the PYTHONPATH in addition to DYLD_LIBRARY_PATH on Mac to get it work. I did this using my own local build of the bindings.

I'm going to try again using the build from CI. If you want, we could test again using the latest version of Python through CI since that was a motivating factor for updating SWIG.

Reviewable status: 13 of 14 files reviewed, all discussions resolved (waiting on @nickbianco)

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

Actually, I forgot I already had a Python 3.9 build downloaded and set up. Just tested again and it works! So we don't have to force Python 3.7. Once you revert that change and the tests pass, feel free to merge.

Reviewable status: 13 of 14 files reviewed, all discussions resolved (waiting on @nickbianco)

@aymanhab aymanhab requested a review from nickbianco April 11, 2021 21:57
Copy link
Member Author

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

Thanks @nickbianco much appreciated. So do you prefer we make builds with 3.7 or 3.9? I prefer we fix a python version either way rather than get the randomly chosen version selected by ci.

Reviewable status: 13 of 14 files reviewed, all discussions resolved (waiting on @nickbianco)

@nickbianco
Copy link
Member

I was going back and forth about this, but I think I agree that we should fix to a specific version (until something in Python changes that necessitates a version upgrade). It could make sense to stay at 3.7 since other Python package might not stay as current (like Antoine's issue with NumPy and Tensorflow).

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aymanhab)

@aymanhab
Copy link
Member Author

Thanks @nickbianco much appreciated 👍

@aymanhab aymanhab merged commit d9cf2a7 into master Apr 12, 2021
@aymanhab aymanhab deleted the swig-40 branch April 12, 2021 16:20
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