Fix-ish hyperopt leak - #2511
Conversation
b7cfb36 to
359c274
Compare
|
Thanks @scarlehoff for this. I will have a look ASAP before the end of this week. |
|
Thanks! Turns out this is going to be important because we need to redo the hyperopt with MHOU (which certainly requires be very light on memory :__ ) |
|
The promised benchmark, running in A first run small run (so take the times with a grain of salt, since due to the computer and the size of the runcard, the overhead is irrelevant, the memory was certainly the cause of the difference) A second run, with a greater number of datasets and replicas |
Radonirinaunimi
left a comment
Radonirinaunimi
left a comment
There was a problem hiding this comment.
This looks generally fine to me. I only have very minor comments/suggestions below and they are mainly pedantic aspects. What I think are really missing though are unit tests, especially for _run_trial_in_subprocess.
We could for example have simple tests that:
- the actual forks really have different PID than the parent
- in case of no forks/macos it falls back directly to the current process
- the timeout is enforced
- ...
| sender.close() | ||
| try: | ||
| # Wait for the child to finish running | ||
| success, ret = receiver.recv() |
There was a problem hiding this comment.
In principle, for some god knows what reasons, a child process could deadlock, causing the job to hang (with no way of telling if it's stuck or slow) (?). Should we perhaps add a timeout to avoid this?
There was a problem hiding this comment.
You are right, but at the same time we don't know beforehand we don't know how long a particular architecture should take.
I think this is a problem that will be solved by the cluster admin's email saying "your job has been blocking a GPU at 0% usage for the last 24 hours" :P
(more seriously, I wouldn't know how to solve this problem effectively)
2 participants
I haven't been able to actually fix the memory leak, so I took a different route. Instead of fixing the leak, now each hyperopt trial is run as forked process that dies once the trial finishes.
With this we can run hyperopt for as many trials we want without hitting OOM. I'm bypassing it when running hyperopt in parallel because in that case one can just leave the database running and run many small jobs and then it would be better not to have the overhead of these changes.
Not very beautiful but functional*. I've left also my previous attempts (in
internal_state.py) at cleaning up tensorflow which were not enough but they did clean up things that were dangling so I think it might be worth having them.(tested by submitting many jobs at BSC and none of them failed due to an OOM, which was the case before! I'll add an actual benchmark)