Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmao-denver
Copy link
Contributor

Fixes #6349

@@ -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)")
Copy link
Member

Choose a reason for hiding this comment

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

This PR is marked as fixing #6349. #6349 mentions two problems. This test is casting the return type, so it is either a partial fix of the ticket or there should be another test to check type inference.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. 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
  2. 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.
  3. 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.

Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

Python type inference does not work for pybind11 wrapped C++
2 participants