Skip to content

Conversation

@kanvi-nervana
Copy link
Contributor

No description provided.

Copy link
Contributor

@sayantan-nervana sayantan-nervana 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 "ngraph/descriptor/layout/dense_tensor_layout.hpp"
#include "ngraph/except.hpp"
#include "ngraph/op/convert.hpp"
#include "ngraph/op/select.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to include all these files? Dont see any obvious dependency.

ngraph/op/convert.hpp"
ngraph/op/select.hpp"

namespace testing {

// Test is_supported API
TEST(DummyBackend, Test_is_supported) {
Copy link
Contributor

@shresthamalik shresthamalik Jan 7, 2020

Choose a reason for hiding this comment

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

Test name should not have underscore
Both names must be valid C++ identifiers, and they should not contain any underscores
https://github.com/google/googletest/blob/master/googletest/docs/primer.md

// See the License for the specific language governing permissions and
// limitations under the License.
//*****************************************************************************

Copy link
Contributor

Choose a reason for hiding this comment

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

missing header guard

class ng::runtime::dummy::DummyExecutable : public ng::runtime::Executable {
public:
DummyExecutable(std::shared_ptr<ng::Function> function,
bool enable_performance_collection = false);
Copy link
Contributor

Choose a reason for hiding this comment

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

curious: will we be able to create a backend like we create the other backends? maybe not.
https://github.com/tensorflow/ngraph-bridge/blob/master/ngraph_bridge/ngraph_backend_manager.cc#L62

Copy link
Contributor

@sayantan-nervana sayantan-nervana Jan 7, 2020

Choose a reason for hiding this comment

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

probably not. these need .so. Kanvi's example just creates a backend class/object on the fly

Copy link
Contributor

@shresthamalik shresthamalik left a comment

Choose a reason for hiding this comment

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

LGTM

@sayantan-nervana sayantan-nervana added the ready to merge This PR is the next in the queue. label Jan 7, 2020
@sayantan-nervana sayantan-nervana merged commit 81fbef4 into master Jan 8, 2020
@sayantan-nervana sayantan-nervana deleted the Kanvi/create_dummy_backend branch January 8, 2020 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fully reviewed ready to merge This PR is the next in the queue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants