-
Notifications
You must be signed in to change notification settings - Fork 613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Constant operator and Python wrapper. #1699
Conversation
!build |
CI MESSAGE: [1095600]: BUILD STARTED |
dali/python/nvidia/dali/ops.py
Outdated
@@ -21,7 +21,7 @@ | |||
from nvidia.dali import backend as b | |||
from nvidia.dali.types import _type_name_convert_to_string, _type_convert_value, \ | |||
_vector_element_type, _bool_types, _int_types, _int_like_types, _float_types, \ | |||
DALIDataType, CUDAStream, Constant | |||
DALIDataType, CUDAStream, ScalarConstant as _Constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...actually, we should add more underscores and import aliases here, because it pollutes ops
module.
CI MESSAGE: [1095600]: BUILD FAILED |
dali/python/nvidia/dali/ops.py
Outdated
arg_inp = kwargs[k] | ||
if arg_inp is None: | ||
continue | ||
if type(arg_inp) is _Constant: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could write this for consistency:
if type(arg_inp) is _Constant: | |
if isinstance(arg_inp, _Constant): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
dali/python/nvidia/dali/ops.py
Outdated
@@ -291,6 +291,9 @@ def __init__(self): | |||
def id(self): | |||
return self._id | |||
|
|||
def _instantiate_constant_node(constant): | |||
return Constant(value = [constant.value], dtype = constant.dtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return Constant(value = [constant.value], dtype = constant.dtype) | |
return _Constant(value = [constant.value], dtype = constant.dtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's intended (it should be ops.Constant, but I don't think a module can refer to itself in Python). I renamed _Constant to _ScalarConstant to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic changed. Now I use the types.Constant function.
dali/python/nvidia/dali/types.py
Outdated
def _numpy_to_dali_type(t): | ||
if t is None: | ||
return None | ||
import numpy as np |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this import here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, it's already there in the outer scope.
dali/python/nvidia/dali/ops.py
Outdated
@@ -21,7 +21,7 @@ | |||
from nvidia.dali import backend as b | |||
from nvidia.dali.types import _type_name_convert_to_string, _type_convert_value, \ | |||
_vector_element_type, _bool_types, _int_types, _int_like_types, _float_types, \ | |||
DALIDataType, CUDAStream, Constant | |||
DALIDataType, CUDAStream, ScalarConstant as _Constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DALIDataType, CUDAStream, ScalarConstant as _Constant | |
DALIDataType, CUDAStream, Constant as _Constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it needs to be exactly the ScalarConstant
. I can use _ScalarConstant
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to add the operator itself.
!build |
CI MESSAGE: [1097209]: BUILD STARTED |
CI MESSAGE: [1097209]: BUILD PASSED |
dali/python/nvidia/dali/ops.py
Outdated
if arg_inp is None: | ||
continue | ||
if isinstance(type(arg_inp), _ScalarConstant): | ||
arg_inp = instantiate_constant_node(arg_inp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can only see _instantiate_constant_node
(starting with underscore). Is this codepath tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug. Fixed.
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@@ -314,14 +317,19 @@ def __init__(self, inputs, op, **kwargs): | |||
# Argument inputs | |||
for k in sorted(kwargs.keys()): | |||
if k not in ["name"]: | |||
if not isinstance(kwargs[k], _EdgeReference): | |||
arg_inp = kwargs[k] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe loop over (keys, values) here? But I don't know what up with sorting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if I can overwrite this reference in a loop when enumerating like that. It would certainly be quite unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorting is necessary to properly match inputs (which are numbered) to arguments. I'd leave it like this.
dali/operators/generic/constant.cc
Outdated
.AddOptionalArg<int>("shape", "The desired shape of the output. " | ||
"If not set, the data is assumed to be 1D", | ||
std::vector<int>()) | ||
.AddOptionalArg<float>("fdata", "Contents of the constant produced (for floating point types).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be prohibited to pass both of fdata
and idata
to one node. I think we should provide an error in such case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also warn the user in the doc string?
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
!build |
CI MESSAGE: [1097734]: BUILD STARTED |
CI MESSAGE: [1097734]: BUILD PASSED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please check my comments
|
||
The floating point input data should be placed in `fdata` argument and integer data in `idata`. | ||
The data is a flat vector of values or a single scalar. The data is then reshaped according | ||
to the `shape` argument. If the data is scalar, it will be broadcast to fill the entire shape. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to the `shape` argument. If the data is scalar, it will be broadcast to fill the entire shape. | |
to the `shape` argument. If the data is scalar, it will be broadcasted to fill the entire shape. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to a dictionary:
to broadcast; p. broadcast, occas. broadcasted.
dali/operators/generic/constant.cc
Outdated
.NumInput(0) | ||
.NumOutput(1) | ||
.AddOptionalArg<int>("shape", "The desired shape of the output. " | ||
"If not set, the data is assumed to be 1D", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: messy indentation here
dali/operators/generic/constant.cu
Outdated
assert(!idata_.empty()); | ||
FillTensorList<type>(output_, output_shape_, idata_, ws.stream()); | ||
} | ||
), (DALI_FAIL("Unsupported type"))); // NOLINT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print the type
dali/operators/generic/constant.h
Outdated
#include "dali/core/static_switch.h" | ||
|
||
#define CONSTANT_OP_SUPPORTED_TYPES \ | ||
(bool, int8_t, uint8_t, int16_t, uint16_t, int32_t, uint32_t, int64_t, uint64_t, float) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering, how about float16? It might be useful if we want to make a quick synthetic pipeline producing constant data just to measure the training speed without preprocessing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would need special treatment for GPU code to compile, but yeah, I can try.
dali/python/nvidia/dali/types.py
Outdated
"explicitly before casting to builtin `float`.").format(_float_types)) | ||
|
||
def __str__(self): | ||
return "{}:{}".format(self.value, self.dtype) | ||
|
||
def __repr__(self): | ||
return "{}".format(self.value) | ||
|
||
def _is_scalar_shape(shape): | ||
return shape is None or shape == 1 or shape == [1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return shape is None or shape == 1 or shape == [1] | |
return shape is None or shape == 1 or shape == [1] or shape == (1,) |
Does it make sense?
dali/python/nvidia/dali/types.py
Outdated
if value.dtype == np.uint64: | ||
value = value.astype(np.uint32) | ||
|
||
def _numpy_to_dali_type(t): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function seems useful enough to be exposed
@@ -0,0 +1,144 @@ | |||
# Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: 2020
bool SetupImpl(std::vector<OutputDesc> &output_desc, const Workspace &ws) override { | ||
output_desc.resize(1); | ||
if (output_shape_.empty()) { | ||
int batch_size = this->spec_.template GetArgument<int>("batch_size"); | ||
output_shape_ = uniform_list_shape(batch_size, shape_arg_); | ||
} | ||
output_desc[0] = { output_shape_, TypeTable::GetTypeInfo(output_type_) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do not want executor to allocate for us, why do we bother with this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. That is, the function can be there, but filling the output_desc is not necessary.
dali/operators/generic/constant.cu
Outdated
|
||
out.Reset(); | ||
out.ShareData(&output_); | ||
out.Resize(output_shape_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to cal Resize after ShareData?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need, the metadata (without the layout) is copied.
dali/operators/generic/constant.cu
Outdated
|
||
int n = tmp.size() * sizeof(Dst); | ||
for (int i = 0; i < shape.num_samples(); i++) | ||
cudaMemcpyAsync(dst.mutable_tensor<Dst>(i), tmp.data(), n, cudaMemcpyHostToDevice, stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this happens once (or rather batch size times), but someone complained to me once about invoking a lot of cudaMemcpys.
You probably can do the Host -> Device copy once and than do Device -> Device for the rest.
void FillTensorVector( | ||
TensorVector<CPUBackend> &dst, const TensorListShape<> &shape, const std::vector<Src> &src) { | ||
dst.SetContiguous(false); | ||
dst.Resize(shape); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Resize
+ mutable_data<Dst>
later will actually do a batch size of allocations.
If you really want to save that memory, you probably should do a singe Tensor, and share data with it.
Do we really want to be that clever or should we just go with one big allocation and have the actual TensorList underneath here?
It was supposed to get optimized in some MakeContiguous scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the constant node is supposed to be copied to the GPU, then it should have had been created with device="gpu" in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the first point - I traced in in the debugger, it doesn't allocate any memory because at the point of resize the underlying tensors don't have a type yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we adjust the tutorials a bit: https://docs.nvidia.com/deeplearning/sdk/dali-master-branch-user-guide/docs/examples/general/expressions/expr_examples.html#Constants
https://docs.nvidia.com/deeplearning/sdk/dali-master-branch-user-guide/docs/examples/general/expressions/expr_type_promotions.html#Using-Constants
There are mentions of Constant
, the code is backward compatible, but the meaning changed a bit.
shape = value.shape | ||
data = value.flatten().tolist() | ||
else: | ||
def _type_from_value_or_list(v): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What with bool
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
dali/python/nvidia/dali/types.py
Outdated
shape = shape, dtype = dtype, layout = layout) | ||
return op() | ||
|
||
def Constant(value, dtype = None, shape = None, layout = None, device = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe pass additional kwargs to the ops.Constant?
I'm thinking about naming the operator for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
yield _test_scalar_constant_promotion, "cpu" | ||
yield _test_scalar_constant_promotion, "gpu" | ||
|
||
def main(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not let nose run the test, and duplicate that. Should we maintain both the main
and the test case definitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's much easier to filter it here for debugging purposes.
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
!build |
CI MESSAGE: [1101517]: BUILD STARTED |
CI MESSAGE: [1101517]: BUILD PASSED |
@klecki Regarding docs: |
Add constant operator and unify it in Python with types.Constant.
Signed-off-by: Michal Zientkiewicz michalz@nvidia.com
Why we need this PR?
Pick one, remove the rest
What happened in this PR?
Fill relevant points, put NA otherwise. Replace anything inside []
dali.types.Constant
todali.types.ScalarConstant
and added a newdali.types.Constant
function which can produce the new Constant node.JIRA TASK: N/A