Remove `inputs` special keyword argument #1790

annawoodard · 2020-07-06T18:27:15Z

@makslevental recently reported a serious bug #1788 and proposed a solution in #1789. One reservation I have with that approach is that it would change current Parsl behavior in a way that might be confusing for users: if a list or tuple of futures is passed as a positional arg, Parsl would wait for each component future to be resolved before executing the app. This might lead users to expect that other structures (like dicts, etc) would also be introspected.

@benclifford pointed out on Slack that because we now check args, keyword args, and the inputs special keyword arg for files to potentially stage in, the inputs special keyword argument is redundant. Borrowing Max's example:

import parsl
from parsl import python_app

@python_app
def monte_carlo():
    from random import random
    from math import sin, cos
    u = random()
    return (sin(u)) / (u + cos(u) ** 2)

@python_app
def mean1(inputs=[]):
    return sum(inputs) / len(inputs)

@python_app
def mean2(inputs=[]):
    return (inputs[0] + inputs[1] + inputs[2]) / len(inputs)

could be replaced by:

import parsl
from parsl import python_app

@python_app
def monte_carlo():
    from random import random
    from math import sin, cos
    u = random()
    return (sin(u)) / (u + cos(u) ** 2)

@python_app
def mean1(*args):
    return sum(args) / len(args)

@python_app
def mean2(*args):
    return (args[0] + args[1] + args[2]) / len(args)

And to clarify how this plays with other arguments and give a full example:

import parsl
from parsl import python_app

@python_app
def monte_carlo():
    from random import random
    from math import sin, cos
    u = random()
    return (sin(u)) / (u + cos(u) ** 2)

@python_app
def mean(foo, *futures, baz='zap'):
    return foo, baz, (futures[0] + futures[1] + futures[2]) / len(futures)

futures = [monte_carlo() for _ in range(3)]
foo, baz, average = mean('bar', *futures, baz='zapzap').result()
print("Foo: {}, Baz: {}, Average: {:.5f}".format(foo, baz, average))

One advantage of removing inputs is that it would be more pythonic, and less Parsl-specific jargon for people to grok. Removing it would also be a fix for #1788.

benclifford · 2020-07-07T08:24:28Z

I support this.

makslevental · 2020-07-12T15:26:34Z

@annawoodard I think the behavior I was naively expecting is exactly that which you rightly pointed out others might end up expecting - that any Future anywhere would be identified as a dependency and waited on. For peak pythonicity I think in fact that's how it should work but I'm not sure if it's possible in Python.

Anyway this is a breaking change (removing inputs) but if this is what you guys think is the right thing to do then I can take it on.

benclifford · 2020-07-13T10:54:57Z

When we've removed features before, we've made them deprecated for one release, and then removed the code in the next release.

What that would look like here, I think, is that from now until parsl (1.0.0 + 1 version) is released, the code should log a warning when inputs is used, such as "inputs is deprecated and will be removed in the next parsl release. See #1790 for more information."

Perhaps that warning could be placed in parsl.app.app.AppBase.__init__ which already has a test to see if inputs is used.

Then this issue should be left open until sometime actually removes all the code, sometime after (1.0.0 + 1 version) is released

kylechard · 2020-07-13T14:00:17Z

I don't think this will be a breaking change? The change here would expand the way we introspect arguments such that any list/tuple would be treated the same way that inputs are currently treated. So those that want to use inputs can continue to do so. We can simply remove inputs from the documentation as a special keyword.

benclifford · 2020-07-13T14:15:34Z

I think @annawoodard 's proposal is to remove inputs, but not add in new deep data structure inspection for parsl-magic (that is, not do #1788, #1789). If so, that makes inputs into a regular argument that is not deep-inspected for parsl-magic, in the same way that any other argument which happens to be a list is also not inspected. (parsl-magic = Futures and Files). That breaks almost any use of inputs.

annawoodard · 2020-07-13T17:25:01Z

@kylechard Ben's summary of the proposal is correct. I favor logging a warning, changing docs, and removing code in next release. Alternatively we could retain the current behavior for backwards compatibility but just change the docs.

@yadudoc @ZhuozhaoLi @danielskatz interested if you have any opinions on this

danielskatz · 2020-07-13T17:30:31Z

We talked about this in the Parsl meeting Friday, and were leaning towards your

retain the current behavior for backwards compatibility but just change the docs.

We also talked about removing outputs but no one but @kylechard really seemed to care

yadudoc · 2020-07-13T18:08:16Z

Summarizing discussion from Friday:

There's general interest in removing inputs and outputs as special kwargs.
One proposal is to move towards inspecting all params at call time to capture Futures, and supporting inspection of lists, tuples and dicts would cover most cases.
Another proposal was to have deep inspection of all data-structures. This comes with a performance penalty and practical limitations from complex data structures within which Futures can be placed.
One proposal regarding outputs was to create a new class that indicates that a param is specifying an output File object :

output_file = File("foo.txt")
app(input_file, ParslOutput(output_file))

Fixing outputs is a less pressing issue, and is mostly likely always going to be jarring given the idea of specifying outputs of a function ahead of time is a foreign concept in python.

Parsl / parsl

Remove `inputs` special keyword argument #1790

Remove `inputs` special keyword argument #1790

annawoodard commented Jul 6, 2020

benclifford commented Jul 7, 2020

makslevental commented Jul 12, 2020

benclifford commented Jul 13, 2020 •

edited

kylechard commented Jul 13, 2020

benclifford commented Jul 13, 2020 •

edited

annawoodard commented Jul 13, 2020

danielskatz commented Jul 13, 2020

yadudoc commented Jul 13, 2020

Parsl / parsl

Join GitHub today

Remove `inputs` special keyword argument #1790

Remove `inputs` special keyword argument #1790

Comments

annawoodard commented Jul 6, 2020

benclifford commented Jul 7, 2020

makslevental commented Jul 12, 2020

benclifford commented Jul 13, 2020 • edited

kylechard commented Jul 13, 2020

benclifford commented Jul 13, 2020 • edited

annawoodard commented Jul 13, 2020

danielskatz commented Jul 13, 2020

yadudoc commented Jul 13, 2020

benclifford commented Jul 13, 2020 •

edited

benclifford commented Jul 13, 2020 •

edited