Skip to content

[PH] Port the C2/ONNX loader to using Placeholders. #1783

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 4, 2018
Merged

[PH] Port the C2/ONNX loader to using Placeholders. #1783

merged 1 commit into from
Oct 4, 2018

Conversation

nadavrot
Copy link
Contributor

@nadavrot nadavrot commented Oct 3, 2018

Description: This is a WIP PR. This PR ports the last major piece of code to use Placeholders instead of variables. This PR will be followed by cleanup PRS.
Testing: At one point I was able to run the loader. Some tests are still failing.

Documentation:
#1334

@nadavrot nadavrot force-pushed the port_loader_to_ph branch 5 times, most recently from 2dd86dc to 4ef6be5 Compare October 4, 2018 03:46
@rdzhabarov
Copy link
Contributor

Testing ONNXIFI changes are not trivial (yet), once we are closer to complete state on the PR I'll run those.

rdzhabarov
rdzhabarov reviewed Oct 4, 2018
View reviewed changes
include/glow/Importer/ProtobufLoader.h Outdated
assert(outputVarsByName_.size() == 1);
return outputVarsByName_.begin()->second;
}

/// \returns the Variable for the external output with \p name.
/// \pre outputVarsByName_.find(name) != outputVarsByName_.end()
Variable *getOutputByName(llvm::StringRef name) const;
Storage *getOutputByName(llvm::StringRef name) const;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@nadavrot nadavrot force-pushed the port_loader_to_ph branch 3 times, most recently from 2b708c9 to a7e5159 Compare October 4, 2018 04:49
@bertmaher
Copy link
Contributor

Looking good! And, um, I'm sorry for all the silly conflicts on the ModelLoaders :-/

@nadavrot nadavrot force-pushed the port_loader_to_ph branch 2 times, most recently from 55740ec to f31f4e5 Compare October 4, 2018 05:13
bertmaher
bertmaher reviewed Oct 4, 2018
View reviewed changes
include/glow/Importer/Caffe2ModelLoader.h Outdated
@@ -63,12 +63,12 @@ class Caffe2ModelLoader
/// Loads the caffe2 model that's represented by a network description file,
/// serialized in \p netDescFilename, and weights file, serialized in
/// \p netWeightFilename, and populates the network in \p F.
/// The tensors in \p tensors are stored with the names in the list of names
/// The types in \p types of tensors and a names in the list of names

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

beicy
beicy reviewed Oct 4, 2018
View reviewed changes
lib/ExecutionEngine/ExecutionEngine.cpp Outdated
@@ -79,6 +79,21 @@ void glow::updateVariables(Context &ctx, llvm::ArrayRef<Placeholder *> ph,
}
}

void glow::updateVariablesbyName(Context &ctx, Module *mod,
llvm::ArrayRef<llvm::StringRef> ph,

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@nadavrot nadavrot force-pushed the port_loader_to_ph branch 2 times, most recently from 0068d98 to a9c4f17 Compare October 4, 2018 16:16
jfix71
jfix71 reviewed Oct 4, 2018
View reviewed changes
Copy link
Contributor

@jfix71 jfix71 left a comment

Choose a reason for hiding this comment

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

Nice!

include/glow/Importer/ProtobufLoader.h Outdated
@@ -142,14 +144,14 @@ class ProtobufLoader {
/// classification, this single final output is usually the result of the last
/// softmax or regression layer.
/// \pre outputVarsByName_.size() == 1
Variable *getSingleOutput() {
Placeholder *getSingleOutput() {
assert(outputVarsByName_.size() == 1);
return outputVarsByName_.begin()->second;
}

/// \returns the Variable for the external output with \p name.

This comment was marked as off-topic.

Sign in to view

tools/loader/TextTranslator.cpp Outdated
@@ -329,10 +329,12 @@ int main(int argc, char **argv) {

assert(!emittingBundle() && "Bundle mode has not been tested.");

Variable *outputTokenBeamList = LD.getOutputByName("output_token_beam_list");
Variable *outputScoreBeamList = LD.getOutputByName("output_score_beam_list");
Variable *outputTokenBeamList =

This comment was marked as off-topic.

Sign in to view

include/glow/Importer/ProtobufLoader.h Outdated
@@ -133,7 +135,7 @@ class ProtobufLoader {
/// F. The tensors in \p tensors are stored with the names in the list of

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

lib/Onnxifi/Base.cpp Outdated
@@ -68,7 +68,7 @@ onnxStatus Graph::initGraph(const void *onnxModel, size_t onnxModelSize,
onnxStatus Graph::run() {
// Copy tensors from the input addresses to the Glow tensors.
llvm::SmallVector<Tensor *, 4> tensors;
llvm::SmallVector<Variable *, 4> vars;
llvm::SmallVector<Placeholder *, 4> vars;

This comment was marked as off-topic.

Sign in to view

tools/loader/ImageClassifier.cpp Outdated
// Get the Variable that the final expected Softmax writes into at the end of
// image inference.
Variable *SMVar = LD->getSingleOutput();
Placeholder *SMVar = LD->getSingleOutput();
Tensor *res = ctx.allocate(SMVar);

This comment was marked as off-topic.

Sign in to view

tools/loader/ModelRunner.cpp Outdated
@@ -42,7 +42,8 @@ int main(int argc, char **argv) {
LD.reset(new ONNXModelLoader(loader.getOnnxModelFilename(), {}, {},
*loader.getFunction()));
}
Variable *output = LD->getSingleOutput();
Placeholder *output = LD->getSingleOutput();
auto *res = ctx.allocate(output);

This comment was marked as off-topic.

Sign in to view

tools/loader/TextTranslator.cpp
@@ -313,13 +313,17 @@ int main(int argc, char **argv) {
constexpr char const *inputNames[5] = {"encoder_inputs", "attn_weights",
"prev_hypos_indices", "prev_scores",
"prev_token"};
std::vector<Tensor *> inputTensors = {
&encoderInputs, &attnWeights, &prevHyposIndices, &prevScores, &prevToken};
std::vector<TypeRef> inputTensors = {

This comment was marked as off-topic.

Sign in to view

@nadavrot nadavrot force-pushed the port_loader_to_ph branch 3 times, most recently from 42a65ce to e4f9972 Compare October 4, 2018 17:09
@nadavrot
Copy link
Contributor Author

nadavrot commented Oct 4, 2018

The textual translator now works:

Enter a sentence in English to translate to German: I love pizza .
ich liebe Pizza .

@nadavrot nadavrot changed the title [WIP][PH] Port the C2/ONNX loader to using Placeholders. [PH] Port the C2/ONNX loader to using Placeholders. Oct 4, 2018
@nadavrot nadavrot force-pushed the port_loader_to_ph branch 3 times, most recently from 9c0ba58 to a1276ec Compare October 4, 2018 17:23
rdzhabarov
rdzhabarov approved these changes Oct 4, 2018
View reviewed changes
Copy link
Contributor

@rdzhabarov rdzhabarov left a comment

Choose a reason for hiding this comment

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

LGTM

@nadavrot nadavrot force-pushed the port_loader_to_ph branch 2 times, most recently from bc07dd3 to 3ee7930 Compare October 4, 2018 17:26
@nadavrot nadavrot merged commit 1057d66 into pytorch:master Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfix71 jfix71 jfix71 left review comments

@bertmaher bertmaher bertmaher left review comments

@beicy beicy beicy left review comments

@rdzhabarov rdzhabarov rdzhabarov approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nadavrot @rdzhabarov @bertmaher @jfix71 @beicy @facebook-github-bot