opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
758 stars 307 forks source link

Upgrade Python and Numpy in ci workflow #3794

Closed mrrezaie closed 1 month ago

mrrezaie commented 1 month ago

Fixes issue #3790

Brief summary of changes

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

Will update after review.


This change is Reviewable

mrrezaie commented 1 month ago

Hi all, Failed on all platforms except Linux. The FindNumPy.cmake may be outdated. The error Could NOT find Python3 (missing: Python3_NumPy_INCLUDE_DIRS NumPy) may be fixed according to this example: https://github.com/SINTEF/dlite/blob/master/cmake/FindNumPy.cmake

by setting

set(Python3_NumPy_INCLUDE_DIRS ${__numpy_path})

and then adding it to the: https://github.com/opensim-org/opensim-core/blob/983f69d822649ab9312d2eb202c021dd0a12daf4/cmake/FindNumPy.cmake#L41-L42

Does it make sense?

aymanhab commented 1 month ago

Thanks @mrrezaie I'd suggest we stick with Python 3.10 which is right in the middle of its cycle and is the default for jupyter notebooks and a corresponding numpy version. Generally conda and colab are the preferred ways to use python rather than the ci builds unless you're using the bleeding edge code

mrrezaie commented 1 month ago

Hi @aymanhab, sorry about it. This was Python 3.10 and NumPy 1.25, exactly the same as the google colab (I also tested numpy 1.21 which was the same as conda distribution), but failed again. What do you think about this?

The FindNumPy.cmake may be outdated. The error Could NOT find Python3 (missing: Python3_NumPy_INCLUDE_DIRS NumPy) may be fixed according to this example: https://github.com/SINTEF/dlite/blob/master/cmake/FindNumPy.cmake

by setting

set(Python3_NumPy_INCLUDE_DIRS ${__numpy_path})

and then adding it to the:

https://github.com/opensim-org/opensim-core/blob/983f69d822649ab9312d2eb202c021dd0a12daf4/cmake/FindNumPy.cmake#L41-L42

aymanhab commented 1 month ago

Thanks for taking this on @mrrezaie I'd be curious why this has changed, in the past we installed numpy using pip in the build script and cmake was able to find it, I don't mind changing our FindNumpy.cmake or better removing it altogether if possible and cmake can do the job on its own

aymanhab commented 1 month ago

The build log on windows suggests that python 3.8 was found during configuration rather than 3.10 and that's why numpy was not found as it's installed in the 3.10 environment (as far as I can tell).

mrrezaie commented 1 month ago

The build log on windows suggests that python 3.8 was found during configuration rather than 3.10 and that's why numpy was not found as it's installed in the 3.10 environment (as far as I can tell).

It's probably because of this argument: -DPython3_ROOT_DIR=C:\hostedtoolcache\windows\Python\3.8.10\x64

I just set it to the same Python version to see what happens.

Is there any option from my side to interrupt the previous checks already running?

aymanhab commented 1 month ago

Yes near the top right of the run page there's cancel workflow popup

mrrezaie commented 1 month ago

Yes near the top right of the run page there's cancel workflow popup

This option is likely not available for non-members.

aymanhab commented 1 month ago

I think you're right. The latest action successfully finds the correct python on windows and mac and looks good, can you check if it's still using 3.8 on ubuntu? Thanks again πŸ‘

mrrezaie commented 1 month ago

From the first commit in this PR, Ubuntu had no issue with any Python version, and the last successful one has Python 3.10 for all platforms.

By recent changes in Windows, Python 3.11 would work for all (https://github.com/opensim-org/opensim-core/actions/runs/9288622368). Do you agree with Python 3.11?

nickbianco commented 1 month ago

Per @aymanhab's previous comment, I think we should stick to 3.10.

aymanhab commented 1 month ago

Thanks @mrrezaie and @nickbianco Indeed 3.10 sounds like a reasonable compromise and the build succeeds on all platforms now but uses 3.8 on ubuntu (https://github.com/opensim-org/opensim-core/actions/runs/9291091418/job/25568847080#step:7:726) I couldn't edit the workflow file but you should be able to include a block similar to mac builds, like below for ubuntu, thank you

- name: Install Python packages
  uses: actions/setup-python@v4
  with:
    python-version: '3.10'
mrrezaie commented 1 month ago

Failed on Ubuntu with new python version. Perhaps it requires DPython3_ROOT_DIR argument similar to Windows, to find the new python directory.

aymanhab commented 1 month ago

On ubuntu I'd add the following after the "Install python packages" so that numpy is installed in 3.10, I'd also remove "python3 python3-dev python3-numpy python3-setuptools" from the list of packages to avoid the ambiguity downstream

mrrezaie commented 1 month ago

Thanks @aymanhab, it is working.

On Windows, if I remove this argument -DPython3_ROOT_DIR=C:\hostedtoolcache\windows\Python\3.10.11\x64, can cmake find Python automatically? I'm asking because of the exact version in this path which may change in future.

aymanhab commented 1 month ago

Perfect, thanks @mrrezaie πŸ‘ Cmake looks for python in some set of standard locations, on windows this set likely doesn't include C:\hostedtoolcache\windows.. so we likely need to keep this hint for ci, on client machines however this may not be necessary depending on where python is installed.

aymanhab commented 1 month ago

@mrrezaie This looks good, you may need to merge the latest main branch to your branch to resolve conflicts and then we can merge. Thank you

mrrezaie commented 1 month ago

Thanks @aymanhab for your guidance, and sorry for taking your time on this. I know you could have done it much faster.

aymanhab commented 1 month ago

Great job @mrrezaie and no worries at all. Thanks for the valuable contribution. Will merge once ci finishes.