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

[CI] Add CPU C++ test stage#13338

Merged
marcoabreu merged 1 commit intoapache:masterfrom
larroy:cpp_ut
Jan 8, 2019
Merged

[CI] Add CPU C++ test stage#13338
marcoabreu merged 1 commit intoapache:masterfrom
larroy:cpp_ut

Conversation

@larroy
Copy link
Copy Markdown
Contributor

@larroy larroy commented Nov 20, 2018 •
edited
Loading

Description

We were not running C++ test compiled without GPU and without MKL, which had test failures, such as #13409

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • 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://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

lebeg
lebeg reviewed Nov 20, 2018
View reviewed changes
Comment thread src/operator/nn/activation-inl.h Outdated
#if (MXNET_USE_CUDNN == 1 || MXNET_USE_MKLDNN == 1)
bool relu = param.act_type == activation::kReLU;
const bool relu = param.act_type == activation::kReLU;
CHECK_EQ(inputs.size(), relu ? 2U : 3U);
Copy link
Copy Markdown
Contributor

@lebeg lebeg Nov 20, 2018

Choose a reason for hiding this comment

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

This code was added in #11827 . Maybe @samskalicky can help here and say whether it's needed

Copy link
Copy Markdown
Contributor Author

@larroy larroy Nov 27, 2018

Choose a reason for hiding this comment

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

I fixed all this stuff in #13409 just keeping the CI pipeline here until @marcoabreu green lights.

@stu1130
Copy link
Copy Markdown
Contributor

stu1130 commented Nov 20, 2018

@mxnet-label-bot add [pr-awaiting-review]
Thanks for your contribution! @larroy

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Nov 20, 2018
@marcoabreu
Copy link
Copy Markdown
Contributor

marcoabreu commented Nov 20, 2018

Please wait with merging this PR until the Jenkinsfile refactoring has been completed

@larroy larroy changed the title Add CPU C++ test stage and fix CPU ACTIVATION_PERF.ExecuteBidirectional [Dont merge] Add CPU C++ test stage and fix CPU ACTIVATION_PERF.ExecuteBidirectional Nov 21, 2018
@vandanavk
Copy link
Copy Markdown
Contributor

vandanavk commented Nov 27, 2018

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

@marcoabreu marcoabreu added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-review PR is waiting for code review labels Nov 27, 2018
@larroy larroy changed the title [Dont merge] Add CPU C++ test stage and fix CPU ACTIVATION_PERF.ExecuteBidirectional [Dont merge][Dont review] Add CPU C++ test stage and fix CPU ACTIVATION_PERF.ExecuteBidirectional Nov 29, 2018
@roywei
Copy link
Copy Markdown
Member

roywei commented Dec 11, 2018

@larroy ping, is this still needed? thanks!

@larroy larroy changed the title [Dont merge][Dont review] Add CPU C++ test stage and fix CPU ACTIVATION_PERF.ExecuteBidirectional Add CPU C++ test stage Dec 11, 2018
@larroy larroy changed the title Add CPU C++ test stage [CI] Add CPU C++ test stage Dec 11, 2018
@larroy
Copy link
Copy Markdown
Contributor Author

larroy commented Dec 11, 2018

@roywei is needed. @marcoabreu pls review.

@larroy larroy changed the title [CI] Add CPU C++ test stage [CI] Add CPU C++ test stage, re-enable kvstore tests Dec 11, 2018
@larroy larroy force-pushed the cpp_ut branch 3 times, most recently from 19a730e to 0518e16 Compare December 18, 2018 11:38
@larroy larroy force-pushed the cpp_ut branch 3 times, most recently from 0481230 to 37291f5 Compare January 3, 2019 13:41
@larroy
Copy link
Copy Markdown
Contributor Author

larroy commented Jan 3, 2019

Ready to merge @marcoabreu

@larroy larroy changed the title [CI] Add CPU C++ test stage, re-enable kvstore tests [CI] Add CPU C++ test stage Jan 3, 2019
jlcontreras
jlcontreras approved these changes Jan 3, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jlcontreras jlcontreras left a comment

Choose a reason for hiding this comment

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

LGTM

lebeg
lebeg approved these changes Jan 4, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Are the mshadow submodule changes needed?

@larroy
Copy link
Copy Markdown
Contributor Author

larroy commented Jan 4, 2019 •
edited
Loading

@lebeg no, fixed.

lebeg
lebeg approved these changes Jan 8, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Looks good to me

@marcoabreu marcoabreu merged commit 4d65790 into apache:master Jan 8, 2019
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@larroy
Add CPU test coverage and refine cmake builds (apache#13338)
479543f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@anirudh2290 anirudh2290 Awaiting requested review from anirudh2290

3 more reviewers

@lebeg lebeg lebeg approved these changes

@jlcontreras jlcontreras jlcontreras approved these changes

@marcoabreu marcoabreu marcoabreu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-awaiting-testing PR is reviewed and waiting CI build and test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@larroy @stu1130 @marcoabreu @vandanavk @roywei @lebeg @jlcontreras