Implementation of the SQIL algorithm #744
Conversation
Codecov Report
@@ Coverage Diff @@
## master #744 +/- ##
==========================================
+ Coverage 96.33% 96.38% +0.04%
==========================================
Files 93 95 +2
Lines 8789 8901 +112
==========================================
+ Hits 8467 8579 +112
Misses 322 322
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Remove redundant parameter
* Pin SB3 version to 1.7.0 (#738) * Update conftest.py (#742) * Custom environment tutorial (#746) * Custom environment tutorial draft * Update the docs website * Clean notebook * Text clarification and new environment * Decrease training duration to hopefully make CI happy * Clarify that BC itself does not learn rewards --------- Co-authored-by: Ariel Kwiatkowski <[email protected]> * Tutorial on comparing algorithm performance (#747) * Add a new tutorial * Update index.rst * Improvements to the tutorial * Some more caution words * Fix typos --------- Co-authored-by: Ariel Kwiatkowski <[email protected]> --------- Co-authored-by: Adam Gleave <[email protected]>
There was a problem hiding this comment.
Thanks for the implementation! Algorithm looks correct. SQIL is a strange beast but at least it's quite simple conceptually.
I think the implementation could be simplified / code duplication could be reduced by moving most of the logic into a ReplayBuffer wrapper, then setting DQN to use that replay buffer (probably with replay_buffer_class). I think you can still do this in composition (which I agree seems right approach -- SQIL is not an RL algorithm, so probably shouldn't inherit from DQN, and multiple inheritance gets messy). I may be missing some subtlety though, but if right this would let us eliminate many lines of code making it much easier to read. Could also pave the way to having this support any OffPolicyAlgorithm rather than just DQN which would be neat.
Other main area is it'd be nice to have slightly more comprehensive tests. Although to be fair the algorithm is so trivial there's not that much to actually test (beyond correct functioning of DQN). Checking it makes some progress on a simple environment might add something. If returns improving is too flaky (at least without too expensive a # of timesteps), could also check the Q-network is moving in right direction (e.g. assigns higher Q-value to expert demos than randomly chosen observation/action pairs)?
docs/tutorials/10_train_sqil.ipynb
Outdated
| "\n", | ||
| "Soft Q Imitation Learning ([SQIL](https://arxiv.org/abs/1905.11108)) is a simple algorithm that can be used to clone expert behavior.\n", | ||
| "It's fundamentally a modification of the DQN algorithm. At each training step, whenever we sample a batch of data from the replay buffer,\n", | ||
| "we also sample a batch of expert data. Expert demonstrations are assigned a reward of 1, while the agent's own transitions are assigned a reward of 0.\n", |
There was a problem hiding this comment.
This is an accurate description but does highlight the algorithm is a bit bizarre. If the agent perfectly mimicked expert demos, they'd still get assigned a reward of 0. Whereas at least with stuff like AIRL/GAIL, they'd get the same reward (as discriminator could no longer distinguish them).
There was a problem hiding this comment.
It is bizarre, but that's probably unavoidable, at least in continuous observation spaces (i.e. everything that's not tabular) -- if we tried to relabel generated data if it matches the demonstrations, that would basically never happen because we're comparing floats for equality.
This makes me wonder if this method would be particularly vulnerable to adversarial attacks, since there's a big difference between being in a state [1.1234] vs state [1.1235]. A larger network could probably overfit to that, which would be less likely with a more dense reward
ernestum
force-pushed
the
redtachyon/740-sqil
branch
from
18a2f11
to
f8584c3
Compare
|
Asking @jas-ho to look at fixing the failing/flaky tests |
| demonstrations=rollouts, | ||
| policy="MlpPolicy", | ||
| ) | ||
| # Hint: set to 1_000_000 to match the expert performance. |
There was a problem hiding this comment.
100_000 was already sufficient to reach expert performance (tried only a couple of times though)
| dqn_kwargs=dict( | ||
| learning_starts=500, | ||
| learning_rate=0.002, | ||
| batch_size=220, |
There was a problem hiding this comment.
I searched for good hyperparams using optuna. With these hyperparams I ended up with 1 failure out of 64 on my machine whereas before it was as many as 1 out of 5 (and on the CI pipeline it seems to have failed almost every time for the last ten-ish runs).
There was a problem hiding this comment.
I've also tried pushing the number of episodes in evaluate_policy up to 100 but it did not improve further so I reverted it.
There was a problem hiding this comment.
@AdamGleave based on the hyperparam search and the manual testing I did I do not think the flakiness here points to a bug. Therefore, I think our best option is to actually fix the seeds for this specific test. Given how slow the CI pipeline is I think it's not good to have even a 2% remaining rate of flakiness.
There was a problem hiding this comment.
see 2bf467d which passes on CI (except for codecov)
I also ran pytest --flake-finder tests/algorithms/test_sqil.py --flake-runs=16 -n 8 locally and found no failures for test_sqil_performance and test_sqil_demonstration_buffer which were the ones failing in CI previously
There was a problem hiding this comment.
Thanks for doing the hyperparam sweep! Yeah, let's fix the seed. Given it tests for a significant improvement in reward it should be a non-trivial test even with only a single seed (if we were just checking for any improvement then it'd be 50/50 whether it passed even if the algorithm was no better than random). We could also @pytest.mark.parametrize the seed for extra robustness if the test runs quick enough.
There was a problem hiding this comment.
test_sqil_performance already takes ~20 sec on my machine. Given that I did not cherry-pick the seed I don't think additional parametrization improves robustness enough to trade off favorably against decreased dev velocity
- also set default tb_log_name to "SQIL"
- also: adjust imports to conform with style guide
I addressed your comments @AdamGleave . From my side it's ready for final review. |
|
@AdamGleave will you review this or should I have a look? |
There was a problem hiding this comment.
Please decide if you want to make this change or not prior to merging @jas-ho , but I don't need to re-review just for that.
That change was implemented already so it might have just been a gh display issue. -> LGTM :) |
Description
Fixes #740
Right now it's a basic implementation based on SB3.
It seems to work on a basic level (i.e. I trained it on CartPole and it converged), but still needs a bunch of cleanup and testing.
One note is that there's quite a few
#type: ignore[...]annotations. I tried to minimize them, but a good chunk of the code is either modifying SB3 code, or closely interfacing with it, and SB3 seems to have more lax type checking.Testing
WiP