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

Fixed a bug in Gluon DataLoader. #15195

Merged
merged 4 commits into from
Jun 12, 2019
Merged

Conversation

chandana1332
Copy link
Contributor

@chandana1332 chandana1332 commented Jun 10, 2019

Issue: #15025
Fix: Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator

Description

Fixes #15025

Bug: Gluon DataLoader terminates the process pool early while _MultiWorkerIter is operating on the pool.
Cause: https://github.com/apache/incubator-mxnet/pull/13537/files
As seen in the patch, the process pool is terminated when DataLoader is garbage collected but the scope of the process pool goes beyond the DataLoader until _MultiWorkerIter

Fix: Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Code is well-documented:
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator. Added a test for the same.

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

    Issue: apache#15025
    Fix: Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator
@chandana1332 chandana1332 requested a review from szha as a code owner June 10, 2019 16:12
@vandanavk
Copy link
Contributor

vandanavk commented Jun 10, 2019

@mxnet-label-bot add [Gluon, Data-loading, pr-awaiting-review]

@chandana1332 Please add "Fixes #15025" in the PR description to automatically close the issue when this PR is merged.

@marcoabreu marcoabreu added Data-loading Gluon pr-awaiting-review PR is waiting for code review labels Jun 10, 2019
@chandana1332
Copy link
Contributor Author

@vandanavk Done!

Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@@ -409,7 +409,7 @@ def _thread_worker_fn(samples, batchify_fn, dataset):
class _MultiWorkerIter(object):
"""Internal multi-worker iterator for DataLoader."""
def __init__(self, worker_pool, batchify_fn, batch_sampler, pin_memory=False,
pin_device_id=0, worker_fn=_worker_fn, prefetch=0, dataset=None):
pin_device_id=0, worker_fn=_worker_fn, prefetch=0, dataset=None, dataloader=None):
Copy link
Member

Choose a reason for hiding this comment

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

nit: dataloader-> data_loader ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it.

    Issue: apache#15025
    Fix: Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator
    Issue: apache#15025
    Fix: Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator
    Issue: apache#15025
    Fix: Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator
@wkcn wkcn merged commit 2e20094 into apache:master Jun 12, 2019
@wkcn
Copy link
Member

wkcn commented Jun 12, 2019

Merged. Thank you for the fix!

haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* Fixed a bug in Gluon DataLoader.
    Issue: apache#15025
    Fix: Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator

* Fixed a bug in Gluon DataLoader.
    Issue: apache#15025
    Fix: Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator

* Fixed a bug in Gluon DataLoader.
    Issue: apache#15025
    Fix: Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator

* Fixed a bug in Gluon DataLoader.
    Issue: apache#15025
    Fix: Broadened the scope of worker pool to iterators. Passed a reference of dataloader to the multi worker iterator
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Data-loading Gluon pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gluon DataLoader incorrectly terminates the process pool in 1.4
5 participants