Skip to content

ensure atomic writes when saving a file - #200

Merged
jbweston merged 1 commit into
stable-0.8from
safe_saving
Jul 9, 2019
Merged

ensure atomic writes when saving a file#200
jbweston merged 1 commit into
stable-0.8from
safe_saving

Conversation

@basnijholt

@basnijholtbasnijholt commented Jun 20, 2019

Copy link
Copy Markdown
Member

Right now, if a program crashes in the middle of saving, you lose all your data. This ensures that the old file is first moved, then the new file is saved, and only then the old file is removed.

This is not a hypothetical scenario but happens every day for some people that I work with ATM.

@basnijholt
basnijholtforce-pushed the safe_saving branch 2 times, most recently from fc9ba2f to 187e064CompareJune 20, 2019 22:09
@basnijholt
basnijholt changed the base branch from master to stable-0.8June 20, 2019 22:10
@basnijholt
basnijholtforce-pushed the safe_saving branch 2 times, most recently from 2b06b62 to 82a300bCompareJune 20, 2019 22:11
@basnijholt

basnijholt commented Jun 20, 2019

Copy link
Copy Markdown
MemberAuthor

Maybe adding a "." in front of the fname isn't the best name to choose.

@basnijholtbasnijholt changed the title make a backup of the data file before savingWIP: make a backup of the data file before savingJun 20, 2019
Comment threadadaptive/learner/base_learner.py Outdated
@akhmerov

Copy link
Copy Markdown
Contributor

This is not a hypothetical scenario but happens every day for some people that I work with ATM.

Maybe that is also worth investigating as a separate issue? Saving usually shouldn't cause a problem if adaptive is stable.

@akhmerov

Copy link
Copy Markdown
Contributor

The implementation is rather fragile and potentially hitting bad corner cases (overwriting a different file, times being messed up, etc); I think there should be a more systematic way to do an atomic write.

@basnijholt

Copy link
Copy Markdown
MemberAuthor

Maybe that is also worth investigating as a separate issue? Saving usually shouldn't cause a problem if adaptive is stable.

The reason for the crash is not Adaptive, but merely low priority nodes that can be evicted at any time, that suddenly stop.

@basnijholt

Copy link
Copy Markdown
MemberAuthor

The implementation is rather fragile and potentially hitting bad corner cases (overwriting a different file, times being messed up, etc); I think there should be a more systematic way to do an atomic write.

Do you have a suggestion?

@akhmerov

Copy link
Copy Markdown
Contributor

https://github.com/untitaker/python-atomicwrites seems like an option or any other alternatives.

Or we could use all the relevant tricks from it.

@akhmerov

Copy link
Copy Markdown
Contributor

I also have to bring up another approach: we provide a way to get an in-memory data in adaptive and the user uses atomicwrites on their own. Separation of concerns and all.

@basnijholt
basnijholtforce-pushed the safe_saving branch 3 times, most recently from 9a45665 to 015c436CompareJune 20, 2019 23:20
@basnijholt

Copy link
Copy Markdown
MemberAuthor

I also have to bring up another approach: we provide a way to get an in-memory data in adaptive and the user uses atomicwrites on their own. Separation of concerns and all.

There is already _get_data and _set_data that every learner implements. The problem is that you can't use the periodic_saver like this

I do not mind to add the dependency, it's used by 19723 other packages and it is tiny. See the commit I've made. Also, I don't see a reason why people wouldn't want a 'safe' save.

@basnijholt
basnijholtforce-pushed the safe_saving branch 2 times, most recently from 2115f48 to 0cc2fc7CompareJune 20, 2019 23:25
@basnijholtbasnijholt changed the title WIP: make a backup of the data file before savingensure atomic writes when saving a fileJun 21, 2019
Right now, if a program crashes in the middle of saving, you lose
all your data.
Right now, if a program crashes in the middle of saving, you lose
all your data. This ensures that the old file is first moved, then
the new file is saved, and only then the old file is removed.
@jbweston
jbweston merged commit c821c49 into stable-0.8Jul 9, 2019
@basnijholt
basnijholt deleted the safe_saving branch July 29, 2019 10:15
@basnijholtbasnijholt mentioned this pull request Aug 31, 2019
Sign up for freeto 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.

3 participants

@basnijholt@akhmerov@jbweston