Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Fix TransformerQA distributed dead-lock bug - #193

Merged
epwalsh merged 7 commits into
mainfrom
rc-dead-lock
Jan 20, 2021
Merged

Fix TransformerQA distributed dead-lock bug#193
epwalsh merged 7 commits into
mainfrom
rc-dead-lock

Conversation

@epwalsh

@epwalsh epwalsh commented Jan 15, 2021

Copy link
Copy Markdown
Contributor

Comment on lines +256 to +262
if dist.get_backend() == "nccl":
device = torch.cuda.current_device()
else:
device = torch.device("cpu")
_min_node_batch_size = torch.tensor(min_node_batch_size, dtype=torch.int, device=device)
dist.all_reduce(_min_node_batch_size, op=dist.ReduceOp.MIN)
min_node_batch_size = _min_node_batch_size.item()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is becoming a really common pattern. It would be to have a helper function for this called dist_reduce or something. Then we could just do:

Suggested change
if dist.get_backend() == "nccl":
device = torch.cuda.current_device()
else:
device = torch.device("cpu")
_min_node_batch_size = torch.tensor(min_node_batch_size, dtype=torch.int, device=device)
dist.all_reduce(_min_node_batch_size, op=dist.ReduceOp.MIN)
min_node_batch_size = _min_node_batch_size.item()
min_node_batch_size = dist_reduce(min_node_batch_size, dist.ReduceOp.MIN, dtype=torch.int)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds like a good change.

@epwalsh

epwalsh commented Jan 19, 2021

Copy link
Copy Markdown
Contributor Author

I don't think there's an easy way to write a test for this.

@dirkgr dirkgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is no way to make this work correctly with uneven batch sizes, so that it doesn't throw anything away?

@epwalsh

epwalsh commented Jan 20, 2021

Copy link
Copy Markdown
Contributor Author

There might be a way. I'll investigate some more tomorrow.

@epwalsh

epwalsh commented Jan 20, 2021

Copy link
Copy Markdown
Contributor Author

@dirkgr updated. Now can handle different batch sizes.

@epwalsh
epwalsh requested a review from dirkgr January 20, 2021 16:19
Comment on lines +35 to +36
cast(List[str], best_span_string)
cast(List[List[str]], answer_strings)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I never knew you could do that.

@dirkgr dirkgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is great! If it comes up with the other metrics, we should use this as a template for how to fix the others.

)
def __call__(
self,
best_span_string: Union[str, List[str]],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be called best_span_strings now?

Comment thread CHANGELOG.md Outdated

### Fixed

- Fixed the potentially for a dead-lock when training the `TransformerQA` model on multiple GPUs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"potential"

@epwalsh
epwalsh merged commit 9472904 into main Jan 20, 2021
@epwalsh
epwalsh deleted the rc-dead-lock branch January 20, 2021 20:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Model] The RC model training just hung there

2 participants