search

tableau / TabPy

Execute Python code on the fly and display results in Tableau visualizations:
https://tableau.github.io/TabPy/
MIT License
1.56k stars 598 forks source link

Use Native Coroutines instead of gen decorator #316

Closed WillAyd closed 5 years ago

WillAyd commented 5 years ago

Unpins older tornado version with updates as suggested by Tornado:

https://www.tornadoweb.org/en/stable/guide/coroutines.html#native-vs-decorated-coroutines

pep8speaks commented 5 years ago

Hello @WillAyd! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2019-06-23 19:16:16 UTC
WillAyd commented 5 years ago

Hmm so error message seems to be a result of mixing but multithreaded and asynchronous operations. Is there any particular reason to have both in the application or would a port to entirely asynchronous be considered?

WillAyd commented 5 years ago

Thanks for the responses! I think one of them might have gotten lost in GitHub somewhere but in regards to the call_subprocess method I don't think it can be awaited because that actually returns a Future from a multi-threaded operation rather than an a Task.

Is there any particular reason to use multi-threaded operations or could we stick all asynchronous? Seems like the latter would make for a more unified code base with easier maintenance so happy to invest time in making that happen just not sure if there is a historical reason both paradigms were intertwined.

rxing-tableau commented 5 years ago

Thank you for all the contributions Will. As I know, there is no historical blocker from moving to unified asynchronous paradigm. It just needs the refactor. For the Tornado version, we can only support 5.1.1 now, because there is known issue when we move to higher version. It probably related with the gen.coroutines decorator.

WillAyd commented 5 years ago

For the Tornado version, we can only support 5.1.1 now, because there is known issue when we move to higher version. It probably related with the gen.coroutines decorator.

Oh yea! So I should have been more explicit but that is the point of this. If we remove these decorators and place with the native async syntax from Python (as suggested by Tornado anyway) we wouldn't need to pin to 5.1. A lot of those features were intentionally removed in Tornado 6

As I know, there is no historical blocker from moving to unified asynchronous paradigm. It just needs the refactor.

Good to know. I'll take a look at removing that altogether over the next few days then - just doing async should be easier to maintain and would be less buggy

0golovatyi commented 5 years ago

@WillAyd Do you still have plans to work on this change? If not I am going to close the PR.

WillAyd commented 5 years ago

Feel free to close for now - can always pick up later