mujoco icon indicating copy to clipboard operation
mujoco copied to clipboard

Fix running tests out of a installed mujoco python package

Open traversaro opened this issue 1 year ago • 6 comments

This should fix running pytest --pyargs mujoco in an environment in which mujoco is installed not in editable mode.

Inspired from:

  • https://github.com/google-deepmind/mujoco/blob/680fb3e5ddbf21fb1e08877f2d03589ad01968ea/python/mujoco/usd/exporter_test.py#L62
  • https://github.com/google-deepmind/mujoco/blob/680fb3e5ddbf21fb1e08877f2d03589ad01968ea/python/mujoco/msh2obj_test.py#L57

traversaro avatar Oct 21 '24 09:10 traversaro

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.

traversaro avatar Oct 21 '24 09:10 traversaro

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?

yuvaltassa avatar Oct 21 '24 21:10 yuvaltassa

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:

  • Duplicate the model.xml to the python testdata folder (easier, but you end up with duplicated files)
  • Copy the model.xml in the python testdata folder in the make_sdist.sh script

Which solution do you prefer?

traversaro avatar Oct 22 '24 04:10 traversaro

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

traversaro avatar Oct 22 '24 07:10 traversaro

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?

yuvaltassa avatar Oct 22 '24 08:10 yuvaltassa

Yeah fork the model into the test data dir.

saran-t avatar Oct 22 '24 09:10 saran-t

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.

traversaro avatar Oct 25 '24 07:10 traversaro

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)/

yuvaltassa avatar Oct 25 '24 08:10 yuvaltassa

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

traversaro avatar Oct 25 '24 08:10 traversaro

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 avatar Oct 25 '24 08:10 traversaro

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

traversaro avatar Oct 25 '24 11:10 traversaro