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

Bypass ThreadedEngine in test_convolution_multiple_streams. #14338

Merged
merged 1 commit into from
Mar 6, 2019
Merged

Bypass ThreadedEngine in test_convolution_multiple_streams. #14338

merged 1 commit into from
Mar 6, 2019

Conversation

DickJC123
Copy link
Contributor

Description

Per discussion with @szha, this PR stabilizes the CI by eliminating errors in test_operator_gpu.py:test_convolution_multiple_streams that began with the merging of the subsequent PR #14223. The segfaults and python double-free aborts currently seen in this test only appear when run with MXNET_ENGINE_TYPE=ThreadedEngine (which uses the ThreadedEnginePooled class). It is hoped that @arcadiaphy will submit a follow-up PR to enable his improvements to run under the ThreadedEngine.

The test being modified was introduced by #14006.

This a the first step to resolving #14329.

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)
  • [X ] Changes are complete (i.e. I finished coding on this PR)
  • [X ] 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)
  • [X ] 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

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

  • If this change is a backward incompatible change, why must this change be made.
  • Interesting edge cases to note here

@DickJC123 DickJC123 mentioned this pull request Mar 6, 2019
Open
@vandanavk
Copy link
Contributor

@mxnet-label-bot add [Flaky, Test, pr-awaiting-review]

@marcoabreu marcoabreu added Flaky pr-awaiting-review PR is waiting for code review Test labels Mar 6, 2019
wkcn
wkcn approved these changes Mar 6, 2019
View reviewed changes
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.

Thanks for your quick fix!
LGTM : )

@DickJC123
Copy link
Contributor Author

@szha @eric-haibin-lin This is ready for merging IMHO.

@wkcn wkcn merged commit d6eafca into apache:master Mar 6, 2019
@wkcn
Copy link
Member

wkcn commented Mar 6, 2019

Merged. Thank you！

@arcadiaphy arcadiaphy mentioned this pull request Mar 10, 2019
Merged
7 tasks
arcadiaphy added a commit to arcadiaphy/incubator-mxnet that referenced this pull request Mar 10, 2019
@arcadiaphy
Revert "Bypass ThreadedEngine in test_operator_gpu.py:test_convolutio…
6cb6255 
…n_multiple_streams. (apache#14338)"

This reverts commit d6eafca.
wkcn pushed a commit that referenced this pull request Mar 11, 2019
@arcadiaphy @wkcn
fix engine crash in shutdown phase (#14382)
4f5cba5 
* fix engine crash in shutdown phase

* fix lint

* Revert "Bypass ThreadedEngine in test_operator_gpu.py:test_convolution_multiple_streams. (#14338)"

This reverts commit d6eafca.
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@DickJC123 @vdantu
Bypass ThreadedEngine in test_operator_gpu.py:test_convolution_multip…
3d2db63 
…le_streams. (apache#14338)
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@arcadiaphy @vdantu
fix engine crash in shutdown phase (apache#14382)
707c6ae 
* fix engine crash in shutdown phase

* fix lint

* Revert "Bypass ThreadedEngine in test_operator_gpu.py:test_convolution_multiple_streams. (apache#14338)"

This reverts commit d6eafca.
nswamy pushed a commit that referenced this pull request Apr 5, 2019
@DickJC123 @nswamy
Bypass ThreadedEngine in test_operator_gpu.py:test_convolution_multip…
ae2dda2 
…le_streams. (#14338)
nswamy pushed a commit that referenced this pull request Apr 5, 2019
@arcadiaphy @nswamy
fix engine crash in shutdown phase (#14382)
d5bf85b 
* fix engine crash in shutdown phase

* fix lint

* Revert "Bypass ThreadedEngine in test_operator_gpu.py:test_convolution_multiple_streams. (#14338)"

This reverts commit d6eafca.
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@DickJC123
Bypass ThreadedEngine in test_operator_gpu.py:test_convolution_multip…
212b30b 
…le_streams. (apache#14338)
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@arcadiaphy
fix engine crash in shutdown phase (apache#14382)
ebb12c7 
* fix engine crash in shutdown phase

* fix lint

* Revert "Bypass ThreadedEngine in test_operator_gpu.py:test_convolution_multiple_streams. (apache#14338)"

This reverts commit d6eafca.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wkcn wkcn wkcn approved these changes

Assignees
No one assigned
Labels
Flaky pr-awaiting-review PR is waiting for code review Test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@DickJC123 @vandanavk @wkcn @marcoabreu