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

Change CUB submodule to track Nvidia CUB project. #13322

Merged
merged 2 commits into from
Mar 31, 2019

Conversation

frankfliu
Copy link
Contributor

@frankfliu frankfliu commented Nov 19, 2018

Description

Current cub submodule is pointing to DLMC's cub repo, the repo doesn't have customization of nvidia's cub project. The historic reason to use DLMC's cub is:

  1. check out of nvidia's cub project takes long time
  2. There was force push in nvidia's cub project broke mxnet submodule link.

Since both above issue has been resolved on Nvidia side, we should move back to Nvidia's cub project to reduce upgrade/merge effort.

The proposal has been discussed on: https://lists.apache.org/thread.html/6c10ab4746ac15b08fb5ac7281abdfdfcd5b534fd3838200941cd2ca@%3Cdev.mxnet.apache.org%3E.

Directly change submodule URL will impact every developer.
"git submodule update" won't work, developer has to use
"git submodule sync" first.

To avoid impact every developer, remove CUB and add a new
"nvidia_cub" to workaround this issue.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http:https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@frankfliu frankfliu requested a review from szha as a code owner November 19, 2018 17:59
@kalyc
Copy link
Contributor

kalyc commented Nov 19, 2018

@mxnet-label-bot add [pr-work-in-progress]

@marcoabreu marcoabreu added the pr-work-in-progress PR is still work in progress label Nov 19, 2018
@kalyc
Copy link
Contributor

kalyc commented Nov 19, 2018

@frankfliu please take a look at the failed CI build and update PR accordingly

@frankfliu frankfliu changed the title [WIP] Change CUB submodule to track Nvidia CUB project. Change CUB submodule to track Nvidia CUB project. Nov 19, 2018
@stu1130
Copy link
Contributor

stu1130 commented Nov 20, 2018

@mxnet-label-bot update [pr-awaiting-review]

@marcoabreu marcoabreu added pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress labels Nov 20, 2018
Copy link
Contributor

@lupesko lupesko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
Thanks for moving off the copy to the Nvidia cub!

@lupesko
Copy link
Contributor

lupesko commented Nov 22, 2018

@frankfliu did you do some sanity perf tests on GPU? we probably should for operators using cub.
Pulling in @anirudh2290 @apeforest and @nswamy to review.

@KellenSunderland
Copy link
Contributor

@frankfliu Thanks for the contribution. Your description indicates why we're using the name '3rdparty/nvidia_cub' but not why we're switching submodules, which in my opinion is the more important piece of information. Can you give some details around why we're switching back to the NVIDIA repo?

If I remember correctly the reason we switched away from using the NVIDIA cub repo was because we had a bunch of force pushes and history re-writes in that repo that were causing us issues. Looking at the current repo it seems like it's in really good shape now. They describe their release process clearly and are making use of release branches. Given they're using release branching, I'd suggest we track a specific stable branch using git 1.8 (and higher) submodule branch feature. I hope this will help to protect us against future force-pushes.

@KellenSunderland
Copy link
Contributor

I've looked into this a little further and I suspect that tracking a branch is still not going to get us any protection against our submodules force pushing. I've opened a question on StackOverflow to see if anyone knows the details around what happens in this case: https://stackoverflow.com/questions/53438716/does-tracking-a-branch-with-a-submodule-protect-you-against-force-pushes

@KellenSunderland
Copy link
Contributor

Looks like adding a branch ref to the submodule isn't necessary / won't alleviate the problem I have in mind. Pending questions for this PR are only

  • What problems does it solve / feature does it provide?
  • What testing has been done?

@KellenSunderland
Copy link
Contributor

@mxnet-label-bot update [pr-awaiting-response]

@marcoabreu marcoabreu added pr-awaiting-response PR is reviewed and waiting for contributor to respond and removed pr-awaiting-review PR is waiting for code review labels Nov 27, 2018
Copy link
Contributor

@lebeg lebeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The submodule is pointing to a release tag v1.8.0 which I think is good.

@nswamy nswamy added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Dec 8, 2018
@lanking520
Copy link
Member

@lebeg Any idea how to conduct a test on this? It seemed to be an improvement on the performance.

@sandeep-krishnamurthy
Copy link
Contributor

@frankfliu - Can you please provide next steps here? and few open questions from Kellen.

@KellenSunderland
Copy link
Contributor

This PR is looking good to me, and I don't want to be a blocker. Consider me approving modulo short testing description. I think my "What problems does it solve / feature does it provide" has been answered well in the edited PR description.

@anirudhacharya
Copy link
Member

@frankfliu ping for update :)

@stu1130
Copy link
Contributor

stu1130 commented Jan 17, 2019

@mxnet-label-bot update [pr-awaiting-response]
@frankfliu could you address the LICENSE conflict? Thanks

@marcoabreu marcoabreu added pr-awaiting-response PR is reviewed and waiting for contributor to respond and removed pr-awaiting-merge Review and CI is complete. Ready to Merge labels Jan 17, 2019
Directly change submodule URL will impact every developer.
"git submodule update" won't work, developer has to use
"git submodule sync" first.
@frankfliu
Copy link
Contributor Author

@mxnet-label-bot add [pr-awaiting-merge] remove [pr-awaiting-response]

@marcoabreu marcoabreu added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Feb 1, 2019
@frankfliu
Copy link
Contributor Author

@mxnet-label-bot remove [pr-awaiting-response]

@marcoabreu marcoabreu removed the pr-awaiting-response PR is reviewed and waiting for contributor to respond label Feb 1, 2019
@ankkhedia
Copy link
Contributor

@KellenSunderland Could you please review/merge this PR after the latest changes?

@vandanavk
Copy link
Contributor

@frankfliu could you have a look at the incomplete CI? may be it needs to be re-triggered

@karan6181
Copy link
Contributor

@frankfliu Could you please look at the incomplete CI? It seems no updates from last couple of weeks. Can you please look into it? Thanks!

Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your contribution!

@wkcn wkcn merged commit b6eac1d into apache:master Mar 31, 2019
@wkcn
Copy link
Member

wkcn commented Mar 31, 2019

The PR has been merged. Thank you!

ZhennanQin pushed a commit to ZhennanQin/incubator-mxnet that referenced this pull request Apr 3, 2019
* Change CUB submodule to track Nvidia CUB project.

Directly change submodule URL will impact every developer.
"git submodule update" won't work, developer has to use
"git submodule sync" first.

* retrigger CI
nswamy pushed a commit that referenced this pull request Apr 5, 2019
* Change CUB submodule to track Nvidia CUB project.

Directly change submodule URL will impact every developer.
"git submodule update" won't work, developer has to use
"git submodule sync" first.

* retrigger CI
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* Change CUB submodule to track Nvidia CUB project.

Directly change submodule URL will impact every developer.
"git submodule update" won't work, developer has to use
"git submodule sync" first.

* retrigger CI
roywei added a commit to roywei/incubator-mxnet that referenced this pull request Jul 17, 2019
@roywei roywei mentioned this pull request Jul 17, 2019
@frankfliu frankfliu deleted the nvidia_cub branch November 16, 2019 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet