-
Notifications
You must be signed in to change notification settings - Fork 5
feat: support visualization of graph queries by adding the --graph argument. #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… add a test to bigquery magic for when spanner-graph-notebook is missing.
This is required for graph visualization.
…python runtime 3.12 instead of 3.8, as spanner-graph-notebook does not support runtime version 3.8.
…raph server is already running, due to another graph query having run previously.
| result = result.to_dataframe(**dataframe_kwargs) | ||
|
|
||
| if args.graph and _supports_graph_widget(result): | ||
| _add_graph_widget(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do a warning if they asked for --graph but the results didn't support it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long-term, I view --graph as a temporary flag to avoid breaking the standard code path while the graph implementation is not stable yet. Eventually, I would like to see the flag go away, and the graph visualization just be automatic in response to running a graph query.
So, I don't think such a warning is warranted right now.
bigquery_magics/graph_server.py
Outdated
| self.handle_post_query() | ||
|
|
||
|
|
||
| atexit.register(GraphServer.stop_server) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with atexit, what does it do and why do we need it? Might be worth adding a few comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code is pretty much self-explanatory. It's necessary to prevent unit tests from hanging, due to the server background thread still running at the end of all of the tests.
| GraphServer._server = None | ||
|
|
||
|
|
||
| class GraphServerHandler(http.server.SimpleHTTPRequestHandler): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love a few docstrings on these methods explaining their purpose. I can make some guesses based on the method names, but they are pretty generic names, so it's hard to interpret the intention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some.
| from google.cloud.spanner_v1.types import StructType, Type, TypeCode | ||
| import networkx | ||
| from spanner_graphs.conversion import ( | ||
| columns_to_native_numpy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of https://github.com/cloudspannerecosystem/spanner-graph-notebook/blob/cf18014e5af6309bfee537d070278aaf76cc5d1b/spanner_graphs/conversion.py this method no longer exists.
…anges to that repository broke our use of is.
feat: Support visualization of graph queries by adding the --graph argument.