-
Notifications
You must be signed in to change notification settings - Fork 310
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
New worker for PEARL #1124
New worker for PEARL #1124
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1124 +/- ##
==========================================
+ Coverage 88.00% 88.04% +0.04%
==========================================
Files 188 189 +1
Lines 8885 8926 +41
Branches 1124 1131 +7
==========================================
+ Hits 7819 7859 +40
Misses 862 862
- Partials 204 205 +1
Continue to review full report at Codecov.
|
Assuming everything passes muster, I'm OK merging this now if KR is okay with it. Clearly, unifying sampling for meta-RL is an area we need to work on. |
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.
- Based on comments and code similarities, not sure if we need a new meta-rl replay buffer. Perhaps a refactor using an existing replay buffer can come later if we decide.
- What is the reasoning behind a multi task replay buffer for PEARL? I saw your comment:
The algorithm keeps a buffer for each task, and I think this looks cleaner. I just followed the implementation on oyster, but I suppose I can keep a list of buffers in the algo without this buffer class.
I'm with you on keeping a list of buffers as opposed to a separate MT replay buffer
- Can you create your own rollout function separate from the sampler/utils
rollout
function.
It seems that you have modified the return types of therollout function
and added some other features. A question I find myself asking is "Will the added functionality be used by algorithms besides PEARL?"
src/garage/sampler/pearl_sampler.py
Outdated
|
||
# save the latent context that generated this trajectory | ||
if accum_context: | ||
path['context'] = self.policy.z.detach().cpu().numpy() |
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.
does self.policy.z
ever update during the duration of the sampling loop? If not, can we store this value instead of querying ever time.
71b2d79
to
0570e5b
Compare
src/garage/sampler/__init__.py
Outdated
'WorkerFactory', | ||
'Worker', | ||
'DefaultWorker', | ||
'PEARLSampler', |
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 this be achieved by adding a PEARLWorker class @krzentner ?
Anson does a similar pattern for RL2: He implements an RL2 worker which lives in garage.tf.algos.rl2
rather than the central sampler package.
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 can definitely be achieved with that.
Alternatively, just put z in agent_infos
. It's there for a reason.
src/garage/sampler/pearl_sampler.py
Outdated
@@ -0,0 +1,156 @@ | |||
# pylint: disable=unnecessary-pass |
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.
you can deal with this by just omitting pass
from your empty methods.
src/garage/sampler/__init__.py
Outdated
'WorkerFactory', | ||
'Worker', | ||
'DefaultWorker', | ||
'PEARLWorker', |
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 put this class in src/garage/torch/algos/pearl.py
-- since it is only used by the PEARL algorithm.
Its import path will be from garage.torch.algos.pearl import PEARLWorker
Please address my comments before submit. @krzentner should also probably take a look. |
src/garage/torch/algos/pearl.py
Outdated
pass | ||
self._agent_infos['context'] = [self.agent.z.detach().cpu().numpy() | ||
] * self._max_path_length | ||
self.agent.sample_from_belief() |
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 might make more sense to put this at the start, since agent updates happen in between calls to this method.
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.
Mostly looks good. There's one small thing you should check, but feel free to submit.
082b2c7
to
313ac27
Compare
313ac27
to
c918641
Compare
Adding a new worker for PEARL sampling because it needs to be able to store context and resample belief in the policy when obtaining samples.