-
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] Refactor pytorch custom model support #3634
Conversation
@@ -292,8 +301,8 @@ def to_batches(arr): | |||
@override(PolicyGraph) | |||
def get_initial_state(self): | |||
return [ | |||
self.model.init_hidden().numpy().squeeze() | |||
for _ in range(self.n_agents) | |||
s.expand([self.n_agents, -1]).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.
The main change here is that we return a list of [num_agents, h_size_i] for i in n_state_tensors
, rather than [h_size] for i in n_agents
previously (which was kind of unnatural and didn't generalize well to multiple state tensors).
Relatedly, I'm regretting allowing multiple hidden state tensors, we probably should have just required them to be fused into one element.
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test PASSed. |
Test FAILed. |
Test PASSed. |
Test FAILed. |
|
||
def __init__(self, obs_space, num_outputs, options): | ||
TorchModel.__init__(self, obs_space, num_outputs, options) | ||
hiddens = options.get("fcnet_hiddens") |
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.
should this have a default?
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.
right now, if not provided, this fails at line 26 (which then you may as well make this options["fcnet_hiddens"]
)
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's filled in by agent, so it should be ok either way.
obs: observations and features""" | ||
res = self._convs(obs) | ||
def _hidden_layers(self, obs): | ||
res = self._convs(obs.permute(0, 3, 1, 2)) |
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 documented somewhere?
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.
Done
|
||
return (TupleActions(list(actions.transpose([1, 0]))), | ||
hiddens.transpose([1, 0, 2]), {}) | ||
return TupleActions(list(actions.transpose([1, 0]))), hiddens, {} |
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 the second [1, 0, 2]
not needed anymore?
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.
Yeah (see comment above), hiddens are handled differently now.
What do these changes do?
I expect we'll have to make more changes (and add more tests) as we implement PyTorch support more fully; this is just an initial cleanup to better unify QMIX with pytorch A3C.
#3365