Add reward and observation arguments to env.reset()

[ChrisCummins]

🚀 Feature

The CompilerEnv constructor accepts a pair of arguments reward_space and observation_space. We should add those to env.reset(), same as specifying the benchmark.

Motivation

Because this feels like a clumsy API:

env.reward_space = "Foobar"
env.reset(benchmark="benchmark://foo-v0/abc")

Pitch

Allow:

env.reset(benchmark="benchmark://foo-v0/abc", reward_space="Foobar")

[uduse]

I prefer env.reset() doesn't take anything at all. If the user wants to change benchmark, just do an extra line of assignment.

Quote Zen of Python:

There should be one-- and preferably only one --obvious way to do it.

It's already unclear to me if these two are strictly equivalent:

env.benchmark = "benchmark://foo-v0/abc"
env.reset()

vs

env.reset(benchmark="benchmark://foo-v0/abc")

Make env.reset() takes nothing but handle potential benchmark changes would yield a simpler and cleaner API. 🤔

[ChrisCummins]

Tbh I think that the solution may be to remove the env.benchmark setter entirely and roll everything into reset(). The reason is that using the setter without immediately calling reset() can cause unexpected behavior as nothing actually gets changed until reset() is called:

>>> env.benchmark = "benchmark://foo-v0/abc"
>>> print(env.benchmark)
"benchmark://bar-v0/def"      # wtf!
>>> env.reset()
>>> print(env.benchmark)
"benchmark://foo-v0/abc"

Another problem with the setters is that they make it harder to debug subtle typos in your code:

>>> env.benchmrak = "benchmark://foo-v0/abc"
>>> env.reset()
>>> print(env.benchmark)
"benchmark://bar-v0/def"      # wtf!

whereas a typo in an argument name will raise an error.

I suppose this could be mitigated by having the env.benchmark setter implicitly call reset(), but this might lead to a different kind of surprising behavior.

Same goes for env.observation_space and env.reward_space.

What do you think?

Cheers,
Chris

[uduse]

Tbh I think that the solution may be to remove the env.benchmark setter entirely and roll everything into reset(). The reason is that using the setter without immediately calling reset() can cause unexpected behavior as nothing actually gets changed until reset() is called:

>>> env.benchmark = "benchmark://foo-v0/abc"
>>> print(env.benchmark)
"benchmark://bar-v0/def"      # wtf!
>>> env.reset()
>>> print(env.benchmark)
"benchmark://foo-v0/abc"

Another problem with the setters is that they make it harder to debug subtle typos in your code:

>>> env.benchmrak = "benchmark://foo-v0/abc"
>>> env.reset()
>>> print(env.benchmark)
"benchmark://bar-v0/def"      # wtf!

whereas a typo in an argument name will raise an error.

I suppose this could be mitigated by having the env.benchmark setter implicitly call reset(), but this might lead to a different kind of surprising behavior.

Same goes for env.observation_space and env.reward_space.

What do you think?

Cheers, Chris

Yes, I actually agree with removing setters for env.benchmark, env.reward_space, env.action_space, and env.observation_space entirely. This way there are only three ways left to mutate the state (as in memory state of an object) of the environment: env.reset, env.apply, and env.step (I assume env.raw_step is not user API). Seems cleaner to me.

Additionally we want to better differentiate "don't change something when reset" vs "remove something when reset'". e.g. env.reset(..., reward_space=None, ...) could be ambiguous––does the user wants to remove the reward space, or reset but not change the reward space? I would say this means the user wants to remove the reward space. This means the default keyword parameter value should be something other than None. Could be an empty tuple or some special string, I'm not sure.

[ChrisCummins]

Yes, I actually agree with removing setters for env.benchmark, env.reward_space, env.action_space, and env.observation_space entirely. This way there are only three ways left to mutate the state (as in memory state of an object) of the environment: env.reset, env.apply, and env.step (I assume env.raw_step is not user API). Seems cleaner to me.

👍

Additionally we want to better differentiate "don't change something when reset" vs "remove something when reset'". e.g. env.reset(..., reward_space=None, ...) could be ambiguous––does the user wants to remove the reward space, or reset but not change the reward space? I would say this means the user wants to remove the reward space. This means the default keyword parameter value should be something other than None. Could be an empty tuple or some special string, I'm not sure.

That's a very good point! I think it may be worth defining a custom type specifically for this purpose to denote "use whatever value is already set". That would make the default values easier to understand than None, and could allow users to be explicit about it, something like:

env.reset(
    benchmark=compiler_gym.ValueNotChanged,
    reward_space="some_new_space",
    observation_space=None,  # no observations
)

Cheers,
Chris

[uduse]

env.reset(
    benchmark=compiler_gym.ValueNotChanged,
    reward_space="some_new_space",
    observation_space=None,  # no observations
)

This seems good to me. Alternatively, we could use python's ellipsis ... as this special placeholder, or just the string "same". Personally I prefer just "same" because it's simple, explicit, easy to understand, and immutable which makes it a good choice as the default value.

[SoumyajitKarmakar]

Hi @ChrisCummins,
I would like to take a shot at fixing this problem.
I have a question and a trivial solution which might be very silly, but if this

env.reward_space = "Foobar"
env.reset(benchmark="benchmark://foo-v0/abc")

already works, then why not just add a new argument in the function and put the first line in the function body ? Something like,

def reset(..., reward_space: str = "same",...):
  if reward_space != "same":
    self.reward_space = reward_space

and then copy the rest of the function below it, and let it run its normal course of actions.

[ChrisCummins]

Hi @SoumyajitKarmakar,

I would like to take a shot at fixing this problem.

Great, thanks!

why not just add a new argument in the function and put the first line in the function body ? Something like,

def reset(..., reward_space: str = "same",...):
  if reward_space != "same":
    self.reward_space = reward_space

and then copy the rest of the function below it, and let it run its normal course of actions.

Yes, your suggestion should work. A few other things to consider though:

1. The reset() method of all wrappers / subclasses need to be extended to support the new arguments.
2. The new args need to be documented + tested
3. The "setter" properties should be marked as deprecated, though can be done in a follow-up PR.

Cheers,
Chris

[ChrisCummins]

I'm also not sure I like the default being the string "same", as it doesn't feel particularly self-documenting, and prevents the (unlikely) use of the name "same" with observation/reward spaces.

I would suggest adding a new enum to compiler_gym/util/gym_type_hints.py

from enum import Enum

class OptionalArgumentValue(Enum):
    UNCHANGED = 1

The OptionalArgumentValue.UNCHANGED could be used as the default value and checked for using:

if reward_space != OptionalArgumentValue.UNCHANGED:
    ...

Cheers,
Chris