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.

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

Fix create_mnbn_model() bug #7718

Merged
merged 10 commits into from
Jul 29, 2019
Merged

Fix create_mnbn_model() bug #7718

merged 10 commits into from
Jul 29, 2019

Conversation

shu65
Copy link
Member

@shu65 shu65 commented Jul 6, 2019 •
edited
Loading

This PR solves #7717

ToDo:

  • fix a bug
  • add unittests

@shu65 shu65 added the ChainerMN Related to ChainerMN. label Jul 6, 2019
@shu65 shu65 added cat:bug Bug report or fix. to-be-backported Pull request that should be backported. labels Jul 6, 2019
@shu65 shu65 changed the title [WIP] Fix create_mnbn_model() bug Fix create_mnbn_model() bug Jul 6, 2019
toslunar
toslunar reviewed Jul 7, 2019
View reviewed changes
Copy link
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

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

LGTM

@ktns
Copy link
Contributor

ktns commented Jul 8, 2019

Due to #6053, it might not be sufficient to reproduce chainer.Sequential._layers because chainer.Sequential._children is dependent in what order links and functions are added. Wouldn't it better to reproduce _children also?

@ktns
Copy link
Contributor

ktns commented Jul 8, 2019 •
edited
Loading

I've found that chainer.Sequential.copy() does not reproduce _children either. Maybe this PR should be left as is and with a PR for #6053 should be compatible with this implementation.

@shu65
Copy link
Member Author

shu65 commented Jul 8, 2019

Thank you for the comments.
Does it mean that we should use this version 2623576 instead of 3d5d943 ?

@ktns
Copy link
Contributor

ktns commented Jul 10, 2019

I think either will do. But I prefer 2623576 because it is similar to chainer.Sequential.copy().

@shu65 shu65 requested a review from kuenishi July 25, 2019 05:46
@kuenishi
Copy link
Member

pfnci, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 3d5d943, target branch master) failed with status FAILURE.

@kuenishi
Copy link
Member

Test failure in Jenskin does not seem related. The code looks good to me. Testing multinode now...

@shu65
Copy link
Member Author

shu65 commented Jul 29, 2019

pfnci, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 9a209ab, target branch master) succeeded!

@kuenishi kuenishi merged commit c8bdfd1 into chainer:master Jul 29, 2019
@chainer-ci
Copy link
Member

@kuenishi This pull-request is marked as to-be-backported, but the corresponding backport PR could not be found. Could you check?

@kuenishi kuenishi mentioned this pull request Aug 1, 2019
Merged
kuenishi added a commit to kuenishi/chainer that referenced this pull request Aug 1, 2019
@kuenishi
Merge pull request chainer#7718 from shu65/fix_mnbn_bug
ffc84b4 
Fix create_mnbn_model() bug
@kmaehashi kmaehashi added this to the v7.0.0b3 milestone Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toslunar toslunar toslunar left review comments

@kuenishi kuenishi Awaiting requested review from kuenishi

Assignees
No one assigned
Labels
cat:bug Bug report or fix. ChainerMN Related to ChainerMN. to-be-backported Pull request that should be backported.
Projects
None yet
Milestone
v7.0.0b3
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@shu65 @ktns @kuenishi @chainer-ci @toslunar @kmaehashi