Add BUILD_PYTHON_WHEELS option, use it to control installation of libraries int…

[aymanhab]

…o sdk/Python folder, also modify setup.py
Fixes issue #<issue_number>

Brief summary of changes

Modify build system to support building python wheels for pypi distribution. Involved some minor cmake refactoring, but ended up making it as a "post install" step on mac, linux to minimize changes to flaky rpath monsters. Wheels on windows do build and install but fail to load one casadi dll. Disabling in ci

Testing I've completed

Published wheels and tested, had others test installation locally, beefed up ci to test after installing wheels.

Looking for feedback on...

CHANGELOG.md (choose one)

• updated.

---

This change is 

[aymanhab]

Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @moorepants and @nickbianco)

---

buildWheel.cmake line 5 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Are these message() call debugging statements, or do you want to keep them in the builds?

They will be removed once all wheels build and function properly.

---

CHANGELOG.md line 40 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Add PR number.

Done.

---

CMakeLists.txt line 16 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove this unused cmake_policy?

was useful for building/testing with recent cmake which fails without it, but will remove.

---

CMakeLists.txt line 504 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I think I would prefer BUILD_WHEELS over BUILD_PYPI. What do you think?

Done.

---

CMakeLists.txt line 573 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Trim trailing whitespace.

Done.

---

.github/workflows/continuous_integration.yml line 46 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Trim trailing whitespace.

Done.

---

.github/workflows/continuous_integration.yml line 247 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Trim trailing whitespace.

Done.

---

.github/workflows/continuous_integration.yml line 383 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Trim trailing whitespace.

Done.

---

.github/workflows/continuous_integration.yml line 388 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Do you know if it's possible to upload multiple wheels with one action? I guess we would need to revamp the CI if we wanted to build wheels for multiple python versions anyway.

Because the python version is pinned by swig into the bindings, we likely need to redo the build of core then build the wheel for every version, since wheel names are different per python version, that's possible. Ideally we have a separate action to build a grid of wheels that we invoke as needed not with every commit. Yes a revamp to use a grid of python versions and cibuildwheel would be worth it down the line.

---

.github/workflows/continuous_integration.yml line 599 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Is UNIX_FHS necessary for building the wheels? Users who download artifacts might be confused by the different file structure, although they might just install the wheels from now on!

Wheel has its own file structure, this preexisting flag controls where the files are installed first, they are later copied during wheel building. The flag was left as is but can be retired later.

---

.github/workflows/continuous_integration.yml line 632 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Trim trailing whitespace.

Done.

---

.github/workflows/continuous_integration.yml line 637 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Trim trailing whitespace.

Done.

---

Bindings/Python/pyproject.tom1 line 7 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Set the current OpenSim version (e.g., 4.6.0)? It would be nice to automate from version.py as the comment suggests.

This file ended up unused so will remove. Thanks for the review tho ;)

---

Bindings/Python/setup.py line 67 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove?

Good reminder for the command to build wheels which is not included anywhere except in ci, will add comment accordingly

---

OpenSim/Moco/CMakeLists.txt line 140 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

You could probably remove this comment. Future developers won't care or know that Moco was installed any differently.

Done.

---

OpenSim/Moco/CMakeLists.txt line 169 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I assume it is fine that the target name is removed here?

Was removed because it now is part of OpenSimTargets and not ad-hoc

[nickbianco]

@nickbianco reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aymanhab and @moorepants)

---

buildWheel.cmake line 5 at r2 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

They will be removed once all wheels build and function properly.

Sounds good.

---

CMakeLists.txt line 504 at r2 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Done.

Sorry, one more suggestion for BUILD_PYTHON_WHEELS.

---

.github/workflows/continuous_integration.yml line 388 at r2 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Because the python version is pinned by swig into the bindings, we likely need to redo the build of core then build the wheel for every version, since wheel names are different per python version, that's possible. Ideally we have a separate action to build a grid of wheels that we invoke as needed not with every commit. Yes a revamp to use a grid of python versions and cibuildwheel would be worth it down the line.

All sounds good.

---

.github/workflows/continuous_integration.yml line 599 at r2 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Wheel has its own file structure, this preexisting flag controls where the files are installed first, they are later copied during wheel building. The flag was left as is but can be retired later.

I believe this flag was changed from OFF to ON, hence my original comment. I'm fine if it needs to be changed, I was just wondering why.

---

OpenSim/Moco/CMakeLists.txt line 169 at r2 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Was removed because it now is part of OpenSimTargets and not ad-hoc

Ah, right. Thanks for the clarification.

---

CMakeLists.txt line 15 at r3 (raw file):

if(COMMAND cmake_policy)
    cmake_policy(SET CMP0003 NEW)
    cmake_policy(SET CMP0005 NEW) 

Trim trailing whitespace.

[aymanhab]

Regarding tests @nickbianco the full set of tests is run before packaging in regular install, the tests commented out/removed never worked because the data files/models are in a different folder and not in the wheel. I run python tests after installation from wheels (more robust) to test packaging (that libraries can be found/loaded) from a different folder. This seems to catch any issues loading libraries, especially Moco libraries on windows and that's the sticking point holding this PR so far. I'm a bit handicapped on windows as I don't have a windows machine. Should we merge and fix later so wheels on mac/linux are widely tested or wait for the fix of moco libraries loading on windows?

Reviewable status: 17 of 20 files reviewed, 3 unresolved discussions (waiting on @moorepants and @nickbianco)

---

CMakeLists.txt line 504 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Sorry, one more suggestion for BUILD_PYTHON_WHEELS.

Done.

---

CMakeLists.txt line 15 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Trim trailing whitespace.

Done.

---

.github/workflows/continuous_integration.yml line 82 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

This seems to be removed unintentionally.

Thanks for the catch, I edited this file online 😢

---

.github/workflows/continuous_integration.yml line 183 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Removed unintentionally?

Done.

---

.github/workflows/continuous_integration.yml line 309 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Removed unintentionally?

Done.

---

.github/workflows/continuous_integration.yml line 327 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Trim trailing whitespace.

Done.

---

.github/workflows/continuous_integration.yml line 345 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Add CASADI?

Done.

---

Bindings/Python/pyproject.tom1 line 1 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Shouldn't this file be pyproject.toml not pyproject.tom1?

Removed

[aymanhab]

Ready for review @nickbianco

Reviewable status: 16 of 20 files reviewed, 4 unresolved discussions (waiting on @moorepants and @nickbianco)

---

buildWheel.cmake line 5 at r2 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Sounds good.

Removed

---

.github/workflows/continuous_integration.yml line 129 at r7 (raw file):

#        pip install dist/opensim-4.5.2-cp311-cp311-win_amd64.whl
#        # Run the python tests, verbosely.
#        python3 -m unittest discover --start-directory opensim/tests --verbose

Removed building and publishing wheels on windows

[nickbianco]

@aymanhab the changes to the CI builds LGTM, but I'm still not sure about the Python tests. I get that some of these tests won't work with the wheels, but I don't feel great about deleting/commenting out some of these tests which could be useful to enforce later. Is there a way to preserve the test suite without deleting/commenting them out?

Lastly, it would be good to git rebase and squash all the commits together to have an interpretable git history.

@nickbianco reviewed 2 of 3 files at r4, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @moorepants)

---

-- commits line 275 at r7:
Before merging, we should squash all these commits into one (i.e., via git rebase -i) to not pollute the git history.

[aymanhab]

@nickbianco Let me clarify, the deleted files were never used as the only way for the tests to pass without copying all the data files and models to the package folder is to run them from the source tree as we have done in the past, we do this (run tests from different folder) on main branch and we're doing it now in this PR. We probably should remove the copying of this opensim/tests folder to the package folder to avoid misleading future developers but functionality/testing-wise we lost absolutely nothing as you can see by checking the ci build log on main. With that if you prefer I restore these unused files I'm ok with that too.

[nickbianco]

@aymanhab, if we do decide to remove the tests, I think we should at the very least handle that on a separate PR. I understand that they might not be currently used, but if there is value in reinstating the tests, then I would like to consider it. Is is possible to revert the test changes on this branch without holding up the PR?

[aymanhab]

Test files restored. Thanks for the review @nickbianco 👍

[nickbianco]

All code changes LGTM. Last thing needed is to squash all the commits together.

@nickbianco reviewed 13 of 13 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aymanhab and @moorepants)