opensim-org / opensim-core

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

Add Geometry folder to the Python package in local installation #3884

Closed mrrezaie closed 3 weeks ago

mrrezaie commented 3 months ago

Hi, when loading a model in Python, there are a bunch of missing geometry warnings until I specify the Geometry path through ModelVisualizer.addDirToGeometrySearchPaths or change the Logger level. This must be done for every script/project which is a bit annoying. When installing the OpenSim package locally, this can be handled once and forever by copying the Geometry folder into the package folder in the Python installation directory.

Brief summary of changes

I just modified the setup.py and __init__.py files.

Testing I've completed

I tested locally and it worked.

Looking for feedback on...

CHANGELOG.md (choose one)

Will do if the idea accepted.


This change is Reviewable

nickbianco commented 3 months ago

@mrrezaie, seems like a reasonable upgrade!

@aymanhab, anything in these changes that would have unexpected effects on the conda packages?

aymanhab commented 3 months ago

@nickbianco I believe this branch of code is never exercised in actual conda environment but rather for local use from a build environment as such I'm ok with the change if it helps local testing but we need to confirm that there's no unintended impact on actual conda packages/users.

mrrezaie commented 3 months ago

Hi @nickbianco and @aymanhab, thanks for your response.

These changes are very likely safe for your conda package; because:

  1. In the setup.py, the bin_files and geometry_files might be empty lists, which means nothing will be copied during installation.
  2. In the __init__.py, the ModelVisualizer.addDirToGeometrySearchPaths will be called only if the Geometry folder exists in the package folder.

However, I have no idea how to test it. Actually, if you could include the Geometry folder in the conda package, this would be beneficial for the conda users as well.

mrrezaie commented 3 months ago

These changes basically only works when installing the Python package shipped with GUI locally. The artifacts in opensim-core also doesn't contain Geometry folder. Any idea how to include it?

nickbianco commented 4 weeks ago

@mrrezaie sorry for the long delay on this. The best way to test would be to create a fresh conda environment locally and run python -m pip install . with the current changes. The bin_files and geometry_files should be installed in the site-packages folder in the conda environment, just as it would in a non-conda environment. In a separate conda environment, I would install OpenSim via one of our existing conda packages and make sure that this environment isn't affected by your manual install.

mrrezaie commented 4 weeks ago

Thanks @nickbianco, I did and there was no interference (different conda environments are basically independent).

I also found a way to include Geometry folder into your API releases (artifacts). These lines in ci can clone opensim-models repo and move the Geometry folder to the opensim-core distribution folder (near the /bin/ directory):

    - name: Checkout opensim-models main
      uses: actions/checkout@v4
      with:
         repository: opensim-org/opensim-models
         path: 'opensim-models'

    - name: Include Geometry folder
      run: move $env:GITHUB_WORKSPACE/opensim-models/Geometry ~/opensim-core-install

If you agree with these changes, I will push a new commit.

nickbianco commented 3 weeks ago

@mrrezaie, do you have a sense of how much time this additional checkout step adds to the CI build? I'm less concerned about having the Geometry folder inside opensim-core artifacts since these are usually used for development and testing purposes, and most developers usually have a Geometry folder already installed somewhere locally that can be pulled in as needed.

aymanhab commented 3 weeks ago

If we're not completely sure regarding including geometry folder in each api distribution, then I would lean not to do it. Space issues as well as the possibility of inconsistencies with public distributions weigh against it. In addition the geometry will likely change soon to accommodate upcoming visualization needs. Sorry for being late to chime in. It's a good idea @mrrezaie but we need to take time to assess.

mrrezaie commented 3 weeks ago

Thanks @nickbianco and @aymanhab. I already have it in my workflow; 12s for cloning opensim-models and 2s for copy, 14s in total.

mrrezaie commented 3 weeks ago

The sparse-checkout would clone the Geometry folder only, and hence, reduce the checkout time to 3s (5s in total). The Geometry folder would be the latest version of the opensim-models repo (any update will be included). If you are not completely sure about this, feel free to close this PR. Thanks.

nickbianco commented 3 weeks ago

@mrrezaie thanks for the profiling numbers. Per Ayman's comment, I would also lean towards not including this addition to the CI. The CI scripts tend to be fragile, and I'm generally opposed to tying in other repos when not absolutely necessary.

Otherwise, I think this LGTM. @aymanhab, any opposition to merging this?

aymanhab commented 3 weeks ago

@nickbianco no opposition to merging, thanks @mrrezaie for the contributions. Before the next official release and conda packages though we may need to revisit/customize but good to see how this works for our internal users in the meantime.

nickbianco commented 3 weeks ago

Thanks for the nice addition @mrrezaie!

mrrezaie commented 3 weeks ago

Thanks for merging.