Reimplement the python machinery to get the model location in pure python #143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…thon - Remove the pybind11 dependency - Implement setup.py and setup.cfg to install the icub_models package
|
Thanks! What is the recommended way of installing both the C++ library and the Python one (i.e. what we would do in the superbuild if |
|
Thanks to this: 32c5201 the python package is also installed by cmake. |
|
Thanks! I still think we need to fix this because a-priori you can't know the relative path between |
|
For example, this is what happens when you install the package in a conda environment on Windows: |
|
And the example code fails with: |
|
@GiulioRomualdi can you give me permission to your fork so that I can push the fix? |
I can't push for some reason, the fix is traversaro@011a877 . |
I did not saw the invitation. Now I was able to push the updated version. |
GiulioRomualdi
commented
Mar 28, 2022
python/icub_models/icub_models.py
Outdated
| # from CMake, i.e. that means that the file was installed via setup.py | ||
| # let's substitute relative_path with its default value | ||
| if relative_path.startswith('@'): | ||
| relative_path = '../../../..' |
There was a problem hiding this comment.
Is it working also on Windows? I'm referring to the usage of /
There was a problem hiding this comment.
Yes, see traversaro@011a877#r69749781 .
There was a problem hiding this comment.
Actually this is does not work on windows when installing with pip install . . Not that it is a workflow I am super-worried of, but it may be an indication we are doing something wrong. Not sure if @diegoferigo can lend a quick eye on the setup.py/Python part (I already checked the CMake part and I am quite sure on that part).
There was a problem hiding this comment.
I didn't check in great detail. In order to get better multiplatform support especially for / vs \, I warmly recommend using pathlib. It allows to use the / operator to separate paths, that is translated automatically to a real path depending on the OS.
There was a problem hiding this comment.
In this case, it would translate in something similar:
root = str(Path(__file__).parent.parent.parent.parent / "share" / "iCub" / "robots")Another suggestion I'd give is to return pathlib.Path objects instead of strings.
There was a problem hiding this comment.
In order to get better multiplatform support especially for
/vs\, I warmly recommend usingpathlib.
Just to understand, why is this necessary? / works on both Windows and Linux/macOS.
There was a problem hiding this comment.
B.t.w. as now the CMake/superbuild/conda part is covered by @ICUB_MODELS_CXX_PYTHON_INSTALL_RELATIVE_DIR@ probably setup.py could just install the models directly in dirname(__file__) so relative path could be just .?
There was a problem hiding this comment.
In order to get better multiplatform support especially for
/vs\, I warmly recommend usingpathlib.Just to understand, why is this necessary?
/works on both Windows and Linux/macOS.
AFAIK this code was working on linux but not on windows. Just a wild guess considering the few lines of code. Disclaimer: didn't test the code myself.
There was a problem hiding this comment.
I tested the code on Windows and / works fine on Windows (even with mixed with \), see #143 (comment) .
There was a problem hiding this comment.
The test is failing finding:
C:/hostedtoolcache/windows/Python/3.8.10/share/iCub/robots
It seems that a icub_models is missing, I'd expect:
C:/hostedtoolcache/windows/Python/3.8.10/*icub_models*/share/iCub/robots
Furthermore, I'd expect also a site-packages dir somewhere, but I'm not familiar of the Python dir structure on Windows.
traversaro
reviewed
Mar 29, 2022
setup.cfg
Outdated
|
|
||
| [metadata] | ||
| name = icub_models | ||
| description = Visualizer for robot logger |
|
I wrote a test for the python package and there seems that on windows is failing https://github.com/robotology/icub-models/runs/5733790646?check_suite_focus=true#step:6:19 |
diegoferigo
reviewed
Mar 29, 2022
python/icub_models/icub_models.py
Outdated
|
|
||
|
|
||
| def get_models_path() -> pathlib.Path: | ||
| root = "" |
There was a problem hiding this comment.
Nit: in python scoped variables like those defined in the if-else are available outside. I know this is confusing for C++ users :) The important thing to remember is to have both if and else branches defining the variable. In other words, what I'm saying is that you can remove this line.
Yes, this confirm what I tested in #143 (comment) , a possible fix is #143 (comment) . |
GiulioRomualdi
force-pushed
the
python
branch
2 times, most recently
from
3621429 to
0534c8f
Compare
Click here to check the content of `tree $env:pythonLocation\..` in windows
|
|
I think I got the problem. LinuxWindows |
|
I think that trying to hardcode the relative path will not help in general. Could we just install the models in the same |
|
I updated #143 (comment). |
GiulioRomualdi
force-pushed
the
python
branch
2 times, most recently
from
58496c5 to
6d9c402
Compare
|
Everything is working now 🚀 6d9c402 fixes the problem on windows |
traversaro
added a commit
to robotology/robotology-superbuild
that referenced
this pull request
Mar 29, 2022
After the change of options in robotology/icub-models#143
|
Here I am, I finally had time to check a bit more in detail this PR. I'm not very familiar with The creation of sdist packages could be achieved with pypa/setuptools_scm, that basically bundles the entire repo in an archive. Pip, when installs the package from PyPI, downloads this sdist, decompresses it, and calls The creation of wheel packages should be straightforward too, just make sure that it detects platform independence since here we just have python files. You can play with pypa/build and its |
|
Closing in favor of #144 |
traversaro
added a commit
to robotology/robotology-superbuild
that referenced
this pull request
Mar 30, 2022
After the change of options in robotology/icub-models#143
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
setup.pyandsetup.cfgto install theicub_modelspackage.Thanks to this PR it will be possible to ship the package in pypi
cc @traversaro