[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r386650044
##
File path: src/imperative/imperative.cc
##
@@ -273,7 +288,153 @@ void Imperative::RecordOp(
info.outputs.back().dtype_ = outputs[i]->dtype();
info.outputs.back().storage_type_ = outputs[i]->storage_type();
}
-outputs[i]->entry_ = nnvm::NodeEntry{node, i, 0};
+outputs[i]->autograd_entry_ = nnvm::NodeEntry{node, i, 0};
+ }
+}
+
+void Imperative::RecordDeferredCompute(nnvm::NodeAttrs &,
+ const std::vector ,
+ const std::vector ) {
+ CHECK(!is_recording())
+ << "Autograd recording is not supported during deferred compute mode.";
+
+ for (const NDArray *output : outputs) {
+CHECK(DCInfo::IsNone(*output))
+<< "Inplace operations (+=, -=, x[:]=, etc) are not supported when "
+<< "recording in deferred compute mode.";
+// However, an inplace operation on a non-deferred compute array inside
Review comment:
Correct. In the first case we just record `_slice_assign_scalar` Op. The
second case is unsupported because the `output` of `_slice_assign_scalar` is a
ndarray that already has a `deferredcompute_entry_` attribute.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r384156913
##
File path: src/imperative/imperative.cc
##
@@ -544,4 +705,60 @@ std::vector Imperative::Backward(
return {};
}
+Imperative::DCInfo::DCInfo(const std::vector ,
+ const std::vector ) {
+ this->inputs_.reserve(inputs.size());
+ this->input_handles_.reserve(inputs.size());
+ for (const NDArray *arr : inputs) {
+CHECK(!arr->is_none());
+this->inputs_.push_back(*arr);
+this->input_handles_.push_back(arr);
+ }
+
+ this->outputs_.reserve(outputs.size());
+ for (const NDArray *arr : outputs) {
+CHECK(!arr->is_none());
+this->outputs_.push_back(*arr);
+ }
+}
+
+Imperative::DCInfo &
+Imperative::DCInfo::Create(const nnvm::ObjectPtr ,
+ const std::vector ,
+ const std::vector ) {
+ node->info.construct(inputs, outputs);
+ return Imperative::DCInfo::Get(node);
+}
+
+void Imperative::DCInfo::Compute(const NDArray ) {
+ if (Imperative::DCInfo::IsComputed(arr))
+return;
+
+ DCInfo = Imperative::DCInfo::Get(arr.deferredcompute_entry_.node);
+ info.is_computed_ = true; // We will Invoke at the end of this function.
+
+ // Recursively compute input arrays
+ for (const NDArray : info.inputs_) {
+Compute(input);
+ }
+
+ // Prepare pointers
+ std::vector ndinputs, ndoutputs;
+ ndinputs.reserve(info.inputs_.size());
+ ndoutputs.reserve(info.outputs_.size());
+ for (NDArray : info.inputs_)
+ndinputs.push_back();
Review comment:
`` here points to an array in `info.inputs_`, which is
`std::vector inputs_`. Ie. it contains proper NDArrays and not only
pointers to NDArrays that could become invalid.
We control the lifetime of the NDArrays in `std::vector inputs_`.
They are copies, created based on the ndarray handle passed when invoking an
operator and created in in `DCInfo::Create`. The copies are destroyed here in
`DCInfo::Compute` after computation finished.
Note that the copies share the same underlying storage chunk. The storage
chunk is deallocated once the frontend doesn't hold any handle to an NDArray
owning that chunk anymore and once all copies located in DCInfo.inputs_ are
destroyed. This is done by `std::shared_ptr`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r384156913
##
File path: src/imperative/imperative.cc
##
@@ -544,4 +705,60 @@ std::vector Imperative::Backward(
return {};
}
+Imperative::DCInfo::DCInfo(const std::vector ,
+ const std::vector ) {
+ this->inputs_.reserve(inputs.size());
+ this->input_handles_.reserve(inputs.size());
+ for (const NDArray *arr : inputs) {
+CHECK(!arr->is_none());
+this->inputs_.push_back(*arr);
+this->input_handles_.push_back(arr);
+ }
+
+ this->outputs_.reserve(outputs.size());
+ for (const NDArray *arr : outputs) {
+CHECK(!arr->is_none());
+this->outputs_.push_back(*arr);
+ }
+}
+
+Imperative::DCInfo &
+Imperative::DCInfo::Create(const nnvm::ObjectPtr ,
+ const std::vector ,
+ const std::vector ) {
+ node->info.construct(inputs, outputs);
+ return Imperative::DCInfo::Get(node);
+}
+
+void Imperative::DCInfo::Compute(const NDArray ) {
+ if (Imperative::DCInfo::IsComputed(arr))
+return;
+
+ DCInfo = Imperative::DCInfo::Get(arr.deferredcompute_entry_.node);
+ info.is_computed_ = true; // We will Invoke at the end of this function.
+
+ // Recursively compute input arrays
+ for (const NDArray : info.inputs_) {
+Compute(input);
+ }
+
+ // Prepare pointers
+ std::vector ndinputs, ndoutputs;
+ ndinputs.reserve(info.inputs_.size());
+ ndoutputs.reserve(info.outputs_.size());
+ for (NDArray : info.inputs_)
+ndinputs.push_back();
Review comment:
`` here points to an array in `info.inputs_`, which is
`std::vector inputs_`. Ie. it contains proper NDArrays and not only
pointers to NDArrays that could become invalid.
We control the lifetime of the NDArrays in `std::vector inputs_`.
They are copies, created based on the ndarray handle passed when invoking an
operator and created in in `DCInfo::Create`. The copies are destroyed here in
`DCInfo::Compute` after computation finished.
Note that the copies share the same underlying storage chunk. The storage
chunk is deallocated once the frontend doesn't hold any handle to an NDArray
owning that chunk anymore and once all copies located in DCInfo.inputs_ are
destroyed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r384153607
##
File path: src/c_api/c_api_ndarray.cc
##
@@ -107,9 +107,17 @@ void MXImperativeInvokeImpl(AtomicSymbolCreator creator,
SetNDInputsOutputs(op, , , num_inputs, inputs,
num_outputs, infered_num_outputs, num_visible_outputs, outputs);
- auto state = Imperative::Get()->Invoke(Context::CPU(), attrs, ndinputs,
ndoutputs);
- if (Imperative::Get()->is_recording()) {
-Imperative::Get()->RecordOp(std::move(attrs), ndinputs, ndoutputs, state);
+ if (Imperative::Get()->is_deferred_compute()) {
+Imperative::Get()->RecordDeferredCompute(std::move(attrs), ndinputs,
ndoutputs);
+ } else {
+for (NDArray* input : ndinputs) {
+ Imperative::DCInfo::Compute(*input);
+}
+auto state = Imperative::Get()->Invoke(
+ Context::CPU(), attrs, ndinputs, ndoutputs);
Review comment:
This context is only used as `default_ctx`. (Also, this line is not changed
in this PR.)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r381565244
##
File path: src/imperative/cached_op.h
##
@@ -294,7 +294,7 @@ void SetInputIndices(const nnvm::Graph& fwd_graph,
const auto& indexed_graph = fwd_graph.indexed_graph();
if (data_indices->ndim() || param_indices.ndim()) {
CHECK_EQ(data_indices->ndim() + param_indices.ndim(),
- indexed_graph.input_nodes().size());
+ static_cast(indexed_graph.input_nodes().size()));
Review comment:
Was able to reproduce with Makefile based build and older gcc. With that
setup, only
``` c++
#include "./imperative_utils.h"
#include "./cached_op.h"
```
works in `imperative.cc`, but not
``` c++
#include "./cached_op.h"
#include "./imperative_utils.h"
```
which was used in the PR initially.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r381565244
##
File path: src/imperative/cached_op.h
##
@@ -294,7 +294,7 @@ void SetInputIndices(const nnvm::Graph& fwd_graph,
const auto& indexed_graph = fwd_graph.indexed_graph();
if (data_indices->ndim() || param_indices.ndim()) {
CHECK_EQ(data_indices->ndim() + param_indices.ndim(),
- indexed_graph.input_nodes().size());
+ static_cast(indexed_graph.input_nodes().size()));
Review comment:
Was able to reproduce with Makefile based build and older gcc. With that
setup, only
``` c++
#include "./imperative_utils.h"
#include "./cached_op.h"
```
works in `imperative.cc`, but not
``` c++
#include "./cached_op.h"
#include "./imperative_utils.h"
```
which was used in the PR initially due to editor auto-sorting imports by
name.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r379850353
##
File path: src/imperative/cached_op.h
##
@@ -294,7 +294,7 @@ void SetInputIndices(const nnvm::Graph& fwd_graph,
const auto& indexed_graph = fwd_graph.indexed_graph();
if (data_indices->ndim() || param_indices.ndim()) {
CHECK_EQ(data_indices->ndim() + param_indices.ndim(),
- indexed_graph.input_nodes().size());
+ static_cast(indexed_graph.input_nodes().size()));
Review comment:
Yes. Unfortunately there seems to be further undefined behavior and the CI
needs more fixing (see failure status below.. Can't reproduce any of these
failures locally..)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r379850186
##
File path: src/imperative/imperative.cc
##
@@ -273,8 +288,126 @@ void Imperative::RecordOp(
info.outputs.back().dtype_ = outputs[i]->dtype();
info.outputs.back().storage_type_ = outputs[i]->storage_type();
}
-outputs[i]->entry_ = nnvm::NodeEntry{node, i, 0};
+outputs[i]->autograd_entry_ = nnvm::NodeEntry{node, i, 0};
+ }
+}
+
+void Imperative::RecordDeferredCompute(nnvm::NodeAttrs &,
+ const std::vector ,
+ const std::vector ) {
+ CHECK(!is_recording())
+ << "Autograd recording is not supported during deferred compute mode.";
+
+ for (const NDArray *output : outputs) {
+CHECK(DCInfo::IsNone(*output))
+<< "Inplace operations (+=, -=, x[:]=, etc) are not supported when "
+<< "recording in deferred compute mode.";
+// However, an inplace operation on a non-deferred compute array inside
+// deferred compute scope will work. For example:
+// a = mx.nd.arange(10)
+// with dc.context():
+// a[:5] = 0
+ }
+ DispatchMode dispatch_mode = DispatchMode::kUndefined;
+ Context default_ctx = Context::CPU();
+ Context ctx = imperative::GetContext(attrs, inputs, outputs, default_ctx);
+ imperative::SetShapeType(ctx, attrs, inputs, outputs, _mode);
+
+ nnvm::ObjectPtr node = nnvm::Node::Create();
+ node->inputs.reserve(inputs.size());
+ // Get NodeEntries for inputs
+ for (const NDArray *array : inputs) {
+// For non-deferred compute arrays, array->deferredcompute_entry_ will be
+// nullptr. We handle this in in GetDeferredComputeSymbol
+node->inputs.emplace_back(array->deferredcompute_entry_);
+ }
+ node->attrs = std::move(attrs);
+ // Need to support NameManager in imperative API to better name
node->attrs.name
+ node->attrs.name = "node_" + std::to_string(node_count_++);
+ DCInfo::Create(node, inputs, outputs);
+
+ for (uint32_t i = 0; i < outputs.size(); ++i) {
+outputs[i]->deferredcompute_entry_ = nnvm::NodeEntry{node, i, 0};
+ }
+}
+
+nnvm::Symbol Imperative::GetDeferredComputeSymbol(
+const std::vector> ,
+const std::vector
+) {
+ Symbol s;
+ s.outputs.reserve(outputs.size());
+ for (NDArray * ndoutput : outputs) {
+CHECK(!Imperative::DCInfo::IsNone(*ndoutput))
+<< "ValueError: output_arrays for GetDeferredComputeSymbol "
+<< "must have a deferred compute history associated with them.";
+s.outputs.emplace_back(ndoutput->deferredcompute_entry_);
}
+ std::unordered_map ndinput_to_variable;
+ std::unordered_set missing_inputs;
+ auto add_symbol_variables = [, _to_variable,
+ _inputs](const nnvm::ObjectPtr ) {
+if (node == nullptr) {
+ // This (nonexistant) "Node" belongs to an array created outside of
deferred compute scope.
+ return;
+}
+
+// Check if node has any non-deferred compute inputs
+for (uint32_t i = 0; i < node->inputs.size(); i++) {
+ nnvm::NodeEntry _entry = node->inputs[i];
+ if (node_entry.node == nullptr || node_entry.node->is_variable()) {
+// Node has non-deferred compute input (nullptr). Find the
corresponding
+// NDArray and create a variable for it. If GetDeferredComputeSymbol
has
+// been called before, a variable already exists and only the name
needs
+// to be updated.
+Imperative::DCInfo = Imperative::DCInfo::Get(node);
+const NDArray *array = dcinfo.input_handles_.at(i);
+
+// Make sure this array is part of GetDeferredComputeSymbol inputs
+auto is_equal = [array](const std::pair
) {
+ return array == std::get<0>(input);
+};
+
+// std::vector>::iterator
input_search =
Review comment:
That one shouldn't have been a comment. I didn't figure out the correct type
at the time of writing and used auto as intermediate solution.
Correct type is `std::vector>::const_iterator` (note the `const`) and is used now.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r379849366
##
File path: src/c_api/c_api_ndarray.cc
##
@@ -428,3 +436,41 @@ int MXCachedOpRegisterOpHook(NDArrayHandle handle,
op->RegisterOpHook(clbk, monitor_all);
API_END();
}
+
+int MXNDArrayIsDeferredComputeEnabled(int *curr) {
Review comment:
`MX` prefix is required for symbol visibility. `NDArray` as this is part of
the `NDArray` related functions.
For consistency with autograd APIs, let's remove the `Enabled` part indeed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r379849366
##
File path: src/c_api/c_api_ndarray.cc
##
@@ -428,3 +436,41 @@ int MXCachedOpRegisterOpHook(NDArrayHandle handle,
op->RegisterOpHook(clbk, monitor_all);
API_END();
}
+
+int MXNDArrayIsDeferredComputeEnabled(int *curr) {
Review comment:
`MX` prefix is required for symbol visibility. `NDArray` as this is part of
the `NDArray` related functions.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r379849363
##
File path: src/c_api/c_api_ndarray.cc
##
@@ -428,3 +436,41 @@ int MXCachedOpRegisterOpHook(NDArrayHandle handle,
op->RegisterOpHook(clbk, monitor_all);
API_END();
}
+
+int MXNDArrayIsDeferredComputeEnabled(int *curr) {
+ API_BEGIN();
+ *curr = Imperative::Get()->is_deferred_compute();
+ API_END();
+}
+
+int MXNDArraySetDeferredComputeEnabled(int deferred_compute, int *prev) {
Review comment:
For consistency with autograd APIs, lets call it
`MXNDArraySetIsDeferredCompute`
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r379021790 ## File path: python/mxnet/_deferred_compute.py ## @@ -0,0 +1,95 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Deferred Compute for NDArray.""" + +import ctypes +import contextlib + +from .base import _LIB, check_call, SymbolHandle, _as_list +from .symbol import Symbol + +__all__ = [] + +def is_deferred_compute(): +"""Get status of deferred compute mode.""" +curr = ctypes.c_bool() +check_call(_LIB.MXNDArrayIsDeferredComputeEnabled(ctypes.byref(curr))) +return curr.value + +def set_deferred_compute(is_deferred_compute): +"""Enable / Disable deferred compute mode. + +Parameters +-- +is_deferred_compute: bool + +Returns +--- +Previous deferred compute state. +""" +prev = ctypes.c_int() +check_call(_LIB.MXNDArraySetDeferredComputeEnabled( +ctypes.c_int(is_deferred_compute), ctypes.byref(prev))) +return bool(prev.value) + + +@contextlib.contextmanager +def context(): +# Like other MXNet context manager, this bleeds state across concurrent Review comment: This refers to problems if users use https://docs.python.org/3/library/asyncio.html This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r379012093
##
File path: include/mxnet/ndarray.h
##
@@ -83,7 +83,7 @@ class NDArray {
public:
/*! \brief default constructor */
NDArray()
-: entry_(nullptr) {
+: autograd_entry_(nullptr) {
Review comment:
It's not required? We just use the `NodeEntry` default constructor.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #17530: Add deferred compute support
leezu commented on a change in pull request #17530: Add deferred compute support
URL: https://github.com/apache/incubator-mxnet/pull/17530#discussion_r379009156
##
File path: include/mxnet/imperative.h
##
@@ -88,6 +89,77 @@ class Imperative {
&& info.out_grads.size() == 1;
}
};
+
+ /*! \brief DCInfo datastructure to enable deferred computation */
+ class DCInfo {
+ public:
+DCInfo() {
+ // Default constructor provided for the sake of any.h. Should not be
used.
+ throw std::invalid_argument("Unsupported default constructor");
+}
Review comment:
Good catch. This was only needed in an earlier, unpublished version of this
PR.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
