-
Notifications
You must be signed in to change notification settings - Fork 63
sindhu/register backends #392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| // Map with all the backends, and what the boolean is_supported should be | ||
| std::map<std::string, bool> backend_map{ | ||
| {"CPU", true}, {"INTERPRETER", true}, {"NOP", false}}; | ||
| std::map<std::string, bool> backend_map{{"CPU", true}, {"INTERPRETER", true}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed NOP case here because we do not have static library available for this backend, so we cannot register NOP with static builds - so this test will fail.
test/test_ngraph_exec.cpp
Outdated
| #include "ngraph_bridge/ngraph_builder.h" | ||
| #include "ngraph_bridge/ngraph_pipelined_tensors.h" | ||
| #include "ngraph_bridge/ngraph_utils.h" | ||
| #include "ngraph_bridge/version.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not required anymore
| return -1; | ||
| } | ||
|
|
||
| // Register backends for static linking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Register backends for static linking | |
| // Register cpu backend for static linking |
| return -1; | ||
| } | ||
|
|
||
| // Register backends for static linking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Register backends for static linking | |
| // Register cpu backend for static linking |
- Enabled --var build to use parallel executor integrating weights-on-device and data pipelining - moved ngraph_var files outside the var build
sayantan-nervana
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| #if defined(NGRAPH_BRIDGE_STATIC_LIB_ENABLE) | ||
| ngraph_register_cpu_backend(); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we create only CPU backend here, hence the ifdef is only for CPU. But suggestions for future is we can query the backend we are trying to set/create and register just that backend if NGRAPH_BRIDGE_STATIC_LIB_ENABLE is enabled
| return -1; | ||
| } | ||
|
|
||
| // Register cpu backend for static linking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to register here, since we have a registration in create backend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it was failing otherwise because we are using set backend here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Revisit this to see if we can remove registering here and register only in BackendManager.
| } | ||
|
|
||
| // Register cpu backend for static linking | ||
| #if defined(NGRAPH_BRIDGE_STATIC_LIB_ENABLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment
Co-Authored-By: Shrestha Malik <shrestha.malik@intel.com>
367d3db to
d74ac48
Compare
shresthamalik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR fixes static build and static linking with recent ngraph core level changes