Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Change Interface of NDArray & TBlob for DLPack Compatible #6345
Conversation
@@ -405,6 +402,16 @@ class NDArray { | ||
} | ||
}; | ||
+ void SetTBlob() { |
piiswrong
May 19, 2017
Contributor
Do you still need this if you are setting it everytime in data()?
+ * \brief return the corresponding DLTensor | ||
+ * \return the address of internal DLTensor | ||
+ */ | ||
+ inline const DLTensor& GetDLTensor() { |
@@ -614,8 +614,7 @@ NDArray &NDArray::operator/=(const real_t &src) { | ||
} | ||
void NDArray::Save(dmlc::Stream *strm) const { | ||
- // save shape | ||
- shape_.Save(strm); |
piiswrong
May 19, 2017
Contributor
@tqchen Does ndarray dupm files have a version system? Can we store new arrays as int64 while supporting legacy read from int32?
ZihengJiang
May 19, 2017
Contributor
Let us do it in another PR. For now, we just make sure it is backward compatible.
tqchen
May 20, 2017
Contributor
There is a reserved field on the dump file format that can be used for version checking
tqchen
May 20, 2017
Contributor
@ZihengJiang maybe consider to do a backward compatible version now by utilizing the hint?
piiswrong
May 20, 2017
Contributor
You can bump the version to 0.9.7 and store that in the reserved field
+ | ||
+ inline void SetDLTensor() { | ||
+ dltensor_.data = dptr_; | ||
+ dltensor_.ctx = DLContext{-1, static_cast<DLDeviceType>(dev_mask_)}; |
tqchen
May 21, 2017
Contributor
we need to fix context to be the right one when we really want to use DLTensor, otherwise it won't be usable
tqchen
May 22, 2017
Contributor
@piiswrong any ideas? do you think if we can construct TBlob with dev_id by default?
piiswrong
May 22, 2017
•
Contributor
we can have a new argument defaults to -1
then modify all the places it was used in mxnet.
tqchen
May 22, 2017
Contributor
to be clear, device_id equals -1 won't be able to be used in dlpack compatible functions
- stride_(shape[shape.ndim() - 1]), | ||
- dev_mask_(dev_mask), | ||
- type_flag_(type_flag) { | ||
+ TBlob(void *dptr, const TShape &shape, int dev_mask, int dev_id, int type_flag) |
@@ -121,7 +121,7 @@ class NDArray { | ||
raw_shape[0] = length; | ||
MSHADOW_TYPE_SWITCH(dtype_, DType, { | ||
res = TBlob(static_cast<DType*>(ptr_->shandle.dptr) | ||
- + offset_ + offset, raw_shape, ptr_->shandle.ctx.dev_mask()); | ||
+ + offset_ + offset, raw_shape, ptr_->shandle.ctx); |
ZihengJiang
May 24, 2017
Contributor
the original tblob::chunk has one offset, also raw_data can accept an offset as an argument
@@ -9,7 +9,10 @@ | ||
url = https://github.com/dmlc/ps-lite | ||
[submodule "nnvm"] | ||
path = nnvm | ||
- url = https://github.com/dmlc/nnvm | ||
+ url = https://github.com/ZihengJiang/nnvm |
ZihengJiang
May 24, 2017
Contributor
As you can see here in the log of ci(http://ec2-52-25-96-65.us-west-2.compute.amazonaws.com/blue/organizations/jenkins/mxnet/detail/PR-6345/12/pipeline/220#step-293-log-131). The size of input_shape is right in the mxnet side but wrong in the nnvm side. So I'm trying to log it out in my nnvm branch for debuging. (There maybe something wrong in CI with updated submodule). But it looks like this method doesn't work due to CI cannot change to my branch.
ZihengJiang
added some commits
May 24, 2017
@@ -639,25 +643,35 @@ void NDArray::Save(dmlc::Stream *strm) const { | ||
} | ||
bool NDArray::Load(dmlc::Stream *strm) { |
please add tests for legacy ndarray loading |
ZihengJiang
added some commits
May 28, 2017
- TBlob(DType *dptr, | ||
- const TShape &shape, | ||
- int dev_mask) | ||
+ TBlob(DType *dptr, const TShape &shape, int dev_mask, int dev_id) |
piiswrong
May 29, 2017
•
Contributor
TBlob constructor interface need to be backward compatible. add -1 as default here
ZihengJiang commentedMay 19, 2017
•
Edited 1 time
-
ZihengJiang
May 19, 2017
@tqchen @piiswrong @mli