google-research / planet

Learning Latent Dynamics for Planning from Pixels
https://danijar.com/planet
Apache License 2.0
1.18k stars 202 forks source link

Environment gets wrapped in `wrappers.ExternalProcess` twice #11

Closed piojanu closed 5 years ago

piojanu commented 5 years ago

Hi!

Environment gets wrapped in ExternalProcess in its constructor in tasks.py (the last but one line):

def _dm_control_env(action_repeat, max_length, domain, task):
    from dm_control import suite

    def env_ctor():
        env = control.wrappers.DeepMindWrapper(suite.load(domain, task), (64, 64))
        env = control.wrappers.ActionRepeat(env, action_repeat)
        env = control.wrappers.LimitDuration(env, max_length)
        env = control.wrappers.PixelObservations(env, (64, 64), np.uint8, 'image')
        env = control.wrappers.ConvertTo32Bit(env)
        return env
    env = control.wrappers.ExternalProcess(env_ctor)
    return env

and in simulate.py again if we have multiple env processes defined:

def define_batch_env(env_ctor, num_agents, env_processes):
    with tf.variable_scope('environments'):
        if env_processes:
            envs = [
                wrappers.ExternalProcess(env_ctor)
                for _ in range(num_agents)]
        else:
            envs = [env_ctor() for _ in range(num_agents)]
        env = batch_env.BatchEnv(envs, blocking=not env_processes)
        env = in_graph_batch_env.InGraphBatchEnv(env)
    return env

Fix: At first I wanted to remove wrapping from task constructor in tasks.py as it shouldn't be needed in case when we execute environments in sequence (BatchEnv meet API requirements when it calls environments). However, it introduces some seg. faults. Strange and certainly not good, I have no idea why is that.

So the other way is to simplify code in simulate.py to:

def define_batch_env(env_ctor, num_agents, env_processes):
    with tf.variable_scope('environments'):
        envs = [env_ctor() for _ in range(num_agents)]
        env = batch_env.BatchEnv(envs, blocking=not env_processes)
        env = in_graph_batch_env.InGraphBatchEnv(env)
    return env

Locally it works when I run training with debug config. @danijar should I create PR?

danijar commented 5 years ago

Thanks! Yes, a PR would be welcome.

danijar commented 5 years ago

Hi @piojanu, I've looked into this more and decided to extend the functionality a bit to also support isolating environments into processes. This helps avoid OpenGL errors on some system configurations. I've already incorporated those changes and pushed them to Github, so I'm closing your PR. By the way, the external process wrapper was only used once as it should since it was disabled in simulate.py. Thanks again for bringing this to my attention!