FEAT: Validate that the output type of a UDF is a single element #2198

timothydijamco · 2020-05-05T16:34:35Z

The normal syntax for defining an elementwise UDF is passing a single value to output_type:

@elementwise(input_type=[dt.float64, dt.float64], output_type=dt.float64)
def sum_udf(a, b):
    return a + b

If the output type is placed into a list (similar to input_type) (see below), then executing an expression with the UDF succeeds on Pandas but fails on Spark with TypeError: 'NoneType' object is not callable

@elementwise(input_type=[dt.float64, dt.float64], output_type=[dt.float64])
def sum_udf(a, b):
    return a + b

Currently, output_type=[dt.float64] is interpreted as the output type being array<float64>

This PR unpacks the single value in output_type if it's a list and uses that as the output type so that e.g. output_type=[dt.float64] is treated as just a float64

edit: This PR was originally "BUG: Allow output type of vectorized UDFs to be specified as a single-element list" but was renamed (see below)

timothydijamco · 2020-05-05T17:23:32Z

cc @icexelloss @jreback

icexelloss · 2020-05-06T14:29:47Z

ibis/tests/all/test_vectorized_udf.py

@@ -10,9 +10,22 @@ def add_one(s):
    return s + 1


+@elementwise(input_type=[dt.double], output_type=[dt.double])
+def add_one_with_output_type_list(s):


I think you can inline this function in the tests, we don't think we need to use this in other tests.

icexelloss · 2020-05-06T14:31:13Z

ibis/tests/all/test_vectorized_udf.py

+
+@pytest.mark.only_on_backends([Pandas, PySpark])
+@pytest.mark.xfail_unsupported
+def test_elementwise_udf_with_output_type_list(backend, alltypes, df):


We should probably comment that this does NOT test the output of the UDF being and list. We only test that specifying output=[dt] is the same as output=dt in the udf decorator.

icexelloss · 2020-05-06T14:32:35Z

ibis/udf/vectorized.py

+
+        self.input_type = list(map(dt.dtype, input_type))
+
+        if type(output_type) is list:


It's more common to use isinstance(output_type, list)

icexelloss · 2020-05-06T14:33:16Z

ibis/udf/vectorized.py

+        self.input_type = list(map(dt.dtype, input_type))
+
+        if type(output_type) is list:
+            [output_type] = output_type


It's more common to use

output_type, = output_type

should probably also catch the exception when the list has more then one element and give a reasonable error message

It's more comment to use

output_type, = output_type

blacken was changing this to (output_type,) = output_type so [output_type] = output_type seemed cleaner, however, if output_type, is more common then maybe (output_type,) is more readable

should probably also catch the exception when the list has more then one element and give a reasonable error message

Will do

icexelloss

left some comments

jreback

the spark back end is not running, see #2201 need to re-enable that first.

also pls add a release note

jreback · 2020-05-28T23:01:00Z

if you can rebase this

timothydijamco · 2020-05-29T12:14:09Z

Done! CI is green

The rebase did involve resolving some merge conflicts, but the changes are exactly the same besides the name of the test and comments / docstrings. Because of #2210 the changes now apply also to analytic and reduction UDFs, so I'll rename the PR

jreback · 2020-05-29T12:23:03Z

hmm, why are we allowing this kind of output type as a list? I would rather just raise if this is specified (on all backends) as better to be strict.

icexelloss · 2020-05-29T14:30:03Z

@jreback I am fine with raise. But I prefer allowing output_type=[dt.double] a little more because this is more symmetrical to the input_type parameter. e.g.

@elementwise(
    input_type=[dt.double, dt.double]
    output_type=[dt.double]
)
def foo(...)

I know currently output type can only be a single col, but it seems natural to type output_type=[dt.double] than output_type=dt.double after typing input_type=[...]

jreback · 2020-05-29T14:45:59Z

@jreback I am fine with raise. But I prefer allowing output_type=[dt.double] a little more because this is more symmetrical to the input_type parameter. e.g.
@elementwise(
    input_type=[dt.double, dt.double]
    output_type=[dt.double]
)
def foo(...)
I know currently output type can only be a single col, but it seems natural to type output_type=[dt.double] than output_type=dt.double after typing input_type=[...]

this syntax goes against all other call syntaxes (numpy ufunc, numba); i don't why i would allow this. ok with raising.

icexelloss · 2020-05-29T14:48:23Z

Ah yes that make senses. We won't want to go against existing conventions. Let's raise.

timothydijamco · 2020-05-29T18:16:37Z

^ Sounds good to me!

I refactored the validation function in ibis/udf/vectorized.py out into a new module, ibis/udf/validate.py and added a function to that module to validate the output_type. That function is now used throughout the backends that have UDFs with output type arguments

This is starting to sound more like a feature (adding validation) than a bug fix to me, so I'm going to rename this PR

icexelloss · 2020-05-29T19:59:15Z

ibis/udf/vectorized.py

@@ -102,6 +52,10 @@ def func(*args):


 def _udf_decorator(node_type, input_type, output_type):
+    if isinstance(output_type, list):


Please update https://github.com/ibis-project/ibis/blob/master/ibis/expr/operations.py#L3462 to do proper rlz checking instead of checking it here.

See https://github.com/ibis-project/ibis/blob/master/ibis/expr/rules.py and other examples in https://github.com/ibis-project/ibis/blob/master/ibis/expr/operations.py

Oops, this shouldn't be here (the checking is already done in UserDefinedFunction __init__)

I'll look into doing checking using rlz

I was trying rlz checking, but now I think it actually might be more appropriate to do the checking before we create the node (in other words, not using rlz)

Using rlz seems to be geared towards describing/validating each argument of a node, when the argument is in its "final form" in the node

E.g.
We want to make sure output_type=[dt.double] is caught and an error is raised. (Note that output_type "starts out" as a Python list)

So we can detect this syntax by (at some point) checking if the output_type is a Python list

But by the time we're in the node doing rlz checking, output_type would have been converted to an Ibis datatype here. The rule would see output_type as an Ibis Array, which I think is valid (user could have intentionally specified output_type=dt.Array(dt.double)) so we can't raise an error

There are workarounds that would let us still use a rlz check (e.g. do the type conversion in the node after validation has been done) but after exploring this I feel like it makes sense to conceptually treat the validation as checking the user's input to the UDF API as opposed to validating the node arguments

(I still plan to remove this particular check in _udf_decorator, it's redundant (the checking is already done elsewhere))

ibis/udf/validate.py

ibis/tests/all/test_vectorized_udf.py

ibis/sql/postgres/udf/api.py

icexelloss · 2020-05-29T20:04:03Z

ibis/spark/udf.py

-        valid_function_signature(self.input_type, func)
+        # Validate that the input_type argument and the function signature
+        # match and that the output_type is valid
+        v.validate_input_type_count(self.input_type, func)


Don't need to touch Spark udf either. Only PySpark and Pandas backend supports "vectorized UDF".

ibis/bigquery/udf/api.py

…item list

…type, raise error for all backends

ibis/udf/validate.py

timothydijamco · 2020-06-03T17:30:53Z

@jreback @icexelloss

Rebased and addressed comments

and CI is green!

icexelloss

LGTM

jreback · 2020-06-08T13:29:00Z

ibis/udf/validate.py

+import ibis.common.exceptions as com
+
+
+def _parameter_count(funcsig):


can you type this

jreback · 2020-06-08T13:29:06Z

ibis/udf/validate.py

+    )
+
+
+def validate_input_type(input_type, func):


can you type this

jreback · 2020-06-08T13:29:13Z

ibis/udf/validate.py

+    return funcsig
+
+
+def validate_output_type(output_type):


can you type this

pep8speaks · 2020-06-08T16:06:37Z

Hello @timothydijamco! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file ibis/sql/postgres/udf/api.py:

Line 95:80: E501 line too long (107 > 79 characters)

jreback

lgtm ping on green

timothydijamco · 2020-06-08T17:42:25Z

@jreback CI is green

Also I'm not sure what the PEP warning is above--the line that it links to is short

jreback · 2020-06-08T21:45:10Z

thanks @timothydijamco

so the PEP warning above is from the configured pep8speaks, but i don't think it uses the same settings as the project. can you open an issue about this? (I think we just need to add a .pep8speaks) or something

timothydijamco · 2020-06-09T13:02:22Z

Thank you both

icexelloss reviewed May 6, 2020

View reviewed changes

icexelloss suggested changes May 6, 2020

View reviewed changes

timothydijamco requested a review from icexelloss May 6, 2020 19:03

icexelloss approved these changes May 6, 2020

View reviewed changes

timothydijamco force-pushed the fix-elementwise-udf-output-type branch from e19ceb3 to 24b9bb6 Compare May 15, 2020 21:02

jreback added bug Incorrect behavior inside of ibis pandas The pandas backend pyspark The Apache PySpark backend labels May 18, 2020

jreback suggested changes May 18, 2020

View reviewed changes

timothydijamco force-pushed the fix-elementwise-udf-output-type branch from 24b9bb6 to 594ae41 Compare May 29, 2020 02:58

timothydijamco changed the title ~~BUG: Allow output type of elementwise UDF to be specified as a single-element list~~ BUG: Allow output type of vectorized UDFs to be specified as a single-element list May 29, 2020

jreback added this to the Next Bugfix Release milestone May 29, 2020

timothydijamco changed the title ~~BUG: Allow output type of vectorized UDFs to be specified as a single-element list~~ FEAT: Validate that the output type of a UDF is a single element May 29, 2020

icexelloss reviewed May 29, 2020

View reviewed changes

ibis/udf/validate.py Outdated Show resolved Hide resolved

icexelloss reviewed May 29, 2020

View reviewed changes

ibis/tests/all/test_vectorized_udf.py Show resolved Hide resolved

icexelloss reviewed May 29, 2020

View reviewed changes

ibis/sql/postgres/udf/api.py Show resolved Hide resolved

icexelloss reviewed May 29, 2020

View reviewed changes

ibis/bigquery/udf/api.py Show resolved Hide resolved

Timothy Dijamco added 14 commits June 2, 2020 23:05

Clean up elementwise UDF tests

96c5322

Add error to be raised if UDF output type is a list but not a single-…

9f29655

…item list

Adjust comment on elementwise UDF test

7cf458d

Shorten name of elementwise UDF output type in list test

4395de3

Fix vectorized UDF rebase

06682ca

Fix spacing of vectorized UDF test

1532576

Add release note

694edc5

Update docstrings of vectorized UDFs

adbda56

Fix typo in docstring of vectorized UDFs

52b205a

Instead of allowing and parsing a single-element list for UDF output_…

46f3c40

…type, raise error for all backends

Update release docs

03109ad

Remove redundant check

08c742f

Add test for invalid output_type syntax

c0daea0

Limit test for invalid output_type to PySpark and Pandas

4b149dc

timothydijamco force-pushed the fix-elementwise-udf-output-type branch from 2181500 to 4b149dc Compare June 2, 2020 23:07

icexelloss reviewed Jun 3, 2020

View reviewed changes

ibis/udf/validate.py Outdated Show resolved Hide resolved

Rename validate_input_type_count to validate_input_type

36e116a

icexelloss self-requested a review June 3, 2020 18:41

icexelloss approved these changes Jun 3, 2020

View reviewed changes

timothydijamco requested a review from jreback June 8, 2020 12:51

jreback suggested changes Jun 8, 2020

View reviewed changes

Type UDF validation functions

08d1744

jreback approved these changes Jun 8, 2020

View reviewed changes

jreback merged commit 6f18f07 into ibis-project:master Jun 8, 2020

timothydijamco mentioned this pull request Jun 9, 2020

Configure pep8speaks #2238

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT: Validate that the output type of a UDF is a single element #2198

FEAT: Validate that the output type of a UDF is a single element #2198

timothydijamco commented May 5, 2020 •

edited

Loading

timothydijamco commented May 5, 2020

icexelloss May 6, 2020

icexelloss May 6, 2020

icexelloss May 6, 2020

icexelloss May 6, 2020 •

edited

Loading

icexelloss May 6, 2020

timothydijamco May 6, 2020

icexelloss left a comment

jreback left a comment

jreback commented May 28, 2020

timothydijamco commented May 29, 2020 •

edited

Loading

jreback commented May 29, 2020

icexelloss commented May 29, 2020

jreback commented May 29, 2020

icexelloss commented May 29, 2020

timothydijamco commented May 29, 2020

icexelloss May 29, 2020

timothydijamco May 29, 2020

timothydijamco May 29, 2020

timothydijamco Jun 2, 2020

timothydijamco Jun 2, 2020

timothydijamco Jun 2, 2020

timothydijamco Jun 2, 2020

icexelloss May 29, 2020

timothydijamco commented Jun 3, 2020

icexelloss left a comment

jreback Jun 8, 2020

jreback Jun 8, 2020

jreback Jun 8, 2020

pep8speaks commented Jun 8, 2020

jreback left a comment

timothydijamco commented Jun 8, 2020

jreback commented Jun 8, 2020

timothydijamco commented Jun 9, 2020


		self.input_type = list(map(dt.dtype, input_type))

		if type(output_type) is list:

		@@ -102,6 +52,10 @@ def func(*args):


		def _udf_decorator(node_type, input_type, output_type):
		if isinstance(output_type, list):

		import ibis.common.exceptions as com


		def _parameter_count(funcsig):

FEAT: Validate that the output type of a UDF is a single element #2198

FEAT: Validate that the output type of a UDF is a single element #2198

Conversation

timothydijamco commented May 5, 2020 • edited Loading

timothydijamco commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

jreback commented May 28, 2020

timothydijamco commented May 29, 2020 • edited Loading

jreback commented May 29, 2020

icexelloss commented May 29, 2020

jreback commented May 29, 2020

icexelloss commented May 29, 2020

timothydijamco commented May 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothydijamco commented Jun 3, 2020

icexelloss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Jun 8, 2020

jreback left a comment

Choose a reason for hiding this comment

timothydijamco commented Jun 8, 2020

jreback commented Jun 8, 2020

timothydijamco commented Jun 9, 2020

timothydijamco commented May 5, 2020 •

edited

Loading

icexelloss May 6, 2020 •

edited

Loading

timothydijamco commented May 29, 2020 •

edited

Loading