-
Notifications
You must be signed in to change notification settings - Fork 202
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
Remove empty default values for inputs/outputs arguments #3744
base: master
Are you sure you want to change the base?
Changes from all commits
f02ea1b
d0a6126
c5b2d44
21470f9
e22b8fa
482ed57
d0ed03e
3e0a124
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -196,7 +196,7 @@ Some keyword arguments to the Python function are treated differently by Parsl | |
return x * 2 | ||
|
||
@python_app() | ||
def reduce_app(inputs = ()): | ||
def reduce_app(inputs): | ||
return sum(inputs) | ||
Comment on lines
+199
to
200
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's one of the cases where an empty iterable would actually work:
But I'm not sure that's useful behavior for this app. |
||
|
||
map_futures = [map_app(x) for x in range(3)] | ||
|
@@ -212,7 +212,7 @@ Some keyword arguments to the Python function are treated differently by Parsl | |
.. code-block:: python | ||
|
||
@python_app() | ||
def write_app(message, outputs=()): | ||
def write_app(message, outputs): | ||
"""Write a single message to every file in outputs""" | ||
for path in outputs: | ||
with open(path, 'w') as fp: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -782,7 +782,7 @@ def _add_input_deps(self, executor: str, args: Sequence[Any], kwargs: Dict[str, | |
logger.debug("Not performing input staging") | ||
return args, kwargs, func | ||
|
||
inputs = kwargs.get('inputs', []) | ||
inputs = kwargs.get('inputs') or [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and in several places below, the old code would assign |
||
for idx, f in enumerate(inputs): | ||
(inputs[idx], func) = self.data_manager.optionally_stage_in(f, func, executor) | ||
|
||
|
@@ -801,7 +801,7 @@ def _add_input_deps(self, executor: str, args: Sequence[Any], kwargs: Dict[str, | |
|
||
def _add_output_deps(self, executor: str, args: Sequence[Any], kwargs: Dict[str, Any], app_fut: AppFuture, func: Callable) -> Callable: | ||
logger.debug("Adding output dependencies") | ||
outputs = kwargs.get('outputs', []) | ||
outputs = kwargs.get('outputs') or [] | ||
app_fut._outputs = [] | ||
|
||
# Pass over all possible outputs: the outputs kwarg, stdout and stderr | ||
|
@@ -884,7 +884,7 @@ def check_dep(d: Any) -> None: | |
check_dep(dep) | ||
|
||
# Check for futures in inputs=[<fut>...] | ||
for dep in kwargs.get('inputs', []): | ||
for dep in kwargs.get('inputs') or []: | ||
check_dep(dep) | ||
|
||
return depends | ||
|
@@ -932,7 +932,7 @@ def append_failure(e: Exception, dep: Future) -> None: | |
append_failure(e, dep) | ||
|
||
# Check for futures in inputs=[<fut>...] | ||
if 'inputs' in kwargs: | ||
if kwargs.get('inputs'): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to above: If |
||
new_inputs = [] | ||
for dep in kwargs['inputs']: | ||
try: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ TARBALL="cctools-$CCTOOLS_VERSION-x86_64-ubuntu20.04.tar.gz" | |
|
||
# If stderr is *not* a TTY, then disable progress bar and show HTTP response headers | ||
[[ ! -t 1 ]] && NO_VERBOSE="--no-verbose" SHOW_HEADERS="-S" | ||
wget "$NO_VERBOSE" "$SHOW_HEADERS" -O /tmp/cctools.tar.gz "https://github.com/cooperative-computing-lab/cctools/releases/download/release/$CCTOOLS_VERSION/$TARBALL" | ||
wget $NO_VERBOSE $SHOW_HEADERS -O /tmp/cctools.tar.gz "https://github.com/cooperative-computing-lab/cctools/releases/download/release/$CCTOOLS_VERSION/$TARBALL" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
and e.g.
Due to the |
||
|
||
mkdir -p /tmp/cctools | ||
tar -C /tmp/cctools -zxf /tmp/cctools.tar.gz --strip-components=1 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,7 +76,7 @@ def slow_increment(x, dur=1): | |
|
||
|
||
@python_app | ||
def join(inputs=[]): | ||
def join(inputs): | ||
return sum(inputs) | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not confident, though, that Parsl treats
args
andkwargs
the same with respect to these two special arguments. I'm going to dig a bit more and see if I can convince myself that these do not have to be keyword arguments in order to be recognized correctly, but I'm worried that the previous=()
default argument habit might have been related to this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I expect that's where that style came from too - but it's not particularly nice and it would be good to figure it out and fix rather than behave not-like-normal-Python