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

add signal handler for fpe, bus error #18956

Merged
merged 4 commits into from
Aug 20, 2020
Merged

add signal handler for fpe, bus error #18956

merged 4 commits into from
Aug 20, 2020

Conversation

szha
Copy link
Member

@szha szha commented Aug 18, 2020

Description

add signal handler for fpe, bus error

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 https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the best of my 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

@mxnet-bot
Copy link

Hey @szha , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [miscellaneous, centos-cpu, clang, centos-gpu, sanity, edge, website, windows-cpu, unix-gpu, unix-cpu, windows-gpu]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@szha szha force-pushed the sigfpe branch 2 times, most recently from 3aabb13 to 75617b8 Compare August 19, 2020 20:15
@szha szha marked this pull request as ready for review August 20, 2020 00:58
@szha szha requested a review from eric-haibin-lin as a code owner August 20, 2020 00:58
samskalicky
samskalicky reviewed Aug 20, 2020
View reviewed changes
include/mxnet/runtime/c_runtime_api.h
@@ -177,4 +178,43 @@ MXNET_DLL int MXNetObjectTypeKey2Index(const char* type_key, unsigned* out_tinde
#ifdef __cplusplus
} // extern "C"
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we copy over the comment too?

Copy link
Contributor

@samskalicky samskalicky Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

thanks, only one nit: should it be MX_API_BEGIN instead of API_BEGIN, etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/apache/incubator-mxnet/blob/master/src/c_api/c_api_common.h#L45-L55

Copy link
Contributor

Choose a reason for hiding this comment

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

is this useful or should we get rid of this indirection?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, removed. thanks for bringing this up

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, there's external dependency on this ... https://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Funix-gpu/detail/PR-18956/7/pipeline
I will revert this change for now.

samskalicky
samskalicky approved these changes Aug 20, 2020
View reviewed changes
Copy link
Contributor

@samskalicky samskalicky left a comment

Choose a reason for hiding this comment

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

Thanks @szha this will be super helpful to debug issues!

@szha szha merged commit bbc39fa into apache:master Aug 20, 2020
@xidulu xidulu mentioned this pull request Aug 20, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@samskalicky samskalicky samskalicky approved these changes

@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin eric-haibin-lin is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@szha @mxnet-bot @samskalicky