Hostfile: register the removal of an existing key #500
Conversation
madsbk
added
3 - Ready for Review
| @@ -148,6 +148,10 @@ def __init__( | |||
| self.fast = self.host_buffer if memory_limit == 0 else self.host_buffer.fast | |||
|
|
|||
| def __setitem__(self, key, value): | |||
| if key in self.device_buffer: | |||
| # Make sure we register the removal of an existing key | |||
| del self[key] | |||
There was a problem hiding this comment.
I'm not sure I understand this, what is the case when this is necessary? If the key already exists in self.device_buffer but it now refers to a non-CUDA object that's going to be stored in self.host_buffer, is that right? If so, does this actually happen in practice?
There was a problem hiding this comment.
No, in the case of DeviceHostFile and in the current Dask/Distributed implementation, I don't think it is likely to happen in practice.
In ProxifyHostFile it is way more likely to happen and I added the check in DeviceHostFile for robustness.
There was a problem hiding this comment.
I see, thanks for confirming.
| @@ -148,6 +148,10 @@ def __init__( | |||
| self.fast = self.host_buffer if memory_limit == 0 else self.host_buffer.fast | |||
|
|
|||
| def __setitem__(self, key, value): | |||
| if key in self.device_buffer: | |||
| # Make sure we register the removal of an existing key | |||
| del self[key] | |||
There was a problem hiding this comment.
I see, thanks for confirming.
pentschev
added
5 - Ready to Merge
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #500 +/- ##
===============================================
+ Coverage 90.42% 91.62% +1.19%
===============================================
Files 15 18 +3
Lines 1128 1432 +304
===============================================
+ Hits 1020 1312 +292
- Misses 108 120 +12
Continue to review full report at Codecov.
|
|
@gpucibot merge |
This PR makes sure that hostfiles register the removal of an existing key. They didn't do that before when mixing CUDA and non-CUDA objects.