box2d kengz vs py

[pzhokhov]

No description provided.

[christopherhesse]

Are you sure we can't just have a single box2d that works on mac and linux? It looks like box2d-py was made by openai to fix this, could we fix box2d-py to work on mac? https://pypi.org/project/box2d-py/

[pzhokhov]

The gist of the problem is follows - the pybox2d build command (the one that builds libraries) is not actually being executed when installing from pypi (specifically, python setup.py bdist_wheel does not trigger it, but python setup.py build or python setup.py develop does; I think related to pypa/setuptools#1194).
To solve it, pybox2d can be installed from source like this:
pip install -e git+git://github.com/pybox2d/pybox2d.git@3818cf5#egg=Box2D
I'll add respective comment to readme and change the logic in the container, but I am afraid it is about as much as we can do without sinking a ton of time into messing with setuptools logic.

[christopherhesse]

Well, if we're unwilling to update our custom python package for box2d, could we have setup.py dynamically depend on box2d-py or the other box2d based on platform.system(), or does that not work? Adding a note to the README puts the work on the user who will probably just think our code is broken since it doesn't work even after following the install instructions.

[pzhokhov]

Fair point... We can definitely do the latter, and let me spend another half-hour on trying to fix the pybox2d or box2d-py package - maybe there is some simple solution after all.

[christopherhesse]

One difference is that box2d-py is source only, while box2d-kengz has a wheel for mac: https://pypi.org/project/Box2D-kengz/#files

[christopherhesse]

Compiling a wheel for mac with python setup.py bdist_wheel seems to be broken with pybox2d and box2d-py, but box2d-kengz works, available here: https://files.pythonhosted.org/packages/81/20/51d6c0c87f7642efb709c518fb0ca8e5eab068259588552c41da5926ae27/Box2D-kengz-2.3.3.tar.gz

But of course, that one seems to fail on linux, while box2d-py works, so updating box2d-py to contain any sort of fix from box2d-kengz could be a pretty good fix.

[pzhokhov]

yeah, that's what I have been thinking too, comparing them now

[pzhokhov]

ugh it looks like bdist_wheel it is actually broken on both; but python file that is exported from swig interface has a version compatible with osx in box2d-kengz and the version compatible with linux in box2d-py :( hence one works in one case and the other - in the other case

[christopherhesse]

Yeah, that's kinda gross. It looks like gym-retro overrides build_ext, does that let you execute the correct build steps on install? https://github.com/openai/gym-retro/blob/master/setup.py#L80

[pzhokhov]

Yeah, the fix I was thinking of is also along these lines, but rather to override install so that it copies newly generated *.py files to where they need to go (the correct files are generated, but *.py files are copied before building, so the correct files end up never being used)

[pzhokhov]

whew I think I fixed it, now we just need to release the pypi package

[christopherhesse]

Nice!

[christopherhesse]

Are we going to have some fork of pybox2d at like openai/box2d-py?

[pzhokhov]

oh, and now I see how the pybox2d CI works - they use swig3.0 instead of default swig :/ Still looks ugly to me.

[pzhokhov]

openai/box2d-py#1 (for some reason I cannot add @christopherhesse as a reviewer)

[christopherhesse]

Nice, are we dropping py27 support here as well?

[pzhokhov]

Well... It was not actually tested with py27 and I am pretty sure tests won't work with py27 (like some extra magic is needed to make that work). I thought it is being at the end of its life, but, apparently, not: https://legacy.python.org/dev/peps/pep-0373/
If we want to support it, we need to make sure tests run with py27.

[pzhokhov]

yeah, there are failures like

>       model = mujoco_py.load_model_from_path(fullpath)
E       AttributeError: 'module' object has no attribute 'load_model_from_path'

with python2.7 (apparently, related to openai/mujoco-py#261).
I think we should drop support of py27 for now, and create a separate issue for that.

[pzhokhov]

@christopherhesse are you ok with merging this?

[christopherhesse]

LGTM!