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.

Sign up for GitHub

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

Jump to bottom

Fix: Word based batching memory with longer source sentences. #430

Merged
merged 3 commits into from
Jun 11, 2018
Merged

Fix: Word based batching memory with longer source sentences. #430

merged 3 commits into from
Jun 11, 2018

Conversation

tdomhan
Copy link
Contributor

@tdomhan tdomhan commented Jun 8, 2018

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@tdomhan
Copy link
Contributor Author

tdomhan commented Jun 8, 2018 •
edited
Loading

This regression was introduced by me in the following PR:
#378

While everything else in define_bucket_batch_sizes is defined on the target side we need to make sure that in scenarios where the source is large than the target the source input data of shape (batch_size, len_source) can be covered by the default bucket batch size. Otherwise MXNet is not able to reuse memory:

 Bucketing: data source has a shape [59,40,1], which is larger than already allocated shape [22,100,1]. Need to re-allocate. Consider putting default bucket key to be the bucket taking the largest input for better memory sharing.

@tdomhan tdomhan added the bug label Jun 8, 2018
davvil
davvil approved these changes Jun 11, 2018
View reviewed changes
CHANGELOG.md Outdated
## [1.18.22]
### Fixed
- Make sure the default bucket is large enough with word based batching when the source is longer than the target (Previously
there as an edge case where the memory usage sub-optimal with word based batching and longer source than target sentences).
Copy link
Contributor

Choose a reason for hiding this comment

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

"there was"

Copy link
Contributor

Choose a reason for hiding this comment

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

"memory usage was sub-optimal"

@tdomhan tdomhan merged commit 83468d2 into master Jun 11, 2018
@tdomhan tdomhan deleted the wordbatching branch June 11, 2018 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davvil davvil davvil approved these changes

@fhieber fhieber Awaiting requested review from fhieber

@mjdenkowski mjdenkowski Awaiting requested review from mjdenkowski

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tdomhan @davvil