Plugins: Give more flexibility to the user how to init client#663
Conversation
| from .plugins.kustograph import KustoGraph | ||
| res = copy.copy(self) | ||
| res._kustograph = KustoGraph(res, kusto_config) | ||
| res._kustograph = KustoGraph.from_config(kusto_config) |
There was a problem hiding this comment.
I'm more of a fan of the previous mode because it doesn't lose current self bindings from the Plottable::copy() , which is our typical pattern here. The important part for safety is it's a fresh base instance object for client-local state.
There was a problem hiding this comment.
(and if the caller wants empty, they can start with an empty)
There was a problem hiding this comment.
This is the crux of what I am trying to reverse
I don't think these XyzGraphs should know anything about what is bound to the Plottable, that is the whole issue we are having in louie with it being unclear if these are multi tenant / thread safe / global values involved
| nodes, edges = res._spannergraph.gql_to_graph(query) | ||
|
|
||
| # TODO(tcook): add more error handling here if nodes or edges are empty | ||
| return res.nodes(nodes, 'identifier').edges(edges, 'source', 'destination') |
There was a problem hiding this comment.
hardcoding bindings here is a little weird to me in that it moves control of the schema out from gql_to_graph, causing coordination confusion, so maybe it should return bindings too? at that point, i think it should just return a plottable as before...
There was a problem hiding this comment.
if the goal is a fresh one, maybe:
g = res._spannergraph.gql_to_graph(query)
return res.nodes(g._nodes, g._node).edges(g._edges, g._source, g._destination)There was a problem hiding this comment.
see the way I did it this time
| self.connection.close() | ||
| logger.info("Connection to Spanner database closed.") | ||
| self.connection.close() | ||
| logger.info("Closed Spanner connection.") |
There was a problem hiding this comment.
fwiw, does this mean the caller is supposed to do res_g = g.kusto_query_to_graph(query); res_g._kusto_client.close() ? Not a blocker for the this PR, but am curious
There was a problem hiding this comment.
(In general, unsure what lifecycle pattern we want for reconnecting, closing connections, etc, getting back to the whole louie discussion)
There was a problem hiding this comment.
In the past the clients are never closed -- here I am assuming that the SpannerGraph is basically a client wrapper with convenience functions
Usually a user is using the graphistry object that seems more optimized for notebooks then for memory management
If someone wants memory management, then I think they can instantiate the SpannerGraph directly, like I want to do in louie
|
|
||
|
|
||
| def gql_to_graph(self, res: Plottable, query: str) -> Plottable: | ||
| def gql_to_graph(self, res: Plottable, query: str) -> Tuple[pd.DataFrame, pd.DataFrame]: |
There was a problem hiding this comment.
-
I like having these explicit versions to help with explicit consumers, so we can do
gql_df: str -> dfandgql_graph: str -> plottableand consumer has known type -
scenarios like louie, and maybe convenience ones for users, benefit from
gql: str -> df | plottable, and up to the consumer to handle the union type
There was a problem hiding this comment.
The weird thing to me is what if I don't want to chain in my Plottable
|
|
||
| # TODO(tcook): add more error handling here if nodes or edges are empty | ||
| return res.nodes(nodes_df, 'identifier').edges(edges_df, 'source', 'destination') | ||
| return nodes_df, edges_df |
There was a problem hiding this comment.
This will be problematic for louie. Louie expects plottable from gql_to_graph. Any specific reason for this?
There was a problem hiding this comment.
Louie pins the graphistry version so we can change the interface, I don't feel as strongly about this - see comments to Leo
There was a problem hiding this comment.
I'm a bit lost on internal methods vs public
wrt public:
-
see: Plugins: Give more flexibility to the user how to init client #663 (comment)
-
Louie afaict benefits from a generic
str -> pd.DataFrame | Plottable, where louie can do its ownisinstance(...)and handle appropriately -
I think beginner/vibes-y python users would benefit from that more dynamic type signature too...
-
... and only when we do more 'serious' typed code would we want the explicit
gql_to_g/gql_to_dfvariants (or maybe there is magic where we cangql<pd.DataFrame>(...)?). So I'd want "both" vs picking
I hit the same thing when we exposed the remote python+gfql endpoints and we wanted to support multiple return types
| import time | ||
| from typing import Any, List, Dict, Optional | ||
|
|
||
| from google.cloud.spanner_dbapi.connection import connect, Connection |
There was a problem hiding this comment.
we can release in louie off of this PR sooner than later, but blocks landing as a public graphistry release
|
Merged into #659 |
XyzGraphas anXyzClientWrapperwith convenience methodsChanges:
XyzGraphdecouple from Plottable - This way we can be more confident about independenceRequested by Thomas
KustoGraph
kusto_query-> List[pd.DataFrame]query_graph-> Plottable