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

NotebookProgressCallback doesn't work in databricks notebooks properly--should either be fixed or removed from trainer automatically if it is a databricks runtime #17406

Closed
Ashvio opened this issue May 24, 2022 · 7 comments · Fixed by #17496

Comments

@Ashvio
Copy link

Ashvio commented May 24, 2022

class NotebookProgressCallback(TrainerCallback):

Hey all,
Using databricks for training, the default Trainer behavior will automatically add NotebookProgressCallback for databricks notebooks, but the databricks UI currently does not display the output properly. It just prints a bunch of text saying <IPython.core.display.HTML object> over and over. This is likely an issue on Databricks' end, so I recommend not adding this callback if the transformers library can detect it is a databricks runtime not a jupyter/google collab notebook. I think there should also be an easier way to delete specific callbacks--it took a long time to trace this issue and reading source code to figure out what the source cause is. I am circumventing the issue for now by popping the callback from the trainer callback handlers list but that is not a good pattern.

Thanks!

@Ashvio Ashvio changed the title NotebookProgressCallback doesn't work in databricks notebooks properly--should either be fixed or removed from trainer automatic cif it is a databricks runtime NotebookProgressCallback doesn't work in databricks notebooks properly--should either be fixed or removed from trainer automatically if it is a databricks runtime May 24, 2022
@sgugger
Copy link
Collaborator

sgugger commented May 25, 2022

Do you know which test we could do to easily detect if we are in a Databrick runtime?

@Ashvio
Copy link
Author

Ashvio commented May 29, 2022

Do you know which test we could do to easily detect if we are in a Databrick runtime?

From stackoverflow:

def isRunningInDatabricks(): Boolean = 
  sys.env.contains("DATABRICKS_RUNTIME_VERSION")

@sgugger
Copy link
Collaborator

sgugger commented May 31, 2022

sys.env is not something that exists. Did you mean os.environ?

@sgugger
Copy link
Collaborator

sgugger commented May 31, 2022

Could you try if the PR mentioned above does solve your problem?

@davidheryanto
Copy link
Contributor

Hi, just want to add, since I experienced the same issue in the past.

I believe the reason why the HTML produced by NotebookProgressCallback is not displayed properly is because Databricks (with runtime version prior to 11.0) is not using IPython kernel to execute the Python code.

There was a guide how to set Databricks to use IPython kernel. And in my experience, when this is set, the evaluation result table produced by NotebookProgressCallback is displayed properly.

https://web.archive.org/web/20211227103927/https://docs.microsoft.com/en-us/azure/databricks/notebooks/ipython-kernel

Most users, however, I believe will use the default setting i.e. not overriding Databricks default setting to specifically use IPython kernel. Therefore, the changes in this commit looks good.

However, in the most recent Databricks runtime version 11.0, IPython kernel is now the default Python code execution engine. Therefore, the HTML produced by NotebookProgressCallback I believe can be displayed correctly by default in Databricks runtime 11.x

https://docs.microsoft.com/en-us/azure/databricks/notebooks/ipython-kernel

I suggest, in addition to checking if this environment variable DATABRICKS_RUNTIME_VERSION is set, we should also check the version. If the version is 11.x, I believe it is ok to use the NotebookProgressCallback. It can show the table HTML output properly in my test.

image

@sgugger
Copy link
Collaborator

sgugger commented Jul 1, 2022

If you want to make a PR with the adjustment, I'll be happy to look at it!

@davidheryanto
Copy link
Contributor

If you want to make a PR with the adjustment, I'll be happy to look at it!

#17988

Thanks

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 a pull request may close this issue.

3 participants