Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Revert "Static alloc for hybridblock" #11318

Merged
merged 1 commit into from Jun 16, 2018
Merged

Conversation

marcoabreu
Copy link
Contributor

Reverts #11313

My veto has been ignored and thus I'm allowed to revert this PR. See https://www.apache.org/foundation/voting.html for reference.

@marcoabreu marcoabreu requested a review from szha as a code owner June 16, 2018 01:28
@tqchen
Copy link
Member

tqchen commented Jun 16, 2018

it seems you have been using veto too often lately, please @marcoabreu we are not being unreasonable here, your concern about testcases have been addressed.

@tqchen tqchen closed this Jun 16, 2018
@marcoabreu marcoabreu reopened this Jun 16, 2018
@marcoabreu marcoabreu merged commit 9b8eb56 into master Jun 16, 2018
@tqchen tqchen deleted the revert-11313-static branch June 16, 2018 01:29
@marcoabreu
Copy link
Contributor Author

My as well as Thomas' concerns have not been addressed.

@tqchen
Copy link
Member

tqchen commented Jun 16, 2018

If you have specific concerns, raise them. Your concern of lack of test coverage has been addressed, and you are abusing your right as committer to revert good changes back...

@tqchen tqchen restored the revert-11313-static branch June 16, 2018 01:36
tqchen added a commit that referenced this pull request Jun 16, 2018
@tqchen tqchen deleted the revert-11313-static branch June 16, 2018 01:47
@marcoabreu
Copy link
Contributor Author

#11171 has not been addressed

Pedros review in #10817 has not been addressed

Thomas comment #10817 (comment) has not been addressed

@ThomasDelteil
Copy link
Contributor

I think I have raised specific concerns (twice) that have gone unanswered, just in case they got lost in the flow, let me write them here again:

  • "good changes", I'm sure they are, but the problem with the previous PR was that it was actually making things slower due to a bug, hopefully now fixed. I think we cannot just rely on blind trust, being data driven is beneficial for everybody. Could we have a runtime of maybe just a MNIST training to show that the speed is actually improved and rejoice accordingly?
  • The new API says that static_shape cannot be used without static_alloc. Can we assert that static_alloc is set if users set static_shape to True to avoid undefined behavior?

Bonus ask:

  • a description of the PR to help reviewers, and users alike to appreciate the changes and guide them through the 1400 lines of code, that have barely any comments.
  • better documentation on the impact of the bulk_size arguments in the hybridize() function

@tqchen
Copy link
Member

tqchen commented Jun 16, 2018

This PR is already fixing the problems and added test coverages. If we just block this, how can we move forward?

@marcoabreu marcoabreu mentioned this pull request Jun 16, 2018
7 tasks
@zheng-da
Copy link
Contributor

@marcoabreu I have been intensively using CachedOp with static memory allocation this week. I also read the code this week. Other than the small bug reported previously, I don't find problems and it works very well with me in my PR. #10451
I observe the memory allocation behavior and it works exactly as I expected.

What are you looking for? If you are looking for test code, my entire PR can serve as test code. If you are looking for review, I have been reading the code for the entire week. I don't think it's reasonable to block other people's work for no reason.

@zheng-da
Copy link
Contributor

@marcoabreu I think you should at least give people who understand the PR the opportunity to review it.

@marcoabreu
Copy link
Contributor Author

Exactly, that's all I'm asking for. Give the community enough time to review the PR and address their concerns before merging it.

piiswrong added a commit to piiswrong/mxnet that referenced this pull request Jun 16, 2018
@ThomasDelteil ThomasDelteil mentioned this pull request Jun 16, 2018
7 tasks
piiswrong added a commit to piiswrong/mxnet that referenced this pull request Jun 18, 2018
piiswrong added a commit to piiswrong/mxnet that referenced this pull request Jun 19, 2018
piiswrong added a commit to piiswrong/mxnet that referenced this pull request Jun 19, 2018
marcoabreu pushed a commit that referenced this pull request Jun 20, 2018
* Revert "Revert "Static alloc for hybridblock (#11313)" (#11318)"

This reverts commit 9b8eb56.

* fix
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
* Revert "Revert "Static alloc for hybridblock (apache#11313)" (apache#11318)"

This reverts commit 9b8eb56.

* fix
XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018
XinYao1994 pushed a commit to XinYao1994/incubator-mxnet that referenced this pull request Aug 29, 2018
* Revert "Revert "Static alloc for hybridblock (apache#11313)" (apache#11318)"

This reverts commit 9b8eb56.

* fix
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants