[Bug] `isinstance(self, Client)` will never be `True`

[eddiebergman]

Found this while looking for where the Client is located.

https://github.com/automl/DEHB/blob/54ce41c4c516e38aefc5944a2b677b95cfa2e05a/dehb/optimizers/de.py#L254

[Neeratyoy]

Does it mean that the client is never destructed in this code base or could be a Dask version-related thing?

[eddiebergman]

Not a dask version related thing, it's basically doing:

class A:

    def __del__(self):
        if isisntance(self, Client):
            ...

Also relying on __del__ for for cleanup is usually a bad practice and can lead to problems if DEHB would be the last thing to be cleaned up.

The correct way, is that Dask Clients can be used as context managers and is likely what you want to have around your hot loop. In the case of a user supplied dask, not sure what you want, might be bad form to automatically shut down their client unless they've specified it somehow.

[Neeratyoy]

Okay thanks, so 2 aspects here as I see:

• There is a bug here and the check should be isinstance(self, self.client):
• Whether we should use __del__ or not

[Bronzila]

I think using the Dask Client as a context manager would definetly be an improvement. However this would involve quite some refactoring and since we are planing to release on Thursday, I would propose to simply fix the bug for now and create a new issue regarding using Dask Clients as context managers. What do you think? @eddiebergman @Neeratyoy

[eddiebergman]

Sounds fine to me, as long as it works generally at the moment, then fire away.

[Neeratyoy]

I second @eddiebergman! Just keep an issue alive to remind us :)