Skip to content
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

Dev/sso login ux #608

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

Dev/sso login ux #608

wants to merge 36 commits into from

Conversation

vaimdev
Copy link
Contributor

@vaimdev vaimdev commented Oct 28, 2024

An attempt to improve UI/UX to the databricks dashboard with SSO usage
Added a few helper functions, so that the databricks dashboard can keep a simpler flow and UX.
Example databricks notebooks that utilize the new helper functions
https://dbc-ac10efa1-77c5.cloud.databricks.com/editor/notebooks/3715908489050384?o=7408253037638600#command/1247495116400904

3 pre-steps for the SSO login using the helper functions (with different cells), followed by the actual plotting python code

graphistry.databricks_sso_login_init()
graphistry.databricks_sso_login(server="test-2-40-74-dbt.grph.xyz", org_name="louiedev")
graphistry.databricks_sso_wait_for_token()

@vaimdev vaimdev requested a review from DataBoyTX October 30, 2024 09:37
@vaimdev vaimdev marked this pull request as ready for review October 30, 2024 09:37
@lmeyerov
Copy link
Contributor

Can you share the code sample in the PR?

(Trouble accessing link, and discussion should be here)

@lmeyerov
Copy link
Contributor

@vaimdev @DataBoyTX This looks strange from a dev UX layer, afaict:

Demo

If I understand correctly, the below is the intended setup:

cell 1:

clear_output()
graphistry.databricks_sso_login_init()

cell 2:

clear_output()
graphistry.databricks_sso_login(server="test-2-40-74-dbt.grph.xyz", org_name="louiedev")

cell 3:

clear_output()
graphistry.databricks_sso_wait_for_token()

cell 4:

g.plot()

Discussion

I understand wanting new underlying primitives, but how to expose it to users should be minimal, clean, integrated, smart defaults, etc

  • Why is this 4 cells instead of 2?

  • why clear_output() ? Can we make that default-on parameter for the auth step with pass-in overrides?

  • I believe part of this weird path is SSO differences in how databricks headless, databricks notebooks, and databricks dashboards work; are these primitives for dashboards only, or notebook + headless too?

Examples of a two-cell: graphistry.register(sso='databricks').... g.plot():

@lmeyerov
Copy link
Contributor

lmeyerov commented Oct 30, 2024

This PR should also include sample ipynb + docs updates. Sounds like we need to iterate first on interface definitions tho.

The current is https://pygraphistry.readthedocs.io/en/latest/demos/demos_databases_apis/databricks_pyspark/graphistry-notebook-dashboard.html

Maybe dashboards or auth should split out?

class DatabricksHelper():

@staticmethod
def sso_repeat_get_token(repeat, wait):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public methods should have .rst-friendly docstrs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, we're trying to make all new code mypy-typed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, noted. I will add them in.

@vaimdev
Copy link
Contributor Author

vaimdev commented Oct 31, 2024

@vaimdev @DataBoyTX This looks strange from a dev UX layer, afaict:

Demo

If I understand correctly, the below is the intended setup:

cell 1:

clear_output()
graphistry.databricks_sso_login_init()

cell 2:

clear_output()
graphistry.databricks_sso_login(server="test-2-40-74-dbt.grph.xyz", org_name="louiedev")

cell 3:

clear_output()
graphistry.databricks_sso_wait_for_token()

cell 4:

g.plot()

Discussion

I understand wanting new underlying primitives, but how to expose it to users should be minimal, clean, integrated, smart defaults, etc

  • Why is this 4 cells instead of 2?
  • why clear_output() ? Can we make that default-on parameter for the auth step with pass-in overrides?
  • I believe part of this weird path is SSO differences in how databricks headless, databricks notebooks, and databricks dashboards work; are these primitives for dashboards only, or notebook + headless too?

Examples of a two-cell: graphistry.register(sso='databricks').... g.plot():

4 cells instead of 2, mainly because each cell will only display the output at the end of running of 1 cell. Previously, there are a few logic written into 1 cell and the output only be displayed after the cell is run which make the messages looks strange,
in some cases, the refresh token will take place and the message shown is not correct.
breaking it into 4 cells will make the the UX consistent and able to control the logic in the case of refresh token, to display the correct message

why clear_output() ? Can we make that default-on parameter for the auth step with pass-in overrides?

clear_output() is not necessary, can be removed.

I believe part of this weird path is SSO differences in how databricks headless, databricks notebooks, and databricks dashboards work; are these primitives for dashboards only, or notebook + headless too?

Mainly for databricks dashboard but tested in both databricks notebook and databricks dashboard.

try:
PyGraphistry.refresh()
except Exception:
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this error case handling may need thinking?

def databricks_sso_login_init(wait=20):
from IPython.display import display, HTML, clear_output

def init_login(wait):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

un-nest

@vaimdev
Copy link
Contributor Author

vaimdev commented Nov 2, 2024

suggestion from Leo:

Show a message on Graphistry server, when SSO login from pygraphistry

@lmeyerov
Copy link
Contributor

lmeyerov commented Nov 8, 2024

@vaimdev FYI ci lint issues

@vaimdev
Copy link
Contributor Author

vaimdev commented Nov 9, 2024

@vaimdev FYI ci lint issues

Yes, noted, added some debugging code, now it is done and cleaning up code.

@staticmethod
def sso_repeat_get_token(repeat: int = 20, wait: int = 5):
""" Function to repeatly call to obtain the jwt token after sso login
Function to obtain the JWT token after SSO login
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sabizmil can you check all public method docstr as these show up in the readthedocs?

Spelling, grammar, docstr types, and examples, following format of the rest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in the latest commit

Copy link
Contributor

@sabizmil sabizmil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left comments with updated message content.

graphistry/pygraphistry.py Outdated Show resolved Hide resolved
graphistry/pygraphistry.py Outdated Show resolved Hide resolved
graphistry/pygraphistry.py Outdated Show resolved Hide resolved
graphistry/pygraphistry.py Outdated Show resolved Hide resolved
graphistry/pygraphistry.py Outdated Show resolved Hide resolved
graphistry/pygraphistry.py Outdated Show resolved Hide resolved
graphistry/pygraphistry.py Outdated Show resolved Hide resolved
graphistry/pygraphistry.py Outdated Show resolved Hide resolved
graphistry/pygraphistry.py Show resolved Hide resolved
graphistry/pygraphistry.py Outdated Show resolved Hide resolved
@lmeyerov
Copy link
Contributor

lmeyerov commented Nov 26, 2024

@vaimdev @sabizmil @DataBoyTX i'm updating def plot() as well as part of some cleanup, and realized it'll conflict. Happy to land mine after this goes in -- any sense of timing here?

@vaimdev
Copy link
Contributor Author

vaimdev commented Nov 26, 2024

@vaimdev @sabizmil @DataBoyTX i'm updating def plot() as well as part of some cleanup, and realized it'll conflict. Happy to land mine after this goes in -- any sense of timing here?

@lmeyerov , my side I am still updating the code with documentation with lower priority because focus on the entitlement Saas. I'll suggest you landed your cleaned up version first

@vaimdev vaimdev requested a review from aucahuasi December 4, 2024 03:45
@lmeyerov
Copy link
Contributor

@vaimdev can you update from master & retest , esp b/c changes from #615

@DataBoyTX any more to do to land? this has been drifting and seems worth putting in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants