openai / coinrun

Code for the paper "Quantifying Transfer in Reinforcement Learning"
https://blog.openai.com/quantifying-generalization-in-reinforcement-learning/
MIT License
395 stars 88 forks source link

Why is calling `init_args_and_threads` multiple times from the same process not recommended? #19

Closed ankeshanand closed 5 years ago

ankeshanand commented 5 years ago

Hi,

I need to initialize the coinrun library multiple times in the file (separately for train / test), and I was able to do so without any visible issues. I did notice though that it's mentioned in the function doc of init_args_and_threads to not do this. https://github.com/openai/coinrun/blob/601de520abec526c101eb87cb445c612a9087407/coinrun/coinrunenv.py#L95-L97 Is there a reason behind this? (Trying to understand why that is the recommendation to avoid any unknown issues)

/cc @christopherhesse @kcobbe

christopherhesse commented 5 years ago

Well it's not threadsafe for one, so if you called in different threads at the same time, I'm not really sure what would happen.

Should you call it multiple times in a single thread? Well if you call it with the same global args each time, maybe nothing bad would happen, especially if you didn't create the environment until after you called it multiple times.

So you said you inited it multiple times for train/test, which means that, if both are in the same process, it's likely either your test environment is the same as your train environment or vice versa since it is using global state (for instance USE_HIGH_DIF). If that's not an issue for you, carry on. If it is an issue, I'd recommend running two different processes. You can even isolate just the environment in a subprocess, like how SubprocVecEnv works, though I don't think we have anything that does that currently.

ankeshanand commented 5 years ago

Thanks, I was re-initing Coinrun when switching b/w train and test to have the right global state, but you're right thread safety might be a problem. I will try isolating them within separate sub-processes, thanks for the tip!

ankeshanand commented 5 years ago

I wrote a quick wrapper to wrap a coinrun env inside a subprocess (pretty similar to SubprocVecEnv). https://gist.github.com/ankeshanand/24d6f45d17cc44a9e0c9d71a636d537e

christopherhesse commented 5 years ago

Nice! Is there a reason it doesn't just use SubprocVecEnv?

ankeshanand commented 5 years ago

This is pretty similar to SubprocVecEnv albeit some minor differences.

From what I got, SubprocVecEnv expects a list of env_fns and converts them into a VecEnv. In this case, we already have a VecEnv (and thus don't need the unsqueeze when doing step) that we want to wrap in a subprocess.

christopherhesse commented 5 years ago

Oh right, unfortunate. There's some functionality in the multiprocessing module to do some of this already, but it seems to not handle properties well at the very least.