Gluon data 2.0: c++ dataloader and built-in image/bbox transforms #17841

zhreshold · 2020-03-15T22:49:03Z

Description

This is the implementation for proposal #17269

Changes

The major components of this PR is:

c++ threaded dataloader with native support from the following datasets/batchify_functions
c++ native support for datasets covering gluon.data.datasets and gluon.data.vision.datasets
c++ batchify functions including Stack, Pad, Groups
Gluon version of transforms that can apply data augmentations to image/bboxes
Gluon version of data iterator substitutes of mx.image.ImageIter and mx.image.ImageDetIter for simplest interface for loading image classification and object detection datsets.

Code Review

Since this PR involves lots of backend and frontend changes and can not be easily divided into multiple PRs, so I would like to suggest modular code reviews:

Modules	notes	suggested reviewers
include/mxnet/c_api.h/cc, include/mxnet/io.h, python/mxnet/gluon/data/_internal.py, python/mxnet/io/io.py	c api changes for io: datasets and batchify functions, with corresponding python frontends	@szha, @leezu
python/mxnet/gluon/data/batchify.py(dataloader.py, dataset.py)	gluon data loader and batchify functions	@sxjscience
python/mxnet/gluon/contrib/data/vision/dataloader.py, python/mxnet/gluon/contrib/data/vision/transforms/*	substitutes for deprecated mx.image.ImageIter and mx.image.ImageDetIter and old augmenters	all committers
python/mxnet/gluon/nn/basic_layers.py	changes to `nn.sequential` and `nn.HybridSequential` to allow Sequential block to take more than 1 arguments	all gluon contributors
python/mxnet/image/image.py	numpy compatitility change	@haojin2
src/imperative/*	Minor modification to cached op to allow operators to skip engine in imperative executions	@eric-haibin-lin
src/io/dataloader.cc(dataset.cc, batchify.cc), src/io/iter_sampler.cc	c++ implementations of gluon.data(gluon.data. dataset, gluon.data.batchify)	@leezu @sxjscience
src/operator/image/*	operators to support many random image transformations	committers familiar with vision operators

Thank you for everyone who participate in code review and sorry about the size of this PR.

zhreshold · 2020-03-20T20:57:27Z

Ping for reviewers' attention. I will fix lint along with review comments to reduce CI triggers.

python/mxnet/gluon/data/_internal.py

leezu · 2020-03-20T22:56:51Z

src/imperative/naive_cached_op.h

+
+
+namespace mxnet {
+/*! \brief NaiveCachedOp which does not involve engine which is useful when executed in parallel.


@eric-haibin-lin how many CachedOp's will we have after the unification?

src/io/dataloader.cc

include/mxnet/io.h

src/io/dataset.cc

include/mxnet/io.h

szha

Awesome job! Would you mind also looking into how we adopt the new FFI?

python/mxnet/gluon/data/batchify.py

zhreshold · 2020-03-22T21:36:15Z

Awesome job! Would you mind also looking into how we adopt the new FFI?

I am thinking of a separate PR that addresses the FFI improvement for all data related APIs. For now, maintaining consistency with other data APIs is preferable.

leezu

Let's use constructor to initialize the classes instead of requiring users to call Init after construction?

zhreshold · 2020-04-15T23:32:44Z

Let's use constructor to initialize the classes instead of requiring users to call Init after construction?

@leezu Sorry I missed this comment. If you look at the creator in c_api used for list and create the existing mxnet iterators: https://github.com/apache/incubator-mxnet/blob/master/src/c_api/c_api.cc#L1859, you will find that the registry is used to hold all uninitialized iterators that has been registered to io module: https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/io/io.py#L984

So if I follow the current registeration patter in python module, I have to follow the empty constructor + init function.

leezu · 2020-04-16T02:53:48Z

I'm not convinced. You can change the call signature of the registry and provide const std::vector<std::pair<std::string, std::string> >& kwargs to the constructor instead of the Init function. There should be no problem.

leezu

Added two comments inline to provide more information on how to use constructor

include/mxnet/io.h

src/c_api/c_api.cc

ruro · 2020-04-20T18:57:00Z

@zhreshold I see, you increased the timeout for the GPU tests. If you are trying to avoid aborts of the Python 3: GPU and Python 3: GPU (TVM_OP OFF) tests on unix-gpu after 3 hours, this won't help. This is a known bug, where those tests sometimes get completely stuck. These tests should normally complete in about an hour. See #18090 for discussion.

zhreshold · 2020-04-26T07:11:55Z

@leezu Outstanding comments fixed, can you take another look?
@szha @eric-haibin-lin can you please either request change or approve?

leezu

Thanks @zhreshold!

I have two general questions and also left some comments / requested changes inline:

What's the plan for python/mxnet/gluon/contrib/data/vision/transform? You mentioned we like to remove vision API and keep it in GluonCV only? Does that not apply to the data API? Will the data API be removed in GluonCV or how is the relation to the one in MXNet?
How can users run the pipeline / part of it on GPU? Some of the operators added in this PR do have GPU implementations, but I'm not sure how they could be used. Is there any plan on how to extend the current design to integrate with external libraries such as nvidia dali?

For the inline comments, the "requested changes" are with respect to CI Windows build script and the random state in the samplers. The others are more open-ended questions

ci/build_windows.py

leezu · 2020-04-28T18:19:47Z

python/mxnet/gluon/contrib/data/vision/dataloader.py

+
+    return augmenter
+
+class ImageDataLoader(object):


This seems to be mostly equivalent to using normal DataLoader and calling dataset.transform_first(augmenter)? Would it be easier for users and more maintainable to just use DataLoader?

It's a thin wrapper on top of DataLoader, the approach you mentioned is exactly what I was thinking of . In fact, anyone can directly use DataLoader with transform or transform_first by manually compose the transforms.

However, I guess users of the legacy ImageDetIter won't be happy with the deprecation of the old iterator so I am trying to keep a contrib version with augmenters wrapped in for convenient transition.

Why wouldn't they be happy, given that the composition of transforms appears straightforward?
Having a lot of wrappers around DataLoader will make it harder for us to maintain the codebase in the future, and may also confuse users as it's not clear to them that the manual composition approach is equivalent to using the wrapper. So I wonder if it's better to remove the wrapper and try keep our codebase shorter?

A 200 line of code can save each user 5min at least, especially for beginners. Actually it's not duplicating any code block and can handle most usecases for images.

So I think it's worth the effort to put in gluon.contrib and get maintained.

python/mxnet/gluon/data/dataloader.py

src/io/iter_sampler.cc

tests/python/unittest/test_numpy_contrib_gluon_data_vision.py

tests/python/unittest/test_contrib_gluon_data_vision.py

tests/python/unittest/test_numpy_interoperability.py

include/mxnet/io.h

src/io/dataset.cc

zhreshold · 2020-04-28T23:29:45Z

Thanks @zhreshold!

I have two general questions and also left some comments / requested changes inline:

What's the plan for python/mxnet/gluon/contrib/data/vision/transform? You mentioned we like to remove vision API and keep it in GluonCV only? Does that not apply to the data API? Will the data API be removed in GluonCV or how is the relation to the one in MXNet?

How can users run the pipeline / part of it on GPU? Some of the operators added in this PR do have GPU implementations, but I'm not sure how they could be used. Is there any plan on how to extend the current design to integrate with external libraries such as nvidia dali?

For the inline comments, the "requested changes" are with respect to CI Windows build script and the random state in the samplers. The others are more open-ended questions

@leezu

We are indeed removing CV specific implementation, especially network definitions, in favor of GluonCV. In the meantime some fundmental image processing transforms will still be added to MXNet. For definition of fundmental, I refer to the usage in D2L for guidance.
For GPU, there's a lot of topics we can discuss, including end-to-end GPU training, DALI integration, hybrid CPU/GPU data pipeline, (de)serialization of data pipeline. I am open to all these discussions, but do not have bandwidth to work on those areas yet.

leezu · 2020-04-29T00:00:15Z

So the fundamental APIs would only be maintained in MXNet side and removed from GluonCV?

zhreshold · 2020-04-29T00:16:31Z

So the fundamental APIs would only be maintained in MXNet side and removed from GluonCV?

@leezu Yes

src/io/dataset.cc

src/io/iter_sampler.cc

src/io/dataloader.cc

…ache#17841) * c++ dataloader and built-in image/bbox * update * fix error * fix import error * fix ci build * fix vs openmp loop type * fix warning as error with sign/unsign comp * sign/unsign comp * update to pytest * remove nose * fix tear_down * address comments * thread safe dataset * address comments * address comments * fix * serial pytest for data download (cherry picked from commit a0e6735)

…ache#17841) * c++ dataloader and built-in image/bbox * update * fix error * fix import error * fix ci build * fix vs openmp loop type * fix warning as error with sign/unsign comp * sign/unsign comp * update to pytest * remove nose * fix tear_down * address comments * thread safe dataset * address comments * address comments * fix * serial pytest for data download

zhreshold requested review from anirudh2290, eric-haibin-lin and szha as code owners March 15, 2020 22:49

zhreshold mentioned this pull request Mar 15, 2020

[WIP][mxnet 2.0] Pure C++ threaded dataloader that shared the interface with gluon dataloader #17464

Closed

6 tasks

leezu reviewed Mar 20, 2020

View reviewed changes

szha reviewed Mar 22, 2020

View reviewed changes

sxjscience reviewed Mar 22, 2020

View reviewed changes

python/mxnet/gluon/data/batchify.py Outdated Show resolved Hide resolved

leezu suggested changes Apr 15, 2020

View reviewed changes

leezu reviewed Apr 16, 2020

View reviewed changes

include/mxnet/io.h Outdated Show resolved Hide resolved

leezu reviewed Apr 16, 2020

View reviewed changes

src/c_api/c_api.cc Outdated Show resolved Hide resolved

zhreshold force-pushed the gluon-data-2 branch from 1b5b4c4 to 44c53e0 Compare April 17, 2020 19:24

zhreshold requested review from aaronmarkham, gigasquid, marcoabreu, nswamy and yzhliu as code owners April 17, 2020 19:24

zhreshold force-pushed the gluon-data-2 branch 2 times, most recently from 870bf59 to 5d0ac0f Compare April 23, 2020 23:48

leezu suggested changes Apr 28, 2020

View reviewed changes

leezu reviewed Apr 28, 2020

View reviewed changes

include/mxnet/io.h Outdated Show resolved Hide resolved

leezu reviewed Apr 28, 2020

View reviewed changes

src/io/dataset.cc Outdated Show resolved Hide resolved

zhreshold force-pushed the gluon-data-2 branch 2 times, most recently from 0a83a6f to 86b7c3f Compare May 1, 2020 20:12

leezu reviewed May 5, 2020

View reviewed changes

src/io/dataset.cc Outdated Show resolved Hide resolved

src/io/iter_sampler.cc Outdated Show resolved Hide resolved

src/io/dataloader.cc Outdated Show resolved Hide resolved

src/io/dataloader.cc Show resolved Hide resolved

leezu approved these changes May 6, 2020

View reviewed changes

zhreshold added 16 commits May 6, 2020 22:08

c++ dataloader and built-in image/bbox

7584efd

update

6f5f527

fix error

7996fe2

fix import error

07c02f8

fix ci build

318d7ae

fix vs openmp loop type

5d7e0e0

fix warning as error with sign/unsign comp

3d3177f

sign/unsign comp

b8048e4

update to pytest

e5cfbfc

remove nose

35d9a67

fix tear_down

e559bd8

address comments

28d5787

thread safe dataset

21d17d9

address comments

654ce99

address comments

9977a09

fix

46707f2

zhreshold force-pushed the gluon-data-2 branch from 79bdfeb to 46707f2 Compare May 7, 2020 05:08

serial pytest for data download

1ef6817

zhreshold merged commit a0e6735 into apache:master May 7, 2020

zhreshold mentioned this pull request May 13, 2020

[mxnet 2.0][item 4.8][RFC] Gluon Data API Extension and Fixes(Part 2) #17269

Closed

rondogency mentioned this pull request Jul 8, 2020

Gluon 2.0 Dataloader should support BERT training using GluonNLP #18672

Open

szha mentioned this pull request Aug 15, 2020

[Development] MXNet 2.0 Update #18931

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gluon data 2.0: c++ dataloader and built-in image/bbox transforms #17841

Gluon data 2.0: c++ dataloader and built-in image/bbox transforms #17841

zhreshold commented Mar 15, 2020

zhreshold commented Mar 20, 2020

leezu Mar 20, 2020

szha left a comment

zhreshold commented Mar 22, 2020

leezu left a comment

zhreshold commented Apr 15, 2020

leezu commented Apr 16, 2020

leezu left a comment

ruro commented Apr 20, 2020

zhreshold commented Apr 26, 2020

leezu left a comment

leezu Apr 28, 2020

zhreshold Apr 28, 2020

leezu Apr 29, 2020

zhreshold Apr 30, 2020

zhreshold commented Apr 28, 2020

leezu commented Apr 29, 2020

zhreshold commented Apr 29, 2020



		namespace mxnet {
		/*! \brief NaiveCachedOp which does not involve engine which is useful when executed in parallel.

Gluon data 2.0: c++ dataloader and built-in image/bbox transforms #17841

Gluon data 2.0: c++ dataloader and built-in image/bbox transforms #17841

Conversation

zhreshold commented Mar 15, 2020

Description

Changes

Code Review

zhreshold commented Mar 20, 2020

leezu Mar 20, 2020

Choose a reason for hiding this comment

szha left a comment

Choose a reason for hiding this comment

zhreshold commented Mar 22, 2020

leezu left a comment

Choose a reason for hiding this comment

zhreshold commented Apr 15, 2020

leezu commented Apr 16, 2020

leezu left a comment

Choose a reason for hiding this comment

ruro commented Apr 20, 2020

zhreshold commented Apr 26, 2020

leezu left a comment

Choose a reason for hiding this comment

leezu Apr 28, 2020

Choose a reason for hiding this comment

zhreshold Apr 28, 2020

Choose a reason for hiding this comment

leezu Apr 29, 2020

Choose a reason for hiding this comment

zhreshold Apr 30, 2020

Choose a reason for hiding this comment

zhreshold commented Apr 28, 2020

leezu commented Apr 29, 2020

zhreshold commented Apr 29, 2020