-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RLlib] Move bandits into main agents folder; Make RecSim adapter more accessible; #21773
[RLlib] Move bandits into main agents folder; Make RecSim adapter more accessible; #21773
Conversation
…its_become_1st_class_citizens
@@ -81,14 +78,14 @@ def make_model_and_action_dist(policy, obs_space, action_space, config): | |||
|
|||
# TODO: Have a separate model catalogue for bandits | |||
if exploration_config: | |||
if exploration_config["type"] == TS_PATH: | |||
if exploration_config["type"] == "ThompsonSampling": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe make this backwards compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this would work: The exploration type is now a single "ThompsonSampling" string as we moved the ThompsonSampling
class into our built-in RLlib utils/exploration
package.
The two supported exploration types are pretty much hard-coded into these two bandit Trainers (UCB and TS).
Also, the old TS_PATH
( -> ThompsonSampling path) does no longer exist as it was inside the contrib folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this may not be the right level to do backward compat messaging. Can you make sure that users code from previous ray versions will automatically raise an actionable error message to help them migrate to new changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done through the added "location" check. The new test_bandits
test case makes sure this works and users will get a deprecation message when they do stuff like:
tune.run("contrib/LinTS")
# OR
from ray.rllib.contrib.bandits.agents.lin_ts import LinTSTrainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a few comments, and asked clarifying questions, etc. Thanks for doing this, and all the hard work.
@@ -81,14 +78,14 @@ def make_model_and_action_dist(policy, obs_space, action_space, config): | |||
|
|||
# TODO: Have a separate model catalogue for bandits | |||
if exploration_config: | |||
if exploration_config["type"] == TS_PATH: | |||
if exploration_config["type"] == "ThompsonSampling": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
if isinstance(original_space, spaces.Dict): | ||
assert "item" in original_space.spaces, \ | ||
"Cannot find 'item' key in observation space" | ||
model_cls = ParametricLinearModelThompsonSampling | ||
else: | ||
model_cls = DiscreteLinearModelThompsonSampling | ||
elif exploration_config["type"] == UCB_PATH: | ||
elif exploration_config["type"] == "UpperConfidenceBound": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here in terms of keeping backwards compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment above.
@@ -32,7 +32,7 @@ def plot_model_weights(means, covs): | |||
if __name__ == "__main__": | |||
num_iter = 10 | |||
print("Running training for %s time steps" % num_iter) | |||
trainer = LinTSTrainer(env=WheelBanditEnv) | |||
trainer = BanditLinTSTrainer(env=WheelBanditEnv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a toy environment? Asking for clarification, not w.r.t the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is one of our "bandit-friendly" example envs, now in the rllib/examples/env/... folder.
# Force good learning behavior (this is a very simple env). | ||
self.assertTrue(results["episode_reward_mean"] == 10.0) | ||
|
||
def test_deprecated_locations(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this
def tearDownClass(cls) -> None: | ||
ray.shutdown() | ||
|
||
def test_bandit_lin_ts_compilation(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think its worth it if we started writing some unit tests for algorithms that test whether the loss functions, etc have the right outputs? Its extra work, but I found in the past that by writing unit tests like these, they heavily reduced the time that it took to debug later performance regressions. That, and it was a good exercise for building knowledge on the algorithms that we were writing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can definitely start this pattern on my own whenever I implement an algorithm next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
not necessarily this pr though.
like if we add tf impl for bandit, we can write unit tests to make sure those loss funcs are the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea @avnishn . Some of our algos do have these tests, e.g. ppo, dqn, pg, marwil. But not all. Yes, we should ideally have these for all algos. I agree that it helps to reduce bugs at a small cost!
"contrib/LinUCB", | ||
config=UCB_CONFIG, | ||
"BanditLinUCB", | ||
config=config, | ||
stop={"training_iteration": training_iterations}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add a reward based stopping criteria whenever we finish benchmarking these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely need to clean up these scripts. These are the original ones from our contributors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this in a follow up PR?
Just moved these here, these are not new scripts. Let's go through them in another PR and make sure the Bandits learn properly.
Examples: | ||
>>> env_ctx = EnvContext({"a": 1, "b": 2}, worker_index=0) | ||
>>> env_ctx.set_defaults({"a": -42, "c": 3}) | ||
>>> print(env_ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it removes clutter from the environments that we've added in a lot of the example environments at least, if I understand this function correctly.
from ray.rllib.utils.error import UnsupportedSpaceException | ||
|
||
|
||
class TestRecSimWrapper(unittest.TestCase): | ||
def test_observation_space(self): | ||
env = make_recsim_env(config={}) | ||
env = InterestEvolutionRecSimEnv() | ||
obs = env.reset() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also check if an observation that has been sampled from the observation space is contained, although im not sure how good a test that is. We could sampled a few thousand observations and check if they're contained in the obs space (its a relatively cheap operation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are doing this right below. Only twice, though after the env.reset()
and the step()
.
Added some more checks for the action space test.
Not sure I fully understand what you are aiming at, though. But please feel free to suggest more checks here.
def tearDownClass(cls) -> None: | ||
ray.shutdown() | ||
|
||
def test_bandit_lin_ts_compilation(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
not necessarily this pr though.
like if we add tf impl for bandit, we can write unit tests to make sure those loss funcs are the same.
rllib/env/wrappers/recsim.py
Outdated
return multi_action | ||
|
||
|
||
def rllib_gym_wrapper(recsim_gym_env: gym.Env, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recsys_gym_wrapper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no! Great catch! :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
…its_become_1st_class_citizens # Conflicts: # doc/source/rllib/rllib-algorithms.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had some comments about seeding learning tests but otherwise lgtm.
results = None | ||
for i in range(num_iterations): | ||
results = trainer.train() | ||
check_train_results(results) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we seed this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
TODO (follow up PR):
Why are these changes needed?
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.