Eagerly populate the class dict for cudf.pandas proxy types - #14534
Conversation
…populate-class-dict
…populate-class-dict
…populate-class-dict
…populate-class-dict
…populate-class-dict
…populate-class-dict
…populate-class-dict
|
/okay to test |
|
/okay to test |
|
@wence- I've reverted the vast majority of the changes I made to this PR and this is now ready for review. Since I've made a couple of minor fixes to this pr I'd like to get it reviewed by someone other than me before merging. Something to note, the drop in test pass rate is expected and are valid failures that are not in the scope of fixing in this PR. |
Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>
| if instance is not None: | ||
| return _maybe_wrap_result( | ||
| getattr(instance._fsproxy_slow, self._name), | ||
| None, # type: ignore |
There was a problem hiding this comment.
Would be good to adjust the func typing in _maybe_wrap_result as func: Callable | None and raise an exception in is_final_type and is_intermediate_type branches if func is None
There was a problem hiding this comment.
This doesn't seem to be straight-forward, while testing this change I noticed 4k additional failures. Maybe we can revisit this in another PR.
| "_constructor": _FastSlowAttribute("_constructor"), | ||
| "__array_ufunc__": _FastSlowAttribute("__array_ufunc__"), | ||
| "_accessors": set(), | ||
| "_data": _FastSlowAttribute("_data", private=True), |
There was a problem hiding this comment.
So these attributes we are wanting the fast path to have a chance to evaluate and not automatically be _Unusable()?
There was a problem hiding this comment.
Ah so in this case since it's private we won't try to operate on e.g. cudf.Index._data which is a ColumnAccessor and instead reach for cudf.Index._data which is a numpy ndarray which we can wrap.
This part of the eager population I'm not too thrilled about. These private variables are not guaranteed to be stable. It would be nice if any slow attribute was attempted to return a proxy object so we didn't have to specify attributes like this but I suppose that can be looked at in a follow up
There was a problem hiding this comment.
Yeah, there are still such cases we can address in follow-ups. At this point the scope of this PR has kept on expanding, that's why I haven't addressed additional issues in this PR.
|
/okay to test |
mroeschke
left a comment
There was a problem hiding this comment.
My typing request and concern about having to define private attributes in our proxies can be addressed in a follow up
|
/merge |
Rather than dynamically looking up class attributes (and methods), this PR makes it so that we eagerly populate the class with all known methods and attributes (by inspecting the "slow" class).
This solves a number of problems:
getattrtrivially inexpensive (no dynamic__getattr__for each attribute access)DataFrame.maxSeries.listsuper().to access attributes of base types