-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Correctly handle signature-less functions for Py UDF calls #6368
base: main
Are you sure you want to change the base?
Conversation
@@ -613,6 +613,12 @@ def f(p1: float, p2: np.float64) -> bool: | |||
self.assertRegex(str(w[-1].message), "numpy scalar type.*is used") | |||
self.assertEqual(10, t.to_string().count("true")) | |||
|
|||
def test_no_signature(self): | |||
builtin_max = max | |||
t = empty_table(10).update("X = (int) builtin_max(1, 2, 3)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into this problem some more, pybind11
actually does provide signature information in a way that I think we are not yet supporting. From the example in #6349, I added:
print(blackscholes.price.__doc__)
This outputs a docstring that is prefixed with the method signature.
price(arg0: float, arg1: float, arg2: float, arg3: float, arg4: float, arg5: bool, arg6: bool) -> float
Here are more things that may be useful:
print(type(blackscholes.price))
<class 'builtin_function_or_method'>
print(blackscholes.price.__qualname__)
PyCapsule.price
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I don't think the doc string of a callable is a reliable source to infer its signature. Even if it is, the work needed to do that correctly may not justify the benefit
- that said,
jedi
does try to infer function arguments for sphinx, epydoc and basic numpydoc docstrings, but it doesn't expose any public API for that. - PyCharm seems to rely solely on signatures to do static check/code assistant on functions
Based on the above, I am not sure it is so bad that we just document the limitation and workaround when we can't get a signature via the standard inspect
module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could easily make this "just work" by using the return type. Relying on documented workarounds is a poor substitute to the product functioning as expected.
Fixes #6349