[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/478 VOTE +1. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/478 Tests pass. I think this is a good base to move forward to a fully functional async API for Gremlin. VOTE +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/478 Rebase is complete now - tests are all good with: ```text $ mvn clean install && mvn verify -pl gremlin-server -DskipIntegrationTests=false -DincludeNeo4j ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/478 I'm rebasing on `tp32` to clean up that conflict. this branch was pretty far behind. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/478 Well I implemented this API for gremlin-python. There is one major problem: side effects. Since the current side effect API is designed to be synchronous (calling `run_sync`), it cannot be used inside a coroutine, as demonstrated by this [test](https://github.com/apache/tinkerpop/blob/54fac5146a5ac067e75f2fbfe135fae2da641c35/gremlin-python/src/main/jython/tests/driver/test_driver_remote_connection.py#L210). This is problematic for a Tornado or Asyncio based driver. I think in the future we should consider the possibility that the `promise()` method return a `AsyncSideEffect` object so the end user can leverage the async techniques with side effects. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/478 As long as the driver returns a future with a compatible API, yes, it can be used with all Python versions. The snippet you provided will throw an error with an Asyncio or Tornado client, as you can only call [`result()`](https://docs.python.org/3/library/asyncio-task.html#asyncio.Future.result) on a completed future. You will typically need to `yield/yield from/await` it first depending on the Python version and driver (like in the examples provided above). However, the underlying coroutine will spawn independently, which means that even if you don't `yield/yield from/await`, but you wait long enough before calling `result()` it will complete. For example: ```python future = g.V().promise(lambda t: t.toList()) assert not future.done() # Future has not completed await asyncio.sleep(5) # Simulate doing a ton of other blocking stuff assert future.done() # Future has completed (assuming 5 seconds was enough time to complete) result = future.result() # Doesn't throw an error ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user aboudreault commented on the issue: https://github.com/apache/tinkerpop/pull/478 Except my initial concern, can you confirm we can use the latest way with all Python versions and with/without a custom driver? It would be nice to have an unified way that just works everywhere. The asyncio + tornado support are good additions. ```python g.V().promise(lambda x: x.toList()).result() ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/478 It's been about a week since @davebshow laid out the approach he's recommending here. I assume the silence means that there are no objections to that approach and we have consensus on what to do? :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/478 I put together a quick example of how this can be implemented in gremlin-python. Obviously the example is incomplete, but hopefully it can help move the discussion forward: https://github.com/apache/tinkerpop/commit/aa85a9b5278c55aa28014aa135c8498428295071 These changes result in the following APIs depending on which Python future is returned by the driver, but the GLV doesn't care as long as it supports Python's common future API: - Tornado w/Python 2.7+ returning a `tornado.concurrent.Future` ```python @gen.coroutine def go(): vertices = yield g.V().promise(lambda x: x.toList()) assert len(vertices) == 6 count = yield g.V().count().promise(lambda x: x.next()) assert count == 6 loop.run_sync(go) ``` - Asyncio/Tornado with Python 3.5 async/await syntax returning `asyncio.Future` or `tornado.concurrent.Future` ```python async def go(): vertices = await g.V().promise(lambda x: x.toList()) assert len(vertices) == 6 count = await g.V().count().promise(lambda x: x.next()) assert count == 6 loop.run_until_complete(go()) ``` - Driver with Python 2.7+ that returns `concurrent.futures.Futures` (or backport) ```python def go(): vertices = g.V().promise(lambda x: x.toList()).result() assert len(vertices) == 6 count = g.V().count().promise(lambda x: x.next()).result() assert count == 6 ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/478 I agree with the `RemoteConnection` implementation being pluggable. Therefore, the GLV should be able to use any remote connection as long as it complies with the API. But, since pre-Python3 doesn't have any standard implementation of a `Future`, in order to maintain this "plugablity", IMO the GLV should be able to handle whatever type of `Future` the `RemoteConnection` returns. This places the onus on the end user to understand what `RemoteConnection` implementation they are using and the type of Future returned in order to comply with the syntax required by underlying framework implementation. This does not mean that responses need have a different API, only that developers know how to either `yield` or `yield from` them, or use the `concurrent.futures.as_completed` method. The good thing is that the three possible future types (asyncio, tornado, and concurrent) have compatible APIs, and should be able to all be handled by the GLV in the same fashion. However, if I understand correctly (not sure that I do) it seems to me that doing specialized handling, would require us to commit to a certain syntax. Unfortunately, all of these problems really stem from the Python community's unwillingness to drop the legacy Python implementation, which the Python Software Foundation has been urging us to do for years. If I am missing something, or there is a solution that I can't see for some reason, please provide an example of a way that allows any pluggable `RemoteConnection` that can be based on any of the possible Python frameworks that maintain the syntax used for handling the resulting future by the end user. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user newkek commented on the issue: https://github.com/apache/tinkerpop/pull/478 I think the `promise()` method is more elegant as well, as it avoids adding many new methods in the Traversal API --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/478 I think I agree with @okram for now that we not add a lot of methods just yet. It's easy to add methods, but harder to take them away later. Let's be sure this gets used in the fashion we expect it to before we add helper methods that we will be stuck with. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/478 I ran about 50 million messages through Gremlin Server over the weekend and didn't see any problems as a result of my changes here. I think this seems pretty solid now. I'd quietly asked both @newkek and @jorgebay to review last week my intermediate work and both seemed satisfied. I think we can focus this PR on how to implement this for gremin-python now. Seems like discussion on that has started, but the consensus on how to implement isn't quite clear. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/478 @aholmberg I'm not sure if I follow...it seems to me that if a `RemoteConnection` implementation returns a list (or future list ) of `FutureTraversers`, the traversal API can still remain independent from the transport. Even if the GLV needs to operate on the Traversers, `asyncio.Future`, `concurrent.Future`, and `tornado.concurrent.Future` have compatible APIs (`done`, `cancel`, `add_done_callback`, `result`, `set_result`, `etc.`). The "specialized" handling of the future, whether it be a coroutine that requires a `yield`, `yield from`, or `await` expression, or a `concurrent.Future`, will in the end be handled by application code written by the end user. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user aholmberg commented on the issue: https://github.com/apache/tinkerpop/pull/478 > the type of future returned will depend on the underlying RemoteConnection implementation. That depends on whether you are okay leaking remote connection implementation through this API. I have been trying to think about this traversal API as something distinct from transport (in which case you would probably want an abstraction). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user aholmberg commented on the issue: https://github.com/apache/tinkerpop/pull/478 Coupling the traversal API to a web framework does not seem appealing to me. Could we consider using the [futures backport](https://pypi.python.org/pypi/futures), which backports the Python 3 standard library features to Python 2? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/478 thanks @davebshow - i might ask for your assistance on implementing that. still trying to get it right for java. that blocking call in `RemoteStep` stinks. having a bit of a hard time trying to get the future delegated all the way to the driver without tearing stuff up. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/478 @spmallette you can easily implement something similar in Python using Futures. Using `tornado.concurrent.Future` would probably make the most sense, as is is already 2/3 compatible and should work with any Python async syntax (`yield`/`yield from`/`await`). The only problem with is that this ties the GLV code more tightly to Tornado, which was previously only used for IO; however, a similar implementation of Future isn't included with the Python 2 standard library, so Tornado is probably the best option. I actually did something pretty similar to this in the `gremlinclient` library. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user newkek commented on the issue: https://github.com/apache/tinkerpop/pull/478 The issue with using a secondary thread pool that will start a new thread for each Traversal execution would be more important when the Traversal is backed by a RemoteConnection. Where each Traversal represents a "query" that is sent to a server, if the driver behind the RemoteConnection is able to handle tens of thousands of requests, the performance will still be limited to the number of threads the async thread pool can handle simultaneously, and there would be a big waste of CPU/Threads. Ideally as @jorgebay suggests with the strategies more of the TinkerPop lib would be changed to become fully async, where maybe synchronous methods would only be blocking calls to the async execution method(s) (`next()` calls `promise().get()`). The thread pool is still needed at some point though, since executing against a local TinkerGraph is currently done in the same thread, as a sequential operation so Returning with a Future, and Continuing to process the operation in the background has to be done in 2 separate threads simultaneously. _just thinking out loud and I probably miss a lot of details_ There may be a way to have a method `submitAsync()` on the `RemoteConnection` that returns a Future of Traverser (instead of Traverser) that, associated with what @jorgebay suggests for the Strategies and _some other changes_ would allow the `promise()` call, when associated with a `RemoteConnection` not to create a new thread each time by default and simply return the Future returned with `RemoteConnection#submitAsync()`, and thus delegate the asynchronous execution to the RemoteConnection --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user jorgebay commented on the issue: https://github.com/apache/tinkerpop/pull/478 > What makes `toListAsync()` more "fully async" compared to `promise(traversal::toList)`? Internally, from a Java perspective anyway, `toListAsync()` does the same thing, doesn't it? By fully async I meant asynchronous execution that doesn't block in any call and doesn't require a threadpool. All the way down, it would be based on futures / async operations. In java, it would mean no `CompletableFuture::get()` calls. That translates into supporting higher levels of concurrency as there is no threadpool limiting the amount of calls in parallel. For TinkerPop, it would require asynchronous strategies as currently the blocking calls are made in the `apply()` method. I've suggested an `applyAsync()` method for an async strategy interface that returns a `CompletableFuture`. For other technologies, the same logic applies as we shouldn't care about the underlying framework / network library (Python tornado / libuv / ...) or thread pools. For C#, it would be `Task` all the way down; for Python it could be async generators or futures all the way down; for Javascript, Promise or async callbacks; ... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/478 > CoreTraversalTest is that it uses mutating steps and thus, not subject to OLAP testing Ah - didn't think to check if that was fully ignored. I'll fix that up. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/478 > Why do you ignore promise tests for GroovyTranslator and JavaTranslator. Seems we would want that to work as well. I thought I was getting errors because of the lambda i use to simulate a slow script. Should that work? > Why not have a PromiseTest in the process computer and process standard suites? It would be good to know that computer tests (which wrap a CompletableFuture too!) work as expected. There is a test in the process standard suite: https://github.com/apache/tinkerpop/pull/478/files#diff-37e31635f13d54b33745544db66cc590R282 `CoreTraversalTest` executes as part of the `ProcessStandardSuite`. Is that sufficient? Should I add something extra for computer tests too? > We should probably get Gremlin-Python set up on this branch before merging into tp32/. I can write the code, I just don't know much about promises nor how to do them in Python. I wouldn't mind writing the code. As I mentioned in the comment, I just knew there would be discussion required to get it done. My limited reading on the subject made it seem like there are difference between how it would be implemented in python 3.x and 2.x as well. note sure if @davebshow has a minute to shed any light on this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] tinkerpop issue #478: TINKERPOP-1490 Implemented promise API for Traversal
Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/478 Wow. This is cool. Questions/comments: 1. Why do you ignore promise tests for `GroovyTranslator` and `JavaTranslator`. Seems we would want that to work as well. 2. Why not have a `PromiseTest` in the process computer and process standard suites? It would be good to know that computer tests (which wrap a `CompletableFuture` too!) work as expected. 3. We should probably get Gremlin-Python set up on this branch before merging into `tp32/`. I can write the code, I just don't know much about promises nor how to do them in Python. 4. We should get @jorgebay to review to make sure Gremlin-JavaScript will be able to use this model/naming convention so we don't diverge Gremlin-JS too much from Gremlin-Java. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
