Conversation
|
thanks Cam! i'm out teaching this week so have very limited time, but i'll take a look soon! |
|
thanks for doing this! would we need to remove blanket notimplementederrors like narwhals/narwhals/_arrow/dataframe.py Lines 380 to 392 in 05cb650 for this to be accurate? |
This is correct, though we may be able to detect this type of blanket ① ASTfrom inspect import getsource
from textwrap import dedent
from ast import parse, NodeVisitor
from narwhals._arrow.dataframe import ArrowDataFrame
class Visitor(NodeVisitor):
def __init__(self):
self.has_notimplemented = False
self.has_return = False
super().__init__()
def visit_Return(self, node):
self.has_return = True
def visit_Raise(self, node):
if node.exc.func.id == 'NotImplementedError':
self.has_notimplemented = True
source = dedent(getsource(ArrowDataFrame.join_asof))
tree = parse(source)
v = Visitor()
v.visit(tree)
print(f'{v.has_notimplemented = }')
print(f'{v.has_return = }')
if v.has_notimplemented and not v.has_return:
print('This method is likely not implemented')
# v.has_notimplemented = True
# v.has_return = False
# This method is likely not implemented② Not Implemented Decorator (store on method)Something more reliable may be to implement a from functools import wraps
def not_implemented(msg):
def decorator(func):
@wraps(func)
def wrapper(*args, **kwargs):
raise NotImplementedError(msg)
wrapper.not_implemented = True # store info on the method itself
return wrapper
return decorator
class T:
@not_implemented('Not available in ...')
def f(self):
pass
def g(self):
pass
t = T()
# t.f()
print(
f"{hasattr(T.f, 'not_implemented') = }",
f"{hasattr(T.g, 'not_implemented') = }",
sep='\n'
)
T.f()③ Not Implemented Decorator (store in registry)from functools import wraps
def not_implemented(msg):
def decorator(func):
@wraps(func)
def wrapper(*args, **kwargs):
raise NotImplementedError(msg)
not_implemented.registry.add(wrapper)
return wrapper
if not hasattr(not_implemented, 'registry'):
not_implemented.registry = set()
return decorator
class T:
@not_implemented('Not available in ...')
def f(self):
pass
def g(self):
pass
t = T()
print(
f'{not_implemented.registry = }',
f'{T.f in not_implemented.registry = }',
f'{T.g in not_implemented.registry = }',
sep='\n',
end='\n'*2
)
t.f() |
|
hmm nice! maybe we can indeed do this! it may well be quite convenient for users |
- improve has_operation tests - add is_implemented utility function - add tests for utils.get_class_that_defines_method
| T = TypeVar("T") | ||
| Namespace = TypeVar("Namespace") | ||
|
|
||
|
|
||
| class MetaProperty(Generic[T, Namespace]): | ||
| def __init__( | ||
| self, func: Callable[[T], Namespace], class_value: type[Namespace] | ||
| ) -> None: | ||
| self._class_value = class_value | ||
| self._inst_method = func | ||
|
|
||
| @overload | ||
| def __get__(self, instance: None, owner: type[T]) -> type[Namespace]: ... | ||
| @overload | ||
| def __get__(self, instance: T, owner: type[T]) -> Namespace: ... | ||
| def __get__(self, instance: T | None, owner: type[T]) -> Namespace | type[Namespace]: | ||
| if instance is None: | ||
| return self._class_value | ||
| return self._inst_method(instance) | ||
|
|
||
|
|
||
| def metaproperty( | ||
| returns: type[Namespace], | ||
| ) -> Callable[[Callable[[T], Namespace]], Namespace]: # TODO(Unassigned): Fix typing | ||
| """Property decorator that changes the returned value when accessing from the class. | ||
|
|
||
| Arguments: | ||
| returns: The object to return upon class attribute accession. | ||
|
|
||
| Returns: | ||
| metaproperty descriptor. | ||
|
|
||
| Arguments: | ||
| returns: The object to return upon class attribute accession. | ||
|
|
||
| Returns: | ||
| A decorator that applies the custom metaproperty behavior. | ||
|
|
||
| Examples: | ||
| >>> from narwhals.utils import metaproperty | ||
| >>> class T: | ||
| ... @property | ||
| ... def f(self): | ||
| ... return 5 | ||
| ... | ||
| ... @metaproperty(str) | ||
| ... def g(self): | ||
| ... return 5 | ||
|
|
||
| >>> t = T() | ||
| >>> assert t.f == t.g # 5 | ||
| >>> assert isinstance(T.f, property) | ||
| >>> assert T.g is str | ||
|
|
||
| """ | ||
|
|
||
| def wrapper( | ||
| func: Callable[[T], Namespace], | ||
| ) -> Namespace: # TODO(Unassigned): Fix typing | ||
| return MetaProperty(func, returns) # type: ignore[return-value] | ||
|
|
||
| return wrapper |
There was a problem hiding this comment.
I'm not sure if this is the same, or just a similar issue - but reading this reminded me of an issue with DType.
The way that the nested types work seems like its unsafe, but isn't flagged by a type checker.
Using Array as an example, it has attributes annotated in class-scope (but not as a ClassVar), without a default.
Lines 788 to 797 in 66628a5
Accessing on type[Array] would cause a runtime issue - but won't give a warning statically.
narwhals/narwhals/_polars/utils.py
Lines 193 to 219 in 66628a5
For comparison, Datetime does provide warnings - and doesn't have class-scoped attributes:
Lines 491 to 500 in 66628a5
I guess what I'm thinking is having something like polars has classinstmethod.
But for properties that have a default (accessible on the type) - without leaking to instances.
I feel like this might be similar to what you have here
There was a problem hiding this comment.
pinging @MarcoGorelli to make sure you're aware of the potential bug here
* fix(RFC): Use metaclass for safe `DType` attr access Mentioned in -#1991 (comment) - #1807 (comment) * chore: add `_DurationMeta` Both `Duration` and `Datetime` are working with `polars` now. From this point it should just be reducing code for all the other backends * refactor: upgrade `_pandas` * refactor: upgrade `_arrow` * refactor: "upgrade" `_duckdb` They're all noops, but good to keep consistent * refactor: upgrade `_spark_like` * chore: remove comment moved to https://github.com/narwhals-dev/narwhals/pull/2025/files#r1958596925 * refactor: simplify `__eq__` The metaclass is much narrower than `type` previously * fix: maybe fix typo "dt_time_unit" Fixes #2025 (comment)
What type of PR is this? (check all applicable)
Related issues
has_operationin Ibis #1610Checklist
If you have comments or can explain your changes, please do so below
Review Items
Wanted to get feedback for an idea I had to implement
has_operation. The current API implements the following changesmetapropertydecorator that behaves similarly toproperty(only the getter syntax other functionality would need to be implemented or would need to inherit from property itself).nw.Expr.dt.dateis valid syntax.has_operationto check if a given function passed using the above syntax is available for any given backend.What would be nice for this feature would be if these components were more readily accessible from the narwhals namespace, dynamic imports based on internalized mappings can become tricky to maintain (see
narwhals/utils:has_operation locals.backend_mapping) Instead these expressions could be added to the Implementation enum providing more informative metadata (such as import location lookups as well as namespace import names).Feedback on the concept as a whole or the specific implementation is welcome! If @MarcoGorelli likes this approach, I do believe some additional work is required here to get this working correctly with the internal versioning interface so guidance in this spot would be appreciated.
Here are a few examples to show what this PR contains