Skip to content

Avoid warning in Gateway.__del__#442

Merged
jcrist merged 2 commits into
dask:mainfrom
TomAugspurger:fix/avoid-test-warning
Sep 9, 2021
Merged

Avoid warning in Gateway.__del__#442
jcrist merged 2 commits into
dask:mainfrom
TomAugspurger:fix/avoid-test-warning

Conversation

@TomAugspurger
Copy link
Copy Markdown
Member

This test was emitting a warning, which came from garbage collection on
Gateway objects that failed in __init__.

E               Traceback (most recent call last):
E                 File "/home/taugspurger/src/dask/dask-gateway/dask-gateway/dask_gateway/client.py", line 376, in __del__
E                   not self.asynchronous
E                 File "/home/taugspurger/src/dask/dask-gateway/dask-gateway/dask_gateway/client.py", line 334, in asynchronous
E                   return self._asynchronous
E               AttributeError: 'Gateway' object has no attribute '_asynchronous'

This test was emitting a warning, which came from garbage collection on
Gateway objects that failed in `__init__`.
Copy link
Copy Markdown
Member

@jcrist jcrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Tom!

Comment thread dask-gateway/dask_gateway/client.py Outdated

def __del__(self):
if (
if hasattr(self, "_asynchronous") and (
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case the property changes implementation, should maybe check the referenced name instead of an implementation detail.

Suggested change
if hasattr(self, "_asynchronous") and (
if hasattr(self, "asynchronous") and (

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me check that. I assumed hasattr always returned true for properties, but maybe not if they raise an AttributeError?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK switched to use asynchronous and there's no warning, but I have no idea why it's working. It seems like hasattr does always evaluate true in this case:

In [1]: class Foo:
   ...:     @property
   ...:     def a(self):
   ...:         return self._a
   ...:

In [2]: hasattr(Foo, "a")
Out[2]: True

so I would expect the warning to be back when using the property name. But I guess it doesn't really matter, as long as it works.

Copy link
Copy Markdown
Member

@jcrist jcrist Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're checking on the class above (which will always work), when what you meant to do was check on an instance

>>> hasattr(Foo, "a")
True
>>> hasattr(Foo(), "a")
False

hasattr is basically:

def hasattr(x, name):
    try:
        getattr(x, name)
        return True
    except AttributeError:
        return False

@jcrist
Copy link
Copy Markdown
Member

jcrist commented Sep 9, 2021

Test failure is a known CI issue. Merging, thanks Tom!

@jcrist jcrist merged commit 8569adc into dask:main Sep 9, 2021
@TomAugspurger TomAugspurger deleted the fix/avoid-test-warning branch September 10, 2021 13:38
@consideRatio consideRatio added the bug Something isn't working label Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants