Uh oh!
There was an error while loading. Please reload this page.
WIP: raise an error when using a lambda and default executor - #210
Conversation
akhmerov
commented
Aug 26, 2019
Here's my thinking:
Therefore I propose to not apply a patch like this, but rather wait until the issue becomes more important, or someone (maybe one of us) is motivated to have a proper fix. |
basnijholt
commented
Oct 11, 2019
To be fair, two persons now reported this issue separately. See #211 for the other one. |
akhmerov
commented
Oct 11, 2019
Perhaps a better implementation would be to try to pickle the function on our own and see if that raises? |
I have added that check. I still also explicitly check whether it is a lambda because that is the most common case I think. Instead, we could probably also just add that in the error message. |
jbweston
left a comment
There was a problem hiding this comment.
Just that minor clarification in the error message to make it very clear what users need to do to make it go away.
Other than that LGTM
Uh oh!
There was an error while loading. Please reload this page.
| return False | ||
| if executor is None: | ||
| if isinstance(learner.function, types.LambdaType): |
There was a problem hiding this comment.
this check doesn't actually check if the function is a lambda. Check it out:
import types
def f():
pass
print(isinstance(f, types.LambdaType))
prints True
There was a problem hiding this comment.
This is why the tests are failing AFAICT
There was a problem hiding this comment.
I've made the check a bit simpler.
basnijholt
commented
Oct 24, 2019
There was a problem hiding this comment.
The tests still fail and the failure is relevant to this issue.
AsyncRunner does not actually use the executor if the function passed in is an async def function. In this case the (async def) function is not pickled at all and we don't need to worry about pickling.
The check should be changed to
if executor is None and not inspect.iscoroutinefunction(learner.function):
basnijholt
commented
Nov 19, 2019
Kim just ran into this problem too. |
If we fix the test failures (i.e. fix the code) we can merge! |
Closes#206.
It is not obvious that this blocks the kernel, but it does hang forever and does not error.
This PR makes sure an error is raised, however, might this be caused by an underlying issue?