-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Fix/public internal header #12374
Fix/public internal header #12374
Conversation
src/common/random_generator.cu
Outdated
@@ -59,6 +59,16 @@ void RandGenerator<gpu, float>::Seed(mshadow::Stream<gpu> *s, uint32_t seed) { | |||
s->Wait(); | |||
} | |||
|
|||
|
|||
static void RandGenerator<gpu, float>::AllocState(RandGenerator<gpu, DType> *inst) { |
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.
Isn't the only defined specialization double based?
src/common/random_generator.h
Outdated
} // namespace common | ||
} // namespace mxnet | ||
#endif // MXNET_COMMON_RANDOM_GENERATOR_H_ | ||
///* |
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.
Is this on purpose?
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.
need to update PR name, still working on this. commented out before deleting.
263ad0a
to
b07262f
Compare
65bc335
to
ac51562
Compare
include/mxnet/random_generator.h
Outdated
@@ -80,7 +79,7 @@ class RandGenerator<cpu, DType> { | |||
std::mt19937 *engine_; | |||
}; | |||
|
|||
static void AllocState(RandGenerator<cpu, DType> *inst) { | |||
static void AllocState(RandGenerator<cpu, DType> *inst) { |
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.
some redundant whitespace
556f83d
to
4c53b38
Compare
@azai91 did you mean "...and should not include internal dependencies..." ? @apeforest @samskalicky @anirudh2290 can you please take a look? |
you're right. will fix PR description. |
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
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.
@azai91 What issue/ticket does this change fix? Please link them in the PR. And why treat CPU and GPU implementation differently?
CUDA_CALL(cudaMalloc(&inst->states_, | ||
kNumRandomStates * sizeof(curandStatePhilox4_32_10_t))); | ||
} | ||
static void AllocState(RandGenerator<gpu, DType> *inst); |
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 do we do this differently with CPU implementation?
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 do you mean? we are calling cudaMalloc in this case instead of just "new"
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 guess my question is not well phrased. What I meant is why move out from header file while leaving the same function for CPU in the header
Aside from that, I think these functions are inside the inner class Impl which is supposed to handle all the implementation. Therefore I think it is very logical to leave them here in the header file. Not to mention the performance advantage of calling inline 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.
I see what you mean. these cuda calls depends on cuda_utils.h which is in the commons folder. I am not sure if we want to expose those. thoughts?
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.
@apeforest are you indicating about perf advantages during compile time ? I think its okay to put the definition in random_generator.cu since we don't want to expose cuda_utils.
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.
@apeforest any thoughts on what to revise? if we want to be consistent we could create a random_generator.cc file and put the non-cuda implementations in there, but I personally think that is unnecessary for such small functions.
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.
@azai91 Thanks for your efforts in refactoring this. Since the main purpose of this PR is refactoring, I hope we can do it in the most elegant way. If we are making this file a public header, I would extract the implementation class Impl to another internal header file so that we do not expose the internal implementation details. By doing that, we can have put the implementation details for both CPU and GPU there. Please let me know your thoughts.
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 seems that they way we are using the random_generator, we expect the developer to be able to access the internal Impl class (https://github.com/azai91/incubator-mxnet/blob/6f7254c91709904a9fb6290f1998fcf2da818d0e/src/operator/random/unique_sample_op.h#L118).
the class is is public as well. I may be interpretting the developers intentions incorrectly though.
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 suppose the developers did not intend to directly access internal Impl class but was rather lacking proper interface API. In fact, throughout the repository, I only found one place that uses RandGenerator<cpu,
GType>::Impl` directly, and therefore I think it's not a big overhead to refactor that one line of code. Regarding how to separating Impl class from the interface, you might find this reference helpful: https://cpppatterns.com/patterns/pimpl.html
CUDA_CALL(cudaMalloc(&inst->states_, | ||
kNumRandomStates * sizeof(curandStatePhilox4_32_10_t))); | ||
} | ||
static void AllocState(RandGenerator<gpu, DType> *inst); |
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.
@apeforest are you indicating about perf advantages during compile time ? I think its okay to put the definition in random_generator.cu since we don't want to expose cuda_utils.
@@ -59,6 +59,17 @@ void RandGenerator<gpu, float>::Seed(mshadow::Stream<gpu> *s, uint32_t seed) { | |||
s->Wait(); | |||
} | |||
|
|||
template<> | |||
void RandGenerator<gpu, float>::AllocState(RandGenerator<gpu> *inst) { |
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.
doesnt this remove support for other DTypes like half_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.
No, the random generator always generates floats between 0 and 1, we scale them as needed in wherever the random generator is used
633d44e
to
19fcae6
Compare
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.
Approving this change given that this issue is blocking some customer development and a TODO ticket MXNET-948 has been created to refactor this class using a PIMP idiom.
I can't build mxnet on master from source anymore… getting this error:
I updated my submodules... I just pulled latest:
Any ideas? |
* move random-gen header to internal code * uncomment header * remove public method from cc * move cuda randgen logic to cc * remove private files * include correct header in cu random_gen * fix include in cu random-gen * fix template * remove static kw * add missing template expression * change dtype to float * remove old header * fix lint * fix imports * fix space * fix template * fix template * add todo * fix lint
Description
the public header
resource.h
current has internal dependencies. public headers should only define interfaces and should not include internal dependencies as it allows users to access non-exposed interfaces. move random_generator.h into public header domain. move private implementations into respective .cc files.Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments