Skip to content

Use __class__ when comparing meta data#6981

Merged
jacobtomlinson merged 2 commits intodask:masterfrom
madsbk:remove_hard_type_check
Dec 18, 2020
Merged

Use __class__ when comparing meta data#6981
jacobtomlinson merged 2 commits intodask:masterfrom
madsbk:remove_hard_type_check

Conversation

@madsbk
Copy link
Copy Markdown
Contributor

@madsbk madsbk commented Dec 16, 2020

In order to support object wrappers/proxies like rapidsai/dask-cuda#451 and weakref.proxy, this PR implements type checking through x.__class__ instead of type(x).

  • Tests added / passed
  • Passes black dask / flake8 dask

@madsbk madsbk force-pushed the remove_hard_type_check branch from dbc6557 to 6473005 Compare December 16, 2020 15:12
@madsbk madsbk marked this pull request as ready for review December 16, 2020 19:53
@jacobtomlinson
Copy link
Copy Markdown
Member

I am a little out of my depth here, so can I just ask what are the differences between this and other options:

  • if not isinstance(x, type(meta)):
  • if type(x) != type(meta):
  • if x.__class__ != meta.__class__:

@martindurant
Copy link
Copy Markdown
Member

I guess type might return something like ProxyType, but .__class__ would fetch the attribute value from the target object, and so give the underlying class.
isinstance seems to work, though - I suppose it uses .__class__ too?

@madsbk
Copy link
Copy Markdown
Contributor Author

madsbk commented Dec 17, 2020

I am a little out of my depth here, so can I just ask what are the differences between this and other options:

  • if not isinstance(x, type(meta)):
    meta is allowed to be a subclass of x and isinstance(x) uses x.__class__ get type info.

  • if type(x) != type(meta):
    The types must be the same and type(x) always returns the exact class of x

  • if x.__class__ != meta.__class__:
    In most cases this is the same as using type(x) but can be overwritten, which is how Individual CUDA object spilling  rapidsai/dask-cuda#451 and weakref.proxy works.

@madsbk
Copy link
Copy Markdown
Contributor Author

madsbk commented Dec 17, 2020

I guess type might return something like ProxyType, but .__class__ would fetch the attribute value from the target object, and so give the underlying class.

Exactly, isinstance(x) should work just as well with proxy objects but in order to catch mismatch between derived types, I think x.__class__ make more sense.

@jacobtomlinson
Copy link
Copy Markdown
Member

Thanks for unpacking that here. Seems like a reasonable change. I think it's good to have this written down here in case anyone needs to refer back to it.

Just to check my understanding does that mean if not isinstance(x, meta.__class__): would also be valid?

@madsbk
Copy link
Copy Markdown
Contributor Author

madsbk commented Dec 18, 2020

Just to check my understanding does that mean if not isinstance(x, meta.__class__): would also be valid?

Not exactly, in the case of isinstance(x, meta.__class__), x is allowed to be a subclass of meta whereas in x.__class__ != meta.__class__ both must be the same class (or proxy thereof).

@jacobtomlinson
Copy link
Copy Markdown
Member

Ok thanks!

Also thanks for the comments, I think that is really helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants