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

Make word2vec uses parallel.do when CI #7970

Merged
merged 4 commits into from
Jan 31, 2018

Conversation

reyoung
Copy link
Collaborator

@reyoung reyoung commented Jan 30, 2018

This PR is a sub-job of #7872

@reyoung reyoung force-pushed the feature/test_w2v_parallel.do branch from 0c2657c to 0b4ade1 Compare January 30, 2018 06:17
* Polish sum_op support SelectedRows in_place
@reyoung reyoung force-pushed the feature/test_w2v_parallel.do branch from 0b4ade1 to 7c0cc11 Compare January 30, 2018 06:56
@reyoung reyoung changed the title Feature/test w2v parallel.do Make word2vec uses parallel.do when CI Jan 30, 2018
@@ -68,32 +68,59 @@ class SumKernel : public framework::OpKernel<T> {
}
}
} else if (out_var->IsType<framework::SelectedRows>()) {
PADDLE_ENFORCE(!in_place, "SelectedRows not support inplace sum now");
std::unique_ptr<framework::SelectedRows> in0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make SumOp support SelectedRows in place.

exit(0) # if avg cost less than 10.0, we think our code is good.
exit(1)

def main(use_cuda, is_sparse, parallel):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

change previous unittest to function main

for use_cuda in (False, True):
for is_sparse in (False, True):
for parallel in (False, True):
inject_test_method(use_cuda, is_sparse, parallel)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

inject_test_method to W2VTest

if use_cuda and is_sparse and parallel:
fn = __impl__
else:
# skip the other test when on CI server
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Skip tests if the environment varaible FULL_TEST is not True. It will make our CI faster.

with fluid.program_guard(prog, startup_prog):
main(use_cuda=use_cuda, is_sparse=is_sparse, parallel=parallel)

if use_cuda and is_sparse and parallel:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only test use_cuda=True, is_sparse=True, parallel=True in our CI sever.

@reyoung
Copy link
Collaborator Author

reyoung commented Jan 30, 2018

@QiJune Please review the SumOp related code. This PR make sum operator supports sum SelectedRows in place.

@tonyyang-svail Please review the unittests.


out_value->Resize(framework::make_ddim(in_dim_vec));
out_value->Resize(framework::make_ddim(in_dim));
out_value->mutable_data<T>(context.GetPlace());

math::SelectedRowsAddTo<DeviceContext, T> functor;

int64_t offset = 0;
for (int i = 0; i < N; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

If in_place=True, the first one should be skipped.

Copy link
Member

Choose a reason for hiding this comment

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

And the value of offset is also related with the value of in_place

Copy link
Member

Choose a reason for hiding this comment

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

I get the logic of of in-place support for SelectedRows.

Copy link
Member

@QiJune QiJune left a comment

Choose a reason for hiding this comment

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

LGTM!

@reyoung reyoung merged commit 865a714 into PaddlePaddle:develop Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants