Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

[BlenderBot2] add model.module to make BB2 DDP compatible #4259

Merged
merged 4 commits into from
Dec 21, 2021
Merged

Conversation

jxmsML
Copy link
Contributor

@jxmsML jxmsML commented Dec 16, 2021

Patch description
Change model -> model.module in DistributedDataParallel setting in

  • blenderbot agent
  • memory_agent for the Multi-session Chat project

Testing steps

Other information

Copy link
Contributor

@mojtaba-komeili mojtaba-komeili left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -800,7 +800,10 @@ def _set_batch_skip_search(self, valid_exs: List[Message], batch: Batch) -> Batc

def eval_step(self, batch):
output = super().eval_step(batch)
if output is None or not hasattr(self.model, 'retriever'):
model = self.model
if isinstance(self.model, torch.nn.parallel.DistributedDataParallel):
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, this is what self.model_api is meant to accomplish, if you can just switch to that

@jxmsML jxmsML requested a review from klshuster December 20, 2021 23:00
Comment on lines 803 to 806
model = self.model_api
if isinstance(model, torch.nn.parallel.DistributedDataParallel):
model = model.module
if output is None or not hasattr(model, 'retriever'):
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, I might not have been clear enough: model_api already takes care of this check

Suggested change
model = self.model_api
if isinstance(model, torch.nn.parallel.DistributedDataParallel):
model = model.module
if output is None or not hasattr(model, 'retriever'):
if output is None or not hasattr(self.model_api, 'retriever'):

This is the only change required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah didn't know that! thanks for the pointer

Copy link
Contributor

@klshuster klshuster left a comment

Choose a reason for hiding this comment

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

thanks for making the changes!

@jxmsML jxmsML merged commit 0ea3d22 into main Dec 21, 2021
@jxmsML jxmsML deleted the bb2_ddp branch December 21, 2021 21:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants