Kanvi/remove additional attr check #150
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Make ngraph_backend mandatory, so the user using ngraph-optimizer can only specify backend using the rewriter config
Make all other config attributes optional Remove backend_config files Fix tests accordingly
sayantan-nervana
suggested changes
Jul 23, 2019
Co-Authored-By: Sayantan Sarkar <sayantan.sarkar@intel.com>
…om/tensorflow/ngraph-bridge into kanvi/remove_additional_attr_check
shresthamalik
approved these changes
Jul 24, 2019
Contributor
shresthamalik
left a comment
shresthamalik
left a comment
There was a problem hiding this comment.
Left few minor comments. Otherwise LGTM 👍
| // For e.g. _ngraph_ice_cores --> ice_cores | ||
| if (itx.first.find("_ngraph_") != std::string::npos) { | ||
| // find the index of the second '_' | ||
| int delimiter_index = itx.first.find('_', 6); |
Contributor
There was a problem hiding this comment.
minor you dont need the delimiter
itx.first.substr(strlen("ngraph")) should give you the attribute name, and you get rid of the hardcoded 6
kanvi-nervana
commented
Jul 24, 2019
| if (i.first == "device_id") { | ||
| config_map["ngraph_" + i.first] = i.second.s(); | ||
| } else { | ||
| config_map["_ngraph_" + i.first] = i.second.s(); |
Contributor
There was a problem hiding this comment.
config_map[(i.first == "device_id" ? "" : "_") + "ngraph_" + i.first] = i.second.s();
sayantan-nervana
approved these changes
Jul 25, 2019
Contributor
sayantan-nervana
left a comment
sayantan-nervana
left a comment
There was a problem hiding this comment.
LGTM. waiting for final review comments to be incorporated and CI.
…om/tensorflow/ngraph-bridge into kanvi/remove_additional_attr_check
gopoka
pushed a commit
that referenced
this pull request
Oct 28, 2019
Grappler to use rewriter config only. backend manager and env variable based backend setting is going to be deprecated for grappler. ngraph_backend & device_id are 2 mandatory fields, other extra params are optional. update_config has been modified to accept these new params
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make ngraph_backend & device_id as a mandatory attribute to be supplied using rewriter_config.
Make all other backend related attributes optional.
All attributes thhat will have 'ngraph' appended will be passed to the specified backend and it is upto the backend to do the checks for the one's it expects.
Remove ngraph_backend_config class as it is no longer required. We can now use common Split and Join functions for all backends.
When using the grappler, rewriter_config is the only way to specify backend, env variable and backend manager will not work. Hence, removed code for the same from ngraph_optimizer.cc
Fix tests.