Skip to content
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

Inference example and inference-framework unit-test for NMT model #8314

Closed

Conversation

sidgoyal78
Copy link
Contributor

@sidgoyal78 sidgoyal78 commented Feb 9, 2018

Addresses second part of #7999

Background
The fluid example of NMT model has 2 parts:
train_main() and decode_main(). As discussed with @jacquesqiao , the current implementation of the book chapter (as of Feb 8, 2018), is a bit disjoint. More specifically, the decode_main should ideally use the parameters post training (that is, after calling train_main()). However, currently the train_main() demonstrates the working example of dynamic RNN training, and current decode_main() uses a random initialization of the weights in the encoder-decoder and demonstrates correctness of beam search. Hence, both of the modules are pretty independent.

Current progress (updated)
In order to add inference example for the "inference project", one option is to add "infer" methods for each of the two train_main() and decode_main(). This is what is done in this PR.

  • For the infer method for train_main, we have to provide both source as well as target sequence, even though ideally we don't have access to the target (This is the same issue as in PR Add Inference example and unit test for rnn_encoder_decoder #8176). This works.
  • For the infer method for decode_main, we add the function in Python and it works.
  • The C++ unit-tests corresponding to the 2 infer methods have been added. Since, we don't have a CUDA implementation of beam search op yet, the unit-test for decode runs only for CPU.

@Xreki Xreki added the 预测 原名Inference,包含Capi预测问题等 label Feb 9, 2018
@sidgoyal78 sidgoyal78 self-assigned this Feb 9, 2018
@sidgoyal78 sidgoyal78 added this to Basic Usage (DOING) in Inference Framework Feb 9, 2018

#include <gtest/gtest.h>
#include "gflags/gflags.h"
#include "test_helper.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use full path name with #include. Here it should be

#include "paddle/fluid/inference/tests/book/test_helper.h"

generic.cmake should fail if it is not full-path inclusion; I will figure out why it doesn't.

Copy link
Contributor

@kexinzhao kexinzhao left a comment

Choose a reason for hiding this comment

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

LGTM!


#ifdef PADDLE_WITH_CUDA
LOG(INFO) << "Beam search isn't supported on gpu yet";
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please delete code which are no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks.

@@ -29,7 +29,7 @@
max_length = 8
topk_size = 50
trg_dic_size = 10000
beam_size = 2
beam_size = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why beam_size =1 is better than 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I just changed it for debugging purposes. Thanks for pointing it out, will fix.

set_tests_properties(test_inference_machine_translation_train
PROPERTIES DEPENDS test_machine_translation)
set_tests_properties(test_inference_machine_translation_decode
PROPERTIES DEPENDS test_machine_translation)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to use inference_test here, and try to simplify the unittest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't get both the tests to be expressed with the inference_test function. So I added explicit statements. Do you suggest modifying the inference_test function to accommodate this?


// Setup input sequence of 5 words
paddle::framework::LoDTensor input_sequence;
std::vector<int64_t> inp_data = {100, 50, 8, 94, 122};
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this data have a specific meaning? Or can we use random data?

Copy link
Contributor Author

@sidgoyal78 sidgoyal78 Mar 1, 2018

Choose a reason for hiding this comment

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

This is a random sequence of 5 words (The ids need to be within a range). I will make it clearer.

@sidgoyal78
Copy link
Contributor Author

Since there is an issue with the test_rnn_encoder_decoder, (https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/fluid/inference/tests/book/CMakeLists.txt), and this unit tests in the PR also has a similar test. I think maybe it is better to not merge the PR yet?

@sidgoyal78
Copy link
Contributor Author

Closing the PR

@sidgoyal78 sidgoyal78 closed this Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
预测 原名Inference,包含Capi预测问题等
Projects
No open projects
Inference Framework
Basic Usage (DOING)
Development

Successfully merging this pull request may close these issues.

None yet

4 participants