mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jun Wu <wujun....@gmail.com>
Subject Re: Implementing zero-dim and zero-size tensors in MXNet and its impact on your codebases
Date Thu, 11 Apr 2019 23:05:25 GMT
Thank all for the discussion. To make this PR unblocked by the discussion
of semver concerns, I made copies of the APIs I need to change and worked
on those copies. Thanks to Tong, Yizhi, and Sergey who updated all the
language bindings to use the new C APIs supporting zero-dim/size tensors,
this PR currently is free of SemVer concerns on either C APIs and any
language bindings.

Nevertheless, I think it's the time for us to ponder the necessity of
keeping all C APIs under the umbrella of semver. As Marco pointed out,
treating C APIs as user-facing APIs brings in the risk of diverging
frontend functionality as well as considerably increases the complexity of
development. We can conduct a survey among MXNet users by collecting the C
APIs they directly interact with and keep those under semver.

Thank all again for sharing the thoughts. We will keeping rolling out NumPy
compatible features after this PR and look forward to helping MXNet users
migrate to using NumPy features.

On Thu, Apr 11, 2019 at 12:32 PM Anirudh Subramanian <anirudh2290@gmail.com>
wrote:

> Hi Marco,
>
> The backend private APIs in engine, executor, storage, ndarray etc. can
> still be changed.
> I understand that it may introduce code duplication, but introducing
> duplicate C APIs can still be better than the backend
> developer having to worry about different frontends. Not to mention a
> frontend which is not yet merged to the
> repo but in its own repo, these repos should also be considered consumers
> of MXNet API.
>
> Anirudh
>
> On Thu, Apr 11, 2019 at 12:12 PM Marco de Abreu <marco.g.abreu@gmail.com>
> wrote:
>
> > Good point about the adoption speed for the different frontends, Anirudh.
> > While this is a quite valid argument, I'm afraid of the complexity it
> might
> > introduce as well as a risk of further diverging frontend functionality.
> >
> > I'd rather propose that we introduce a guideline to follow when changes
> to
> > C-APIs are being made. Part of that could be starting a thread like this
> > one that lays down the changes that are being made to the C-API. We could
> > then coordinate the changes to the different frontends and gather people
> > from the community who feel comfortable to do the changes in the
> respective
> > frontends. If nobody speaks up, the original proposer of that change
> could
> > be responsible to do the necessary changes.
> >
> > An adjacent topic for this discussion could be test coverage: We
> currently
> > have no tools to determine which frontend hits which C-API and where
> > changes have to be made. This might be a topic we should spark up again
> > separately.
> >
> > -Marco
> >
> > On Thu, Apr 11, 2019 at 8:55 PM Marco de Abreu <marco.g.abreu@gmail.com>
> > wrote:
> >
> > > My personal opinion towards that discussion is that we should keep the
> > > C-API free from semantic versioning because otherwise we're introducing
> > two
> > > "fronts" that we have to maintain backwards compatibility for. By the
> > way,
> > > currently, we have no way to verify and guarantee the compatibility of
> > the
> > > C-API. The major issue I'd see with adding SemVer for the C-API is that
> > > this would increase the complexity of changes that are (in my opinion)
> > > entirely internal to MXNet by introducing another thing that developers
> > > would have to look out for - possibly introducing code duplication as
> > > described by Jun while not providing any clear benefits to me.
> > >
> > > If there is a use-case where people can not even use our C++ package,
> > then
> > > we could have discussions about introducing a user-facing C-API, but
> > right
> > > now this approach to interface with our C-API (although I know that
> > people
> > > use it) seem a bit like using undocumented Windows APIs: They work, but
> > > it's on your own risk, they might always break and there's no
> guarantee.
> > >
> > > -Marco
> > >
> > > On Thu, Apr 11, 2019 at 8:52 PM Anirudh Subramanian <
> > anirudh2290@gmail.com>
> > > wrote:
> > >
> > >> Hi Jun,
> > >>
> > >> Till now from what I have observed this has been an undocumented
> > guideline
> > >> to not break C APIs (example:
> > >>
> >
> https://github.com/apache/incubator-mxnet/pull/11429#discussion_r199564999
> > >> ).
> > >> Although the C APIs are supposed to serve only as bridges for frontend
> > >> language bindings (exception being C Predict API), I think there are
> 3rd
> > >> party libraries like Horovod which are starting to
> > >> depend on it (https://github.com/apache/incubator-mxnet/pull/14615) .
> > >>
> > >> Also, since MXNet has a lot of frontend bindings ensuring backward
> > >> compatibility with semver can help frontend bindings adopt the new
> APIs
> > at
> > >> their own pace.
> > >>
> > >> Anirudh
> > >>
> > >>
> > >> On Thu, Apr 11, 2019 at 10:58 AM Jun Wu <wujun.nju@gmail.com> wrote:
> > >>
> > >> > I'm not sure about whether C APIs should fall under semver. This is
> > the
> > >> > discussion we would like to have with the community.
> > >> >
> > >> > My thinking on this:
> > >> > 1. In most of the cases, C APIs only serve as bridges between
> frontend
> > >> > language bindings and C++ backend. Most of users/developers do not
> > >> interact
> > >> > directly with C APIs.
> > >> > 2. The cases I can think of where C APIs are directly adopted in
> > >> > application development are model deployment in a C/C++ environment.
> > In
> > >> > those cases, developers only interact with C Predict APIs, which we
> > >> didn't
> > >> > touch.
> > >> >
> > >> > If the community feel that we are obliged to keep the semver for
> all C
> > >> > APIs, we can try to make a copy of the C APIs we intend to modify
in
> > >> the PR
> > >> > and keep the old signatures intact, this will introduce a lot of
> > >> duplicate
> > >> > code though.
> > >> >
> > >> > On Thu, Apr 11, 2019 at 8:50 AM Anirudh Subramanian <
> > >> anirudh2290@gmail.com
> > >> > >
> > >> > wrote:
> > >> >
> > >> > > I was under the impression that C API does fall under semver.
Has
> > this
> > >> > been
> > >> > > discussed somewhere before ? Is this also the case for C Predict
> > API ?
> > >> > >
> > >> > > On Thu, Apr 11, 2019, 8:08 AM Marco de Abreu <
> > marco.g.abreu@gmail.com
> > >> >
> > >> > > wrote:
> > >> > >
> > >> > > > In case only changes to the c-api are being made, it doesn't
> fall
> > >> under
> > >> > > our
> > >> > > > semantic versioning since that's not a user facing API and
thus
> > I'd
> > >> be
> > >> > in
> > >> > > > favour as doing it as part of a minor release. If there
is any
> > >> > > behavioural
> > >> > > > change from a user perspective (a good indicator would be
if
> tests
> > >> have
> > >> > > to
> > >> > > > be changed as reaction to the Backend changes), then I'd
prefer
> a
> > >> major
> > >> > > > release.
> > >> > > >
> > >> > > > I'd slightly prefer a minor release since this change touches
> > quite
> > >> a
> > >> > few
> > >> > > > parts and could risk being outdated/diverged as the time
until
> 2.0
> > >> > > > progresses.
> > >> > > >
> > >> > > > -Marco
> > >> > > >
> > >> > > > Aaron Markham <aaron.s.markham@gmail.com> schrieb
am Do., 11.
> > Apr.
> > >> > 2019,
> > >> > > > 16:28:
> > >> > > >
> > >> > > > > Just curious about when this kind of change will land.
Would
> it
> > >> wait
> > >> > > for
> > >> > > > > 2.0 or would it be in 1.5 or another minor release?
> > >> > > > >
> > >> > > > > On Thu, Apr 11, 2019, 00:15 Junru Shao <
> junrushao1994@gmail.com
> > >
> > >> > > wrote:
> > >> > > > >
> > >> > > > > > Really nice improvement over MXNet's usability!
I suggest
> that
> > >> we
> > >> > > could
> > >> > > > > > make numpy-compatible behavior default in 2.0.
> > >> > > > > >
> > >> > > > > > On Wed, Apr 10, 2019 at 11:34 PM Jun Wu <
> wujun.nju@gmail.com>
> > >> > wrote:
> > >> > > > > >
> > >> > > > > > > Dear Community,
> > >> > > > > > >
> > >> > > > > > > A while ago, we sent out an RFC
> > >> > > > > > > <https://github.com/apache/incubator-mxnet/issues/14253>
> > >> > > discussing
> > >> > > > > the
> > >> > > > > > > initiative introducing NumPy compatibility
into MXNet. As
> > the
> > >> > first
> > >> > > > > > outcome
> > >> > > > > > > of this initiative, we submitted the PR
> > >> > > > > > > <https://github.com/apache/incubator-mxnet/pull/14661>
> > >> providing
> > >> > > the
> > >> > > > > > > infrastructure of supporting zero-dim (scalar)
and
> zero-size
> > >> > > tensors,
> > >> > > > > > which
> > >> > > > > > > have been long-missing in MXNet.
> > >> > > > > > >
> > >> > > > > > > In our implementation, we have put the best
efforts of
> > keeping
> > >> > the
> > >> > > > > > promise
> > >> > > > > > > of backward compatibility in all the language
bindings.
> > >> > > Nevertheless,
> > >> > > > > we
> > >> > > > > > > still would like to call out the changes
explicitly that
> may
> > >> > impact
> > >> > > > > your
> > >> > > > > > > existing codebases developed on top of MXNet
by calling
> > C-APIs
> > >> > > > directly
> > >> > > > > > or
> > >> > > > > > > implementing operators in your own repos.
> > >> > > > > > >
> > >> > > > > > > 1. In you application, if you called any
one of the
> > following
> > >> > > > > > shape-related
> > >> > > > > > > C-APIs, you will need to change the data
type of shape's
> > ndim
> > >> and
> > >> > > > > > dim_size
> > >> > > > > > > from *unsigned int* to signed *int*, because
we have to
> use
> > >> -1 to
> > >> > > > > > represent
> > >> > > > > > > unknown shape information, and reserve 0
for scalar and
> > >> zero-size
> > >> > > > > > tensors.
> > >> > > > > > > One example of such changes can be seen in
the cpp-package
> > >> > > > > > > <
> > >> > > > > > >
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> >
> https://github.com/apache/incubator-mxnet/pull/14661/files#diff-c0e77771fcfe1619faa4ff5f59d94e8bR183
> > >> > > > > > > >
> > >> > > > > > > calling MXSymbolInferShape.
> > >> > > > > > > - MXSymbolInfershape
> > >> > > > > > > - MXSymbolInfershapePartial
> > >> > > > > > > - MXExecutorSimpleBind
> > >> > > > > > > - MXExecutorReshape
> > >> > > > > > > - MXNDArrayGetShape
> > >> > > > > > > - MXNDArrayCreaetFromSharedMem
> > >> > > > > > >
> > >> > > > > > > 2. If you have implemented operators in your
own
> codebases,
> > >> you
> > >> > > will
> > >> > > > > > > probably need to change every operator's
shape inference
> > >> function
> > >> > > to
> > >> > > > > use
> > >> > > > > > > the following util functions to check whether
shape
> > >> information
> > >> > is
> > >> > > > > known,
> > >> > > > > > > instead of checking against 0 directly. One
example of
> such
> > >> > changes
> > >> > > > can
> > >> > > > > > be
> > >> > > > > > > seen in the shape inference function
> > >> > > > > > > <
> > >> > > > > > >
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> >
> https://github.com/apache/incubator-mxnet/pull/14661/files#diff-afa640c4653c59f00f43a84455f91ef9R35
> > >> > > > > > > >
> > >> > > > > > > of concat operator.
> > >> > > > > > > - shape_is_known (include/mxnet/tuple.h)
> > >> > > > > > > - ndim_is_known (include/mxnet/tuple.h)
> > >> > > > > > > - dim_size_is_known (include/mxnet/tuple.h)
> > >> > > > > > >
> > >> > > > > > > If you are interested in knowing the value
of scalar
> > tensors,
> > >> and
> > >> > > > hence
> > >> > > > > > > understanding our motivation further, this
thread
> > >> > > > > > > <
> > >> > > >
> > >> https://discuss.mxnet.io/t/rank-0-arrays-in-mxnet-aka-pi-is-wrong/108
> > >> > > > > >
> > >> > > > > > of
> > >> > > > > > > discussion provides very good insights from
the view of
> data
> > >> > > science.
> > >> > > > > It
> > >> > > > > > > was actually related to an opportunity for
MXNet becoming
> > the
> > >> > > backend
> > >> > > > > of
> > >> > > > > > > PyMC <https://en.wikipedia.org/wiki/PyMC3>,
but somehow
> it
> > >> > didn't
> > >> > > go
> > >> > > > > > > through due to missing several key features
> > >> > > > > > > <
> > >> https://discuss.mxnet.io/t/moving-pymc3-from-theano-to-mxnet/86
> > >> > >,
> > >> > > > and
> > >> > > > > > > scalar tensors is one of them.
> > >> > > > > > >
> > >> > > > > > > Please leave comments in the PR
> > >> > > > > > > <https://github.com/apache/incubator-mxnet/pull/14661>
if
> > you
> > >> > have
> > >> > > > any
> > >> > > > > > > concerns or suggestions of our work.
> > >> > > > > > >
> > >> > > > > > > Thank you very much for your time and consideration.
> > >> > > > > > >
> > >> > > > > > > Best,
> > >> > > > > > > Jun
> > >> > > > > > >
> > >> > > > > > > *References*
> > >> > > > > > > [1] RFC of NumPy compatibility:
> > >> > > > > > > https://github.com/apache/incubator-mxnet/issues/14253
> > >> > > > > > > [2] Pull request of supporting scalar and
zero-size
> tensors:
> > >> > > > > > > https://github.com/apache/incubator-mxnet/pull/14661
> > >> > > > > > > [3] The value of scalar tensors from the
view of data
> > science:
> > >> > > > > > >
> > >> > > >
> > >> https://discuss.mxnet.io/t/rank-0-arrays-in-mxnet-aka-pi-is-wrong/108
> > >> > > > > > > [4] Previous discussion for MXNet becoming
the backend of
> > >> PyMC:
> > >> > > > > > >
> > >> https://discuss.mxnet.io/t/moving-pymc3-from-theano-to-mxnet/86
> > >> > > > > > >
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message