databricks: fix register to display SSO login link prior to timer expiration, fixes: #539 #527
Conversation
…HTML to allow viewing for databricks users
|
@DataBoyTX see ci fails |
|
@vaimdev I think this is really 2 different issues:
|
DataBoyTX
changed the title
|
is this closing in favor of #543 ? |
yes |
CHANGELOG.md
Outdated
| @@ -78,6 +84,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
| ### Breaking 🔥 | |||
|
|
|||
| * GFQL `e()` now aliases `e_undirected` instead of the base class `ASTEdge` | |||
| >>>>>>> 2506b798ec723e906c1c5279f613fe0c37bdbad2 | |||
| PyGraphistry._is_authenticated = True | ||
| arrow_uploader.token = None | ||
| return PyGraphistry.api_token() | ||
| except Exception: # required to log on |
There was a problem hiding this comment.
can we be selective on the Exception for the one we expect to be thrown?
| return auth_url | ||
|
|
||
| @staticmethod | ||
| def databricks_notebook_sso_login( |
There was a problem hiding this comment.
is this for databricks notebooks or databricks dashboards?
Where in dashboards, I think the idea is first cell is a non-blocking print, and second cell is a blocking poll on successful sso
Where in notebooks... ?
| @@ -670,6 +727,12 @@ def register( | |||
| import graphistry | |||
| graphistry.register(api=3, protocol='http', server='nginx', client_protocol_hostname='https://my.site.com', token='abc') | |||
|
|
|||
| **Example: Databricks notebook and dashboard users need to call the following before register to correctly display the HTML link: | |||
There was a problem hiding this comment.
I think in the dashboard case, it needs to be in 2 different cells, one is a non-blocking url print, and the other the blocking register for whichever cell will plot
I'm not sure what's needed, if anything, for notebooks?
| display(HTML(f'<a href="{auth_url}" target="_blank">Login SSO</a>')) | ||
| print("Please click the above URL to open browser to login") | ||
| print(f"If you cannot see the URL, please open browser, browse to this URL: {auth_url}") | ||
| print("Please close browser tab after SSO login to back to notebook") |
There was a problem hiding this comment.
mixing display() and print() suggests an incomplete case analysis & handling, can we handle more carefully?
There was a problem hiding this comment.
we can make print() vs display() options, and sniffers pick the right one
| except Exception: # required to log on | ||
| PyGraphistry.sso_state(arrow_uploader.sso_state) | ||
| auth_url = arrow_uploader.sso_auth_url | ||
| if auth_url and not PyGraphistry.api_token(): |
There was a problem hiding this comment.
in the notebook case (but not needed in dashboard), we risk having an expired token (e.g., been up for an hour)
normally there's an auto-refresh that kicks in for some of the dispatched methods, so this gets auto-handled at plot
BUT when an explicit public call like this one, it makes sense to show the url either way, or to make this an option?
| print("After, if you get timeout error, run graphistry.sso_get_token() to complete the authentication") | ||
| if not in_databricks(): | ||
| # elif in_ipython() or sso_opt_into_type == 'display': # If run in notebook, just display the HTML | ||
| if in_ipython() or sso_opt_into_type == 'display': # If run in notebook, just display the HTML |
There was a problem hiding this comment.
let's make sure overrides get followed before sniffers, so sniffers don't supercede them
…HTML to allow viewing for databricks users