Fix warning / static function in header. #14900
Conversation
|
@mxnet-label-bot add [pr-awaiting-review, Operator] |
larroy
force-pushed
the
warning
branch
3 times, most recently
from
025169f to
50f4176
Compare
src/operator/nn/dropout-inl.h
Outdated
| @@ -492,7 +492,7 @@ class DropoutOp { | |||
| #endif // MXNET_USE_CUDNN_DROPOUT | |||
| }; // class DropoutOp | |||
|
|
|||
| static OpStatePtr CreateDropoutState(const nnvm::NodeAttrs &attrs, | |||
| inline OpStatePtr CreateDropoutState(const nnvm::NodeAttrs &attrs, | |||
There was a problem hiding this comment.
What is the warning message ?
There was a problem hiding this comment.
file included from /Users/pllarroy/devel/mxnet/tests/cpp/operator/dropout_perf.cc:30:
/Users/pllarroy/devel/mxnet/3rdparty/mshadow/../../src/operator/nn/dropout-inl.h:495:19: warning: unused function 'CreateDropoutState' [-Wunused-function]
static OpStatePtr CreateDropoutState(const nnvm::NodeAttrs &attrs,
^
But the relevant part is that
This is going to create multiple instantiations of this function, is not correct to have a static function in a header.
src/operator/nn/dropout-inl.h
Outdated
| @@ -492,7 +492,7 @@ class DropoutOp { | |||
| #endif // MXNET_USE_CUDNN_DROPOUT | |||
| }; // class DropoutOp | |||
|
|
|||
| static OpStatePtr CreateDropoutState(const nnvm::NodeAttrs &attrs, | |||
| inline OpStatePtr CreateDropoutState(const nnvm::NodeAttrs &attrs, | |||
There was a problem hiding this comment.
What is the warning message ?
|
@haojin2 to my knowledge there are no other ocurrences of this case. |
|
@haojin2 I agree, addressed your comments. It's a small function so being inline was fine in my opinon. In any case I moved it to the implementation file as per your request. Please review again. |
src/operator/nn/dropout.cc
Outdated
| namespace { | ||
|
|
||
| using namespace mxnet; | ||
| using namespace mxnet::op; |
There was a problem hiding this comment.
I'd prefer putting this function into mxnet::op namespace below, BTW this seems to be a duplicate of #14940, can you close this PR and merge your changes into that one?
There was a problem hiding this comment.
Can you elaborate on your request? is local to that file so I think it should be in the anonymous namespace. That would be the best practice in my opinion since it's not exposed publicly in that namespace.
There was a problem hiding this comment.
Is not a duplicate, if you look closely. As fixing this warning is required for the other PR. When this is merged I will rebase the other one.
There was a problem hiding this comment.
So if you merge the content of this PR into the other PR, does it not unblock your other PR? From the title and content of both PRs I'm reading that you're trying to get rid of a few compilation warnings, so how does this PR not fit into the scope of that PR then? I'm also a bit confused about how would a fix for a compilation warning be a dependency of the other PR?
You may also argue that you're addressing different types of warning per PR, but seems like that PR is a mixed fix for multiple causes of compilation warnings, so why cannot this fix be a part of it?
To be fair, I'm not against having multiple PRs to complete one goal, but only if you have a logical split/organization of your PRs. For example, #14926 and #14946 are essentially the same fix, and I personally prefer them to be merged into the same PR for easier tracking in the future.
Regarding the usage of anonymous namespace, I think keeping the current convention (simply putting it in the mxnet::op namespace) would cause less confusion/surprise for other collaborators. Since you believe this is the best practice, I'd prefer and be supportive of you raising another major refactor PR in the future to make this happen for every single such cases.
There was a problem hiding this comment.
@haojin2 You make good points and I think there's different schools of thought on grouping changes in bigger PRs or sending more smaller PRs/commits that can be reviewed quicker and bisected for bugs easier. While your points are totally valid, I think solving the warning was a simple change from static to inline, I separated into a single PR to have it merged quicker than a PR aggregating other unrelated changes and refactorings that can lead to extended discussions. In this case you requested additional refactor, I had to spend additional valuable time dealing with CI failures and again change to the op namespace which I think is a matter of taste and having diminishing returns. I would suggest to discuss this issue regarding grouping logical changes in a bigger PR in the mailing list so there can be a consensus on how we want to do it in the project. Discussing it on every PR is not productive for external contributors like me. After your second request for additional refactor I feel like closing the PR and not fixing any more warnings.
There was a problem hiding this comment.
Just a quick note, I believe that grouping the 2 specifc PRs in my last comment, which are essentially both doing exactly the same thing (changing from using mx_test_utils.list_gpus to mx.context.num_gpus), would lead to easier bisection in the future than having 2 different PRs which will be merged far apart on the timeline. And, even if you combine those 2, it would become a +30/-30-ish PR, which I would not consider as a big and difficult-to-review one. To be fair, I'm not trying to blame you for not grouping those PRs, simply suggesting a better way of performing similar fixes in the future.
On the other hand, I believe that while your time is precious, so is other people's. The reason to have a code review process is to make sure that every PR is merged to help the community to be a better one as a whole, otherwise why don't we automatically merge a PR if it does not break the CI?
I do understand that everyone tend to defend their own preferred way of doing certain things (I defend my code/docs/PRs from time to time too), but we're a community and you're also part of it (I'm not sure why you identify yourself as an "external contributor", I believe you have been in the community for quite a while, maybe you could elaborate a bit?), which means that we want to be considerate to others during our development. Organizing and documenting one's code/commits/PR in a fashion that's more logical and easier to understand/debug/re-use/extend should be a duty for everyone in the community, because we're not working alone on a private project. If those values do not align with what you believe at this moment, Apache Guidelines would be a good read.
I think this PR LGTM now, I'll approve it and it could be merged as long as @anirudh2290 has no more comments (there have been several new changes since his last approval). Thanks for your contribution.
There was a problem hiding this comment.
- Every time a build request is sent to the CI it costs, a lot. It would be great if we all could be mindful about it when organizing PRs. We trust people to use the best judgment.
- While I fully trust that it's not @larroy's intention to send duplicate PRs, I'd like to make it clear why requesters should not file duplicate PRs. While it hasn't been an issue so far, I think we all can see how that can be used to circumvent vetos from committers. Allowing duplicate PRs would thus defeat the purpose of the veto rights in an Apache community.
Feel free to bring the discussion to dev@ if you're still in doubt.
|
thanks for the fix |
* Fix warning * CR comments * Fix windows CI * as per Haojin2 request, moving to op namespace
Description
Fix static function in header. Static functions should not be in headers.
I did a quick search and I didn't find that there are other cases of this problem.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.