Skip to content
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

[Algorithm] QMixer loss and multiagent models #1378

Merged
merged 28 commits into from
Jul 14, 2023

Conversation

matteobettini
Copy link
Contributor

@matteobettini matteobettini commented Jul 11, 2023

No description provided.

Signed-off-by: Matteo Bettini <matbet@meta.com>
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 11, 2023
Signed-off-by: Matteo Bettini <matbet@meta.com>
@matteobettini
Copy link
Contributor Author

@Acciorocketships it would be nice to add a QGNN mixer in modules/models/multiagent.py.
implementing the mixer interface

@Acciorocketships
Copy link

What about the QGNN model? If that's not already implemented, I would prioritise that. It has a much larger impact on performance.

@matteobettini
Copy link
Contributor Author

matteobettini commented Jul 11, 2023

Yeah that is just a normal gnn right? I am thinking to make a MultiagentGNN as i made the MLP.
So that it can be used also in MADDPG and so on.

I can take care of that but the mixer is up to you

@vmoens vmoens added enhancement New feature or request new algo New algorithm request or PR labels Jul 11, 2023
@vmoens vmoens changed the title [MARL] QMixer loss and multiagent models [Algorithm] QMixer loss and multiagent models Jul 11, 2023
Signed-off-by: Matteo Bettini <matbet@meta.com>
@matteobettini
Copy link
Contributor Author

Ready for review @vmoens

torchrl/objectives/__init__.py Outdated Show resolved Hide resolved
torchrl/objectives/multiagent/qmixer.py Outdated Show resolved Hide resolved
torchrl/objectives/multiagent/qmixer.py Outdated Show resolved Hide resolved
torchrl/objectives/multiagent/qmixer.py Outdated Show resolved Hide resolved
torchrl/objectives/multiagent/qmixer.py Outdated Show resolved Hide resolved
torchrl/modules/models/multiagent.py Outdated Show resolved Hide resolved
torchrl/modules/models/multiagent.py Show resolved Hide resolved
torchrl/modules/models/multiagent.py Outdated Show resolved Hide resolved
torchrl/modules/models/multiagent.py Show resolved Hide resolved
torchrl/modules/models/multiagent.py Show resolved Hide resolved
@matteobettini
Copy link
Contributor Author

Thanks a lot! Will fix.

Most of the comments on the new loss are on parts that have been copy-pasted from DQN.
Should i modify that too when I fix those?

@vmoens
Copy link
Contributor

vmoens commented Jul 11, 2023

Thanks a lot! Will fix.

Most of the comments on the new loss are on parts that have been copy-pasted from DQN. Should i modify that too when I fix those?

You mean the loss_function default? Yeah sure makes sense

@matteobettini
Copy link
Contributor Author

matteobettini commented Jul 11, 2023

Not only the default, basically all of the comments.
I am just saying that most of the code of qmixerloss comes from dqnloss.

So your review comments apply to dqnloss too.
I'll modify that too when i fix them

@vmoens
Copy link
Contributor

vmoens commented Jul 11, 2023

Oh i see what you mean
Better to fix them but not mandatory. I guess my point was that it's better to fix things before merging than after, but that does not mean that you need to take care of everything in this PR.
Those changes should not be too hard to implement though. Careful with bc-breaking changes, eg the device of DQN should be discouraged but not taken away all at once

Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
@in_keys.setter
def in_keys(self, values):
self._in_keys = values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change you requested makes it bc-breaking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want me to still allow it but warn?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no if tests pass without needing it we're good without it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thin they pass

@vmoens
Copy link
Contributor

vmoens commented Jul 13, 2023

Tests are failing on GPU tests:

test/test_cost.py::TestQMixer::test_qmix_batcher[categorical-device0-True-2] FAILED [  4%]
___________ TestQMixer.test_qmix_batcher[categorical-device0-True-2] ___________
Traceback (most recent call last):
  File "/work/env/lib/python3.9/site-packages/tensordict/nn/common.py", line 1178, in forward
    raise err
  File "/work/env/lib/python3.9/site-packages/tensordict/nn/common.py", line 1164, in forward
    tensors = self._call_module(tensors, **kwargs)
  File "/work/env/lib/python3.9/site-packages/tensordict/nn/common.py", line 1121, in _call_module
    out = self.module(*tensors, **kwargs)
  File "/work/env/lib/python3.9/site-packages/torch/nn/modules/module.py", line 1522, in _wrapped_call_impl
    return self._call_impl(*args, **kwargs)
  File "/work/env/lib/python3.9/site-packages/torch/nn/modules/module.py", line 1531, in _call_impl
    return forward_call(*args, **kwargs)
  File "/work/env/lib/python3.9/site-packages/tensordict/nn/functional_modules.py", line 567, in new_fun
    return getattr(type(self), fun_name)(self, *args, **kwargs)
  File "/work/env/lib/python3.9/site-packages/torch/nn/modules/linear.py", line 114, in forward
    return F.linear(input, self.weight, self.bias)
RuntimeError: Expected all tensors to be on the same device, but found at least two devices, cuda:0 and cpu! (when checking argument for argument mat2 in method wrapper_CUDA_mm)

@matteobettini
Copy link
Contributor Author

oh i am seeing now that some are failing

Signed-off-by: Matteo Bettini <matbet@meta.com>
@vmoens
Copy link
Contributor

vmoens commented Jul 13, 2023

There is still this to be fixed

test/test_cost.py::TestQMixer::test_qmix_batcher[categorical-device0-False-3] - assert not True

Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
@matteobettini
Copy link
Contributor Author

There is still this to be fixed

Thanks a lot and sorry for not noticing that.

For the future, can you explain to me where do you see such failures?
I was opening the gpu tests pipelines but I did not see that

Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Copy link
Contributor

@vmoens vmoens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Woohoo!

@vmoens vmoens merged commit 574dbf1 into pytorch:main Jul 14, 2023
@matteobettini matteobettini deleted the marl_losses_modules branch July 14, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. enhancement New feature or request new algo New algorithm request or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants