[wip, fix] small changes and fixes #76
Conversation
…r no input_spec; removing loop.close when Jupyter in os.environ (I add this variable in the tutorials); adding output_names to Task; adding tests (some xfail)
pydra/engine/submitter.py
Outdated
| @@ -149,7 +150,9 @@ def __exit__(self, type, value, traceback): | |||
| return futures | |||
|
|
|||
| def close(self): | |||
| self.loop.close() | |||
| # jupyter is also using cf, so can't close the loop | |||
| if not "Jupyter" in os.environ: | |||
There was a problem hiding this comment.
this seems something that Jupyter should handle, not us. i don't want pydra to be dependent on whether it's used in a specific environment.
There was a problem hiding this comment.
The short answer is - it also surprised me, but Jupyter doesn't handle... Note, that it's not only this, I still had to import nest_asyncio to the notebook!
I agree that we don't want to change pydra, the if statement seemed to me the only "acceptable" option, but I will search more.
There was a problem hiding this comment.
yesterday I was thinking about forgetting about jupyter, but I'm guessing it's not an option that will be approved ;-)
There was a problem hiding this comment.
Do we have config?
| if not "Jupyter" in os.environ: | |
| if not config.getboolean("execution", "nested_asyncio"): |
Then, Jupyter notebooks could use a modified config file, or users could add at import:
import pydra
pydra.config.set("execution", "nested_asyncio", True)There was a problem hiding this comment.
but the problem is that we also want pydra to change the behavior, i.e. do not close the loop. At least that was the solution we found yesterday
There was a problem hiding this comment.
we were thinking about it yesterday, but we were afraid that this might happen also in pydra Workflow itself, and has nothing to do with jupyter.
Just to be clear all tests pass without closing the loop at all, it's "just" a best practice, and I'm not sure when this might be a problem.
@mgxd can comment as well
There was a problem hiding this comment.
a) a loop should be opened only when the program is not already in a loop
b) a loop should be closed only if the loop has been opened by the program
There was a problem hiding this comment.
we can easily detect if the available loop is currently running, and avoid closing it, but we'll still need to import / activate nest_asyncio to allow running tasks through jupyter
There was a problem hiding this comment.
@satra - looks like this is not issue create by jupyter, but newer version of tornado, and it looks like this is still not fixed. It works fine (at least for my simple example, will be testing more) if older version of tornado is used tornado==4.5.3
There was a problem hiding this comment.
didn't read all posts, but you can find more here: jupyter/notebook#3397
| @@ -60,6 +60,7 @@ def __init__( | |||
| self, | |||
| func: ty.Callable, | |||
| output_spec: ty.Optional[BaseSpec] = None, | |||
| output_names=None, | |||
There was a problem hiding this comment.
no output_names - if needed to be specified they should use output_spec
There was a problem hiding this comment.
There was a problem hiding this comment.
perhaps an example we should work towards is how to annotate an existing function via the to_task decorator
There was a problem hiding this comment.
I was thinking that output_names could be the easier option
There was a problem hiding this comment.
i agree it would be easier, but we want them to do it through proper annotation. since we will want people to provide type hints as well eventually.
so we should see if we can add __annotations__ to the function using the decorator and adding arguments to the decorator.
There was a problem hiding this comment.
ok, will work on it
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
==========================================
- Coverage 85.01% 85.01% -0.01%
==========================================
Files 12 13 +1
Lines 2109 2115 +6
Branches 539 541 +2
==========================================
+ Hits 1793 1798 +5
- Misses 231 232 +1
Partials 85 85
Continue to review full report at Codecov.
|
djarecka
changed the title
| @@ -441,6 +437,8 @@ def __init__( | |||
| + [("_graph", ty.Any)], | |||
| bases=(BaseSpec,), | |||
| ) | |||
| else: | |||
| raise Exception("Workflow has to have input_spec") | |||
There was a problem hiding this comment.
I think you can just do this with types:
class Workflow(TaskBase):
def __init__(
self,
name,
- input_spec: ty.Union[ty.List[ty.Text], BaseSpec, None] = None,
+ *,
+ input_spec: ty.Union[ty.List[ty.Text], BaseSpec],
output_spec: ty.Optional[BaseSpec] = None,
audit_flags: AuditFlag = AuditFlag.NONE,Keyword-only arguments don't have to have defaults.
|
should this be closed? |
various small changes:
input_spec(Assuming that changing input_spec to allow tasks without input #70 won't be accepted);output_namesto Task, so it doesn't have to be "out"tests that xfail (will be fixing later):
output_namesis not specified we can't save as an output multiple variables