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

Asynchronous snapshot writers #4472

Merged
merged 42 commits into from
Jan 10, 2019
Merged

Asynchronous snapshot writers #4472

merged 42 commits into from
Jan 10, 2019

Conversation

niboshi
Copy link
Member

@niboshi niboshi commented Mar 15, 2018

This PR takes over #3401.
We appreciate the great work of @tyohei on this PR. Thank you very much!

Please refer to #3401 for the objective of this PR and previous discussions.

@niboshi niboshi added the cat:feature Implementation that introduces new interfaces. label Mar 15, 2018
@niboshi
Copy link
Member Author

niboshi commented Mar 15, 2018

Despite the comment I left in the previous discussion, I put writer implementations in a new module chainer.training.extensions.snapshot_writers.
Comments / suggestions are welcome.

@okuta okuta self-assigned this Apr 9, 2018
@niboshi
Copy link
Member Author

niboshi commented Apr 25, 2018

@okuta
We discussed that the simpler writer interface (rather than separate classes) would be preferred.
Do you have any specific suggestion how the user can specify the writing backend?

@okuta
Copy link
Member

okuta commented Apr 26, 2018

OK, I understood and respect your discussion.

But, I have one question. Is Process type writer really necessary?
I am concerned that it takes time to transfer data between processes.
I agree to add if it is a function actually used.

@kmaehashi kmaehashi added the takeover Pull request that has been taken over from an external contributor. label May 28, 2018
@okuta
Copy link
Member

okuta commented May 28, 2018

@niboshi Do you have any comment?

@okuta okuta added this to the v5 milestone Jun 17, 2018
@okuta okuta added the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Jun 26, 2018
@stale
Copy link

stale bot commented Sep 25, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 30 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Not updated for a longer period of time. label Sep 25, 2018
@kmaehashi kmaehashi modified the milestones: v5, v5.0.0 Sep 27, 2018
@stale stale bot removed stale Not updated for a longer period of time. labels Sep 27, 2018
@hvy hvy modified the milestones: v5.0.0, v5.1.0b1 Oct 25, 2018
@niboshi niboshi modified the milestones: v5.1.0, v5.2.0 Dec 3, 2018
@niboshi
Copy link
Member Author

niboshi commented Dec 25, 2018

Resolved conflict (merged snapshot_on_error), and fixed temporary directory handling in the tests.

@okuta
Copy link
Member

okuta commented Dec 26, 2018

jenkins, test this please.

@okuta okuta modified the milestones: v5.2.0, v6.0.0b2 Dec 26, 2018
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 2914c06, target branch master) succeeded!

@niboshi
Copy link
Member Author

niboshi commented Jan 10, 2019

Ready for merge?

@okuta okuta removed the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Jan 10, 2019
@okuta
Copy link
Member

okuta commented Jan 10, 2019

OK.
LGTM!

@okuta okuta merged commit ff238f9 into chainer:master Jan 10, 2019
@niboshi niboshi deleted the new_snapshot branch January 10, 2019 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@okuta okuta okuta left review comments

Assignees

@okuta okuta

Labels
cat:feature Implementation that introduces new interfaces. takeover Pull request that has been taken over from an external contributor.
Projects
None yet
Milestone
v6.0.0b2
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@niboshi @okuta @chainer-ci @kmaehashi @hvy @tyohei