Added support for a C API.

[eaplatanios]

I am planning to use Gym Retro from Swift (in combination with Swift for TensorFlow) and so I implemented a C API. It's almost complete (except for a couple functions that I can add soon), but I wanted to get feedback on whether this looks ok to merge.

Thanks,
Anthony

[christopherhesse]

Doesn't Swift for Tensorflow support python calls directly? Is this a performance optimization for that?

[eaplatanios]

You can invoke Python yes, but that has the extra overhead of going through the Python interpreter for every call and the requirement that Python and all relevant dependencies is installed. This PR introduces a C API that can be used directly from Swift (i.e., no Python overhead or requirements) and that can also be potentially used to interface with other languages (e.g., Java/Scala/etc through JNI).

[christopherhesse]

Out of curiosity, what is the overhead for swift + retro?

[eaplatanios]

If you mean by Swift + Python Retro, I haven't implemented the Swift interface yet to test, but I have benchmarked another simple RL environment we built in C++ between Python and Swift (using C API bindings similar to this PR) and the performance increase was more than 5-fold. Python is interpreted vs Swift is compiled, which slows things down a lot, plus the communication overhead: Swift -> Python -> Native is much slower than Swift -> Native, which is what this PR would allow.

Note also that his enables retro to be used from other languages than Swift and Python.

[eaplatanios]

@christopherhesse This is now complete. The C API now supports all the functionality of the python interface. I plan to add a Swift API in a separate repository soon.

[christopherhesse]

How does a line like https://github.com/openai/retro/pull/152/files#diff-ade9c892f04c242a8aa4fefdd15f8116R99 compile in C? And how does an exception like https://github.com/openai/retro/pull/152/files#diff-372cd2f0ca7ccb713fd591baddcc3d4cR34 get handled?

[christopherhesse]

@endrift any objections to the idea of a C API for retro?

[eaplatanios]

@christopherhesse The implementation is still C++, but the API is C-compatible. This means that the calling convention for all functions defined in the new header is C-compatible and can be linked to when compiling C code that uses them.

[eaplatanios]

@christopherhesse Actually, I need to make a few more edits. I'll try to push them asap.

[eaplatanios]

@christopherhesse Actually never mind, I just pushed the fixes.

[eaplatanios]

@christopherhesse I now have a very early (working) draft implementation of a Swift API for Retro that uses this C API. You can find it here.

[eaplatanios]

I also fixed a couple of minor bugs in emulator.cpp and coreinfo.cpp (the latter causing the issue in #153).

[endrift]

This PR has some...interesting ideas about memory management. Remember that you can't call free on something allocated via new. Many of these things could be cached in one of the associated structs to avoid having to have the user delete them too. It also has a lot of style grievances and general other issues. I'm curious why so much has been reimplemented instead of just being a thin wrapper.

[eaplatanios]

Sorry the missing delete functions are because I didn't end up using these functions in the Swift API and forgot to add the deletes. I am actually curious, where are "searches" used? I didn't see them being used anywhere in the Python API.

Also, where do I use free for something allocated with new?

[eaplatanios]

@endrift I believe I addressed all of your comments, but please tell if there is anything else I can address now. I also pulled out the couple of changes you requested in separate PRs.

[christopherhesse]

Thanks for fixing those issues @eaplatanios! I have a couple of suggestions:

1. Seems like the tests should build the C API by default
2. Would be nice to have a ctypes or cffi python test that loads the retro-c library and uses it to load a rom and perform a single step, just to make sure this doesn't get broken by accident, similar to https://github.com/openai/retro/blob/master/tests/test_env.py#L13

[eaplatanios]

@christopherhesse Sorry for not responding here for so long but I got caught up with multiple conferences and with my thesis proposal. I'll get back to this very soon.

[christopherhesse]

No rush, the code isn't changing quickly so the PR isn't going to be invalidated anytime soon.

[eaplatanios]

I just got back to this and updated the other PR. Regarding your points:

1. Where should I make that change? As in, in which file of the repo?
2. I'm not familiar with ctypes or cffi and I don't have to add such a test now and so it may be a bit of time until I add such a test.

[christopherhesse]

1. probably https://github.com/openai/retro/blob/master/travis.py#L81
2. Getting a basic ctypes thing working isn't too hard, and the advantage is that we wouldn't accidentally break the C API as easily later. I'm not sure if it should be a requirement to merge this PR, but I'm leaning towards yes. Here's a simple ctypes example: https://pgi-jcns.fz-juelich.de/portal/pages/using-c-from-python.html

[eaplatanios]

Yes, that totally makes sense. I will look into it as soon as I get a chance. By the way, why do you have s_loadedEmulator be a static variable, thus only allowing one emulator instance per process? Wouldn't it be better to make it local to Emulator instances? Or is there a libretro limitation?

[christopherhesse]

I am told that this is a libretro limitation, gym-retro contains code to raise exceptions if you try to load multiple emulators at the same time.

[eaplatanios]

@christopherhesse fyi, I also implemented a Swift API for ALE and I observe a ~5x speedup over using retro. Note that this is a larger difference than you'd observe with atari-py and python retro as it is not yet using the latest version of ALE which gives a further performance boost.

[christopherhesse]

That is a known issue: #127

@endrift's suggestion from here #172 (comment), which seems to be the best fix, is to use ALE's stella in retro instead of the the normal libretro stella.

[endrift]

I'd strongly recommend using configure_file and a .in file instead of doing this inline.

[endrift]

@crypt096 Why did you leave a review? To the best of my knowledge you've never been involved with this project and my "strong recommendation" was never address?