ProcessInfo: save atomically #104
Conversation
| delete=False, | ||
| ) as tmp: | ||
| json.dump(self.asdict(), tmp) | ||
| os.replace(tmp.name, filename) |
There was a problem hiding this comment.
How likely is it that this file gets overwritten? Is it only written once?
In Windows, os.replace may fail with PermissionError, if there are open file handles to destination, so for making it robust, we may need to retry.
There was a problem hiding this comment.
Very often, in manager.py we will update the process info via __setitem__ if there are some status updates on queue tasks.
There was a problem hiding this comment.
@karajan1001, can you show me where we overwrite this? And when? And what do you mean by "very often", like how frequently that happens?
Apologies for asking too many questions, I'm still trying to understand the codebase. :)
There was a problem hiding this comment.
Although os.replace() can be fixed separately, we need a timeout based on how much load we are putting in.
There was a problem hiding this comment.
OK, I'm sorry looking into it I found that I had mixed ProcessInfo with ExecutorInfo, here the ProcessInfo only dumps in
dvc-task/src/dvc_task/proc/process.py
Line 203 in 59caee6
Codecov ReportBase: 78.18% // Head: 78.39% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 78.18% 78.39% +0.21%
==========================================
Files 21 21
Lines 935 935
Branches 149 148 -1
==========================================
+ Hits 731 733 +2
+ Misses 176 175 -1
+ Partials 28 27 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -50,12 +51,17 @@ def asdict(self) -> Dict[str, Any]: | |||
|
|
|||
| def dump(self, filename: str) -> None: | |||
There was a problem hiding this comment.
In load() above, do we need to serialize loading with lock/unlock? Are there any side-effects from load()? Is it possible to get rid of it?
There was a problem hiding this comment.
The only problem is that we might do some updates on this file. If the replacement progress is also atomic we can get rid of it.
|
I ran it 100 Windows jobs in CI, 9 failed (8 of them were See https://github.com/iterative/dvc/actions/runs/3854804740 |
|
ping @karajan1001 @pmrowla. |
Trying to fix iterative/dvc#8570 (comment).