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

RFC: Task initialization syntax #295

Open
effigies opened this issue Jun 18, 2020 · 9 comments
Open

RFC: Task initialization syntax #295

effigies opened this issue Jun 18, 2020 · 9 comments
Labels
to consider suggesting changes that require more discussion

Comments

@effigies
Copy link
Contributor

Let's break things!

Right now, when you instantiate a Task, you can give it a name:

@pydra.mark.task
def greet(name : str) -> str:
    return f"Hi, {name}"

greeter = greet(name="greeter")
print(greeter(name="@effigies").output.out)
Hi, @effigies

So this works, but it might be worth considering whether this is going to cause confusion in a workflow context:

wf = pydra.Workflow(name="myworkflow", input_spec=["name"])
wf.add(greet(name="greeter"))
wf.greeter.inputs.name = wf.lzin.name
wf.set_output([("out", wf.greeger.lzout.out")])
wf.inputs.name = "@effigies"

with pydra.Submitter() as sub: 
    print(sub(wf).output.out)
Hi, myworkflow

So this is both confusing and actually doesn't work.

In any event, I wanted to open discussion about possibly rethinking the syntax, or possibly just some names.

@effigies effigies added the to consider suggesting changes that require more discussion label Jun 18, 2020
@satra
Copy link
Contributor

satra commented Jun 18, 2020

a couple of options are:

1. greet(name="greeter", [other_init_keywords,] inputs={"name": wf.lzin.name})
2. greet(name="greeter", [other_init_keywords,] pydin_name=wf.lzin.name)

@satra
Copy link
Contributor

satra commented Jun 19, 2020

@effigies - we already had this in the base class:
https://github.com/nipype/pydra/blob/master/pydra/engine/core.py#L84
https://github.com/nipype/pydra/blob/master/pydra/engine/core.py#L157

@djarecka - somehow this was not adhered to in derived classes. any memories why? but in general we should follow this pattern. note that this issue is only for init

@djarecka
Copy link
Collaborator

@satra - i don't remember why, but it was a time that input was the only option to provide input in __init__.

I'm a bit confuse your second option... is it possible that you wanted to write:
2. greet(pydin_name="greeter", [other_init_keywords,], name=wf.lzin.name)?

@effigies
Copy link
Contributor Author

I think I would be inclined to keep inputs explicit at __init__, and continue passing them as keywords at __call__(). I think for a subclass to change the syntax just invites confusion.

@satra
Copy link
Contributor

satra commented Jun 19, 2020

  1. greet(pydin_name="greeter", [other_init_keywords,], name=wf.lzin.name)?

i did not mean that. i did mean the prefix to indicate input keywords.

i think at this point i would take the decision of restricting input values during init, not during run or function call, only through the inputs keyword.

@djarecka
Copy link
Collaborator

I got used to the shorter syntax, but this can cause problems, so probably we should change it.
I guess we already proofed that the option 1 is better than the option 2 ;-)

@djarecka
Copy link
Collaborator

should I work on this next week, or we still wanna take more time to think?

@effigies
Copy link
Contributor Author

Is there a hive mind to ping that isn't already getting notifications from here? If not, I'd say go ahead.

@satra
Copy link
Contributor

satra commented Jun 20, 2020

i say go ahead too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to consider suggesting changes that require more discussion
Projects
None yet
Development

No branches or pull requests

3 participants