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 - Bash App definition change #37

Closed
yadudoc opened this Issue Nov 1, 2017 · 9 comments

Comments

@yadudoc
Contributor

yadudoc commented Nov 1, 2017

We currently use a magic variable "cmd_line" in bash apps to specify the command-line string to be executed. For eg :

@App('python', dfk)
def hello ( ):
   cmd_line = "echo 'Hello world' "

This is bad for a few reasons:

  1. This is surprising behavior in python
  2. There's ambiguity in behavior if the user were to reassign to cmd_line
  3. Any values returned are certainly lost (again unusual behavior for python)
  4. Implementation wise, we trace the value assigned to the magic variable and this is computationally expensive.

I recommend that we switch to having bash apps return a string, and that string be treated as the command line executable and if we decide that this change is acceptable, we should merge for 0.3.0.
For eg:

@App('python', dfk)
def hello ( ):
   return "echo 'Hello World' "
@danielskatz

This comment has been minimized.

Show comment
Hide comment
@danielskatz

danielskatz Nov 1, 2017

Collaborator

I can see how this solves 1 and 2. What about 3?

Collaborator

danielskatz commented Nov 1, 2017

I can see how this solves 1 and 2. What about 3?

@danielskatz

This comment has been minimized.

Show comment
Hide comment
@danielskatz

danielskatz Nov 1, 2017

Collaborator

Also, how does this compare with subprocess's way of doing things, which is what I guess most people would use in a roughly similar situation in "normal" python?

Collaborator

danielskatz commented Nov 1, 2017

Also, how does this compare with subprocess's way of doing things, which is what I guess most people would use in a roughly similar situation in "normal" python?

@jmjwozniak

This comment has been minimized.

Show comment
Hide comment
@jmjwozniak

jmjwozniak Nov 1, 2017

Collaborator

3: The return value is not lost, it is used by Parsl. Multiple return statements are processed with expected Python behavior

Collaborator

jmjwozniak commented Nov 1, 2017

3: The return value is not lost, it is used by Parsl. Multiple return statements are processed with expected Python behavior

@yadudoc

This comment has been minimized.

Show comment
Hide comment
@yadudoc

yadudoc Nov 1, 2017

Contributor

@danielskatz The normal way of doing it with subprocess would look like this :

import subprocess
proc = subprocess.Popen(['echo', 'Hello World'])

# You handle waiting or asynchronous behavior explicitly
proc.wait()

PyDFlow does something like this :

from PyDFlow.app import *
from PyDFlow.types import *

@app((localfile), (Multiple(localfile)))
def sort(*infiles):
    return App("sort", "-o", outfiles[0], *infiles)
Contributor

yadudoc commented Nov 1, 2017

@danielskatz The normal way of doing it with subprocess would look like this :

import subprocess
proc = subprocess.Popen(['echo', 'Hello World'])

# You handle waiting or asynchronous behavior explicitly
proc.wait()

PyDFlow does something like this :

from PyDFlow.app import *
from PyDFlow.types import *

@app((localfile), (Multiple(localfile)))
def sort(*infiles):
    return App("sort", "-o", outfiles[0], *infiles)
@danielskatz

This comment has been minimized.

Show comment
Hide comment
@danielskatz

danielskatz Nov 2, 2017

Collaborator

ok, looks fine to me

Collaborator

danielskatz commented Nov 2, 2017

ok, looks fine to me

@mjwilde

This comment has been minimized.

Show comment
Hide comment
@mjwilde

mjwilde Nov 2, 2017

mjwilde commented Nov 2, 2017

@yadudoc

This comment has been minimized.

Show comment
Hide comment
@yadudoc

yadudoc Nov 2, 2017

Contributor

Yes, the proposed change is about how the decorator gets the command line string from the decorated function. Moving to capturing from returns instead of intercepting mid-run makes the implementation simpler, and leaves little room for surprises and confusion for the user (I hope).

If we put " return cmd_line " at the end of every bash app we have now, we get the target behavior, but without it existing code will break.

Here's what I meant by the returns issues :

@App('python', dfk)
def hello ( ):
   cmd_line = "echo 'Hello world' "
   return 10

# The return value is lost since the app call returns an app future
x = hello () 
Contributor

yadudoc commented Nov 2, 2017

Yes, the proposed change is about how the decorator gets the command line string from the decorated function. Moving to capturing from returns instead of intercepting mid-run makes the implementation simpler, and leaves little room for surprises and confusion for the user (I hope).

If we put " return cmd_line " at the end of every bash app we have now, we get the target behavior, but without it existing code will break.

Here's what I meant by the returns issues :

@App('python', dfk)
def hello ( ):
   cmd_line = "echo 'Hello world' "
   return 10

# The return value is lost since the app call returns an app future
x = hello () 
@mjwilde

This comment has been minimized.

Show comment
Hide comment
@mjwilde

mjwilde Nov 2, 2017

mjwilde commented Nov 2, 2017

@yadudoc

This comment has been minimized.

Show comment
Hide comment
@yadudoc

yadudoc Nov 6, 2017

Contributor

This behavior is in master now.

Contributor

yadudoc commented Nov 6, 2017

This behavior is in master now.

@yadudoc yadudoc closed this Nov 6, 2017

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