Skip to content
This repository was archived by the owner on Jan 3, 2023. It is now read-only.

Sarkars/flib ngenc compute#529

Open
sayantan-nervana wants to merge 20 commits intomasterfrom
sarkars/flib_ngenc_compute
Open

Sarkars/flib ngenc compute#529
sayantan-nervana wants to merge 20 commits intomasterfrom
sarkars/flib_ngenc_compute

Conversation

@sayantan-nervana
Copy link
Contributor

No description provided.

GraphDef* NGraphClusterManager::GetClusterGraph(int idx) {
std::lock_guard<std::mutex> guard(s_cluster_graphs_mutex);
return s_cluster_graphs[idx];
return idx < s_cluster_graphs.size() ? s_cluster_graphs[idx] : nullptr;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need this change since now we might have nothing stored in NGraphClusterManager, so need to safely avoid accessing out-of-bounds requests

#include <utility>

#include "tensorflow/core/common_runtime/dma_helper.h"
#include "tensorflow/core/common_runtime/function.h"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included for FunctionBody

}
}
library {
function {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The library of graphdefs

}
node {
name: "Sigmoid"
op: "IdentityN"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IdentityN due to Kanvi's change

# Comparing with expected value
assert np.isclose(res1, exp).all()

@pytest.mark.skip(reason="Not passing through grappler")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kanvi-nervana : I think this is a minimal repro of your squeeze-net issue

@avijit-nervana avijit-nervana added the Release Candidate PRs needed for the next release label Apr 18, 2019
from __future__ import division
from __future__ import print_function

import pytest, pdb
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove pdb

os.environ['NGRAPH_TF_DISABLE_DEASSIGN_CLUSTERS'] = '1'
ngraph_bridge.enable()
with tf.Session(config=config) as sess:
with tf.Session(graph=graph, config=config) as sess:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this change? Just Curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we write python code like a = tf.constant(...); b = tf.constant(...); c = a+b, this underlying graph is added to the default graph. When we read it from a pbtxt (like in this case), the graph is not added to default graph. So was passing the graph along to common.py to use during session construction.

But found a way to set the graph read from pbtxt as default graph (as_default)

const FunctionLibraryDefinition flib =
*ctx->function_library()->GetFunctionLibraryDefinition();
const FunctionDef* fdef =
flib.Find("Enc_" + to_string(m_ngraph_cluster) + "_native_segment");
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 use the node name (eg. ngraph_cluster_251) instead of this "Enc_" + to_string(m_ngraph_cluster)" ?

std::getenv("NGRAPH_TF_DISABLE") != nullptr) {
std::getenv("NGRAPH_TF_DISABLE") != nullptr ||
IsProcessedByNgraphPass(&graph)) {
NGRAPH_VLOG(0) << "NGTF_OPTIMIZER: Ngraph is disabled ";
Copy link
Contributor

Choose a reason for hiding this comment

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

Include the or condition in the error message.

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.

Copy link
Contributor

@sindhu-nervana sindhu-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

@sindhu-nervana sindhu-nervana added the Fully Reviewed All reviewers have approved label Apr 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Fully Reviewed All reviewers have approved Release Candidate PRs needed for the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants