google-deepmind / mujoco

Multi-Joint dynamics with Contact. A general purpose physics simulator.
https://mujoco.org
Apache License 2.0
8.25k stars 823 forks source link

Fix running tests out of a installed mujoco python package #2160

Closed traversaro closed 3 weeks ago

traversaro commented 1 month ago

This PR should fix running pytest --pyargs mujoco in an environment in which mujoco is installed not in editable mode, by moving the model.xml file used by spec_test.py to the testdata folder of python bindings (as suggested by @saran-t and @yuvaltassa).

Furthermore, I modified the github actions test to ensure that they do not run in the python/dist folder, to ensure that they can catch this kind of regressions in the future.

I also integrated @hartikainen suggestions on the use of epath.

traversaro commented 1 month ago

Actually this will not work as https://github.com/google-deepmind/mujoco/tree/680fb3e5ddbf21fb1e08877f2d03589ad01968ea/python/mujoco/testdata and https://github.com/google-deepmind/mujoco/tree/680fb3e5ddbf21fb1e08877f2d03589ad01968ea/test/testdata are not the same directory.

yuvaltassa commented 1 month ago

Hold on, is the issue that we're not exporting some file/files in the wheel? If yes then presumably that's easy to fix?

traversaro commented 1 month ago

Hold on, is the issue that we're not exporting some file/files in the wheel? If yes then presumably that's easy to fix?

Yes, the nutshell of the issue is that. The problem is that the Python MjSpec test access the model.xml in a folder that is not copied to the wheel. So the possible solutions I can think of are:

Which solution do you prefer?

traversaro commented 1 month ago

To avoid regression of this kind, I opened https://github.com/google-deepmind/mujoco/pull/2163 (that should fail until this issue is fixed).

yuvaltassa commented 1 month ago

I don't think the Python tests should depend on files external to that directory. I think forking the model file into Python is the correct thing, duplication is fine in this case.

@saran-t WDYT?

saran-t commented 1 month ago

Yeah fork the model into the test data dir.

traversaro commented 4 weeks ago

Yeah fork the model into the test data dir.

Ok, I updated the PR to do that and modified the CI to catch future errors of this kind.

yuvaltassa commented 4 weeks ago

Hi @traversaro, I ran the CI, something is obviously wrong. Let me know if you think you've fixed this and I will rerun (requires manual retriggering by one of us)/

traversaro commented 4 weeks ago

My bad, I introduced @hartikainen suggestions without double checking, let me fix that.

traversaro commented 4 weeks ago

Let me know if you think you've fixed this and I will rerun (requires manual retriggering by one of us)/

Note to self: I did not realized that Actions run also on the fork, so the easiest option is to check there (in this case https://github.com/traversaro/mujoco/actions) instead of waiting for CI to be approved here (testing locally is obviously possible as well, but not straightforward w.r.t. to regular C++ with Python bindings projects due to themake_sdist.sh + pip install workflow instead of having a single command).

traversaro commented 4 weeks ago

Ok, the CI seems to be green now, the PR is actually ready for review, thanks! @saran-t @yuvaltassa