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

Fix: notebook_login() does not update UI on Databricks #1414

Merged
merged 15 commits into from
Apr 4, 2023

Conversation

fwetdb
Copy link
Contributor

@fwetdb fwetdb commented Mar 29, 2023

Fixes 1384

The PR adjusts notebook_login() to use the ipywidgets Box output to print the feedback.
This enables the logic to work in all notebooks that are compatible with ipywidgets - including Databricks where clear_output does not work.

Examples for the code flow:
Screenshot 2023-03-29 at 12 08 35

Screenshot 2023-03-29 at 12 08 48

Screenshot 2023-03-29 at 12 09 00

Screenshot 2023-03-29 at 12 16 49

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 29, 2023

The documentation is not available anymore as the PR was closed or merged.

@Wauplin
Copy link
Contributor

Wauplin commented Mar 29, 2023

Hey @fwetdb thanks for working on this! At first glance PR looks good :)

I'll make some tests in different environments (colab/jupyter, python3.7/3.10) soon and get back to you for a proper review. If you see some code quality issues, don't worry I can handle them.

@Wauplin
Copy link
Contributor

Wauplin commented Mar 31, 2023

Hi @fwetdb, I made some tests and it works great! I think we are good to go. I pushed some code to your branch. Instead of the add_string_to_widget_output method passed as argument, I re-used a utility that we have in our tests that redirects the output from print to a buffer. I prefer it this way so that we don't change _login itself. I hope you are fine with me changing it 🙏

@osanseviero could you have a second look please as it impact all our notebook users? I made a colab for you to quickly test the new UI. The main change is that the returned message ("you're logged in !") is not printed from the terminal but displayed as a widget (and centered). For context, the goal is to avoid clear_output() that is not handled by Databricks. cc @LysandreJik that might have an opinion as well.

(For testing, I tried jupyter notebook on Python 3.7 and 3.10 + tried google Colab. Is there another notebook we should try?)

@osanseviero
Copy link
Contributor

It looks ok from my side! Thanks for the fix

@julien-c
Copy link
Member

LGTM too and quite happy that we improve our support of Databricks / Databricks notebooks 👍

@fwetdb
Copy link
Contributor Author

fwetdb commented Apr 3, 2023

Thank you @Wauplin and everyone else involved!
The adjustments look good to me and I just double-checked them on Databricks.
So, good to merge from my side.

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Good, thanks @fwetdb for double-checking this!
I'm merging the PR then 🎉

@Wauplin Wauplin merged commit 7a3e1e8 into huggingface:main Apr 4, 2023
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.

notebook_login() does not update UI on Databricks
5 participants