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

add extra header file to export #13795

Merged
merged 10 commits into from
Jan 9, 2019

Conversation

apeforest
Copy link
Contributor

Description

Export this header file for Horovod to throw and catch exception elegantly in MXNet.
Horovod PR: horovod/horovod#542

Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

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

LGTM. Left a nit comment.

@@ -26,9 +26,9 @@
#include <dmlc/base.h>
#include <mxnet/base.h>
#include <mxnet/ndarray.h>
#include <mxnet/c_api_common.h>
Copy link
Member

Choose a reason for hiding this comment

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

QQ: do we have guideline regarding the order of putting include files? Shall we follow alphabetic order of the filename? If so, please move this include to the right place here and all other places in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't find a guideline for order. I was following the order of full path header followed by relative path header

#ifndef MXNET_C_API_C_API_COMMON_H_
#define MXNET_C_API_C_API_COMMON_H_
#ifndef MXNET_C_API_COMMON_H_
#define MXNET_C_API_COMMON_H_

#include <dmlc/base.h>
#include <dmlc/logging.h>
Copy link
Member

Choose a reason for hiding this comment

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

Do you need all these functions in the public header files, including MXAPIThreadLocalEntry, CopyAttr, etc?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mind pointing out how horovod will use these APIs?

Copy link
Contributor Author

@apeforest apeforest Jan 8, 2019

Choose a reason for hiding this comment

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

@eric-haibin-lin I agree MXAPIThreadLocalEntry, CopyAtt are not needed in error handling, nor should we make them visible externally. I don't know the historical reason of adding them to this header file in the first place. My change was following the rationale of making least change to existing structure.

I could extract the error handling functions from c_api_common.h into c_api_error.h and only export c_api_error.h to include. Does that sound like a better approach to you?

Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

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

I'm concerned about what gets exposed publicly due to this change. I think it's necessary to list the APIs that is needed for horovod integration (and discussing briefly what API such use cases require), and then expose only the necessary subset.

@apeforest
Copy link
Contributor Author

@eric-haibin-lin @szha The PR that requires this change is ctcyang/horovod#19. The reason we need this header is that we are using check_call in Horovod to check the return status here. We need a way to catch the exception thrown from MXNet C API like this. However, the API_BEGIN() and API_END() are defined in c_api_common.h which is the one I need to export.

@eric-haibin-lin
Copy link
Member

eric-haibin-lin commented Jan 8, 2019

I agree that MXNet should expose c apis necessary for error handling. Exposing API_BEGIN/API_END/API_END_HANDLE_ERROR sounds reasonable. We probably should prepend "MX_" in these macro. WDYT @apeforest @szha

@szha
Copy link
Member

szha commented Jan 8, 2019

Let's limit the exposure to only what's required.

@apeforest
Copy link
Contributor Author

@eric-haibin-lin @szha Per your suggestion, I have stripped out the error handling APIs into a separate header file and export that one only. The remaining code in MXNet modules remain unchanged. Please review this change again at your earliest convenience. Thanks a lot!

Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

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

LGTM. just a nit comment

src/c_api/c_api_common.h Outdated Show resolved Hide resolved
Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

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

LGTM.

#include <dmlc/base.h>
#include <dmlc/logging.h>
#include <mxnet/c_api.h>
#include <mxnet/base.h>
Copy link
Member

Choose a reason for hiding this comment

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

are all the headers needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. removed unnecessary headers

@apeforest apeforest changed the title add extra header file to include add extra header file to export Jan 9, 2019
@eric-haibin-lin eric-haibin-lin merged commit 44c21bd into apache:master Jan 9, 2019
yuxihu pushed a commit to yuxihu/incubator-mxnet that referenced this pull request Jan 9, 2019
* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
apeforest added a commit to apeforest/incubator-mxnet that referenced this pull request Jan 11, 2019
* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
yuxihu pushed a commit to yuxihu/incubator-mxnet that referenced this pull request Jan 14, 2019
* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
eric-haibin-lin pushed a commit that referenced this pull request Jan 16, 2019
* Get the correct include path in pip package (#13452)

* add find_include_path API

* address reviewer comment

* change return type from list to string

* add unit test

* address reviewer comment

* address reviewer comment

* address reviewer comment

* address reviewer comment

* fix include path problem in pip package

* add comment

* fix lint error

* address reviewer comment

* address reviewer comment

* Add extra header file to export for error checking (#13795)

* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
lanking520 pushed a commit to lanking520/incubator-mxnet that referenced this pull request Feb 18, 2019
* Get the correct include path in pip package (apache#13452)

* add find_include_path API

* address reviewer comment

* change return type from list to string

* add unit test

* address reviewer comment

* address reviewer comment

* address reviewer comment

* address reviewer comment

* fix include path problem in pip package

* add comment

* fix lint error

* address reviewer comment

* address reviewer comment

* Add extra header file to export for error checking (apache#13795)

* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
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.

4 participants