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

Add a disconnect button for RayContext in notebooks #35507

Merged
merged 3 commits into from
May 30, 2023

Conversation

peytondmurray
Copy link
Contributor

@peytondmurray peytondmurray commented May 18, 2023

Why are these changes needed?

This PR reverts #35426, adding back in a nice button for disconnecting (i.e. calling ray.shutdown()) when ray.init() is called in a notebook.

Testing

The main issue with the previous disconnect PR is that checking for the ipywidgets soft dependency at run time introduces a small performance penalty which bumps some test suites up beyond their timeout upper bound, breaking them. This happens in tests unrelated to where these changes were made previously, making troubleshooting more difficult.

Here, I've introduced some optimizations to avoid this penalty wherever possible:

  • In ray.widgets.util.in_notebook, a significant performance penalty was previously being incurred by the try-import/except block upon each call. Now, we just check whether "IPython" in sys.modules is True before attempting an import. This is much faster as IPython is pre-loaded for IPython kernels. I've also added this optimization to repr_fallback_if_colab.
  • Secondly, ensure_notebook_deps now does dependency checking at function definition time, rather than on each function call.
  • I've also reworked the logic for detecting the current shell, since there's really only one situation where displaying widgets is okay (when the user is running Jupyterlab). In all other cases, we fall back to simple reprs.
  • The changes here also solve [Core] Import in Colab produces error #35490.

Here's the artifact from the CI run below loaded onto google colab:

image

The rendered output has no dashboard URL row (evidently this isn't available when run on colab), but the HTML repr looks okay. The only graphical issue that I see is that colab is doing something weird to the "RAY" text that is supposed to appear by the Ray logo. Here's what this looks like in Jupyterlab:

image

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@peytondmurray peytondmurray force-pushed the fix-disconnect-button-tests branch 2 times, most recently from 0e98b75 to 64646e9 Compare May 18, 2023 21:07
…ebooks (ray-project#34815)" (ray-project#35426)"

This reverts commit 45067ae.

Signed-off-by: pdmurray <peynmurray@gmail.com>
Signed-off-by: pdmurray <peynmurray@gmail.com>
@peytondmurray peytondmurray force-pushed the fix-disconnect-button-tests branch from 64646e9 to 3b2720b Compare May 19, 2023 22:56
Signed-off-by: pdmurray <peynmurray@gmail.com>
@peytondmurray peytondmurray force-pushed the fix-disconnect-button-tests branch from 3b2720b to 6cfcdf0 Compare May 20, 2023 03:07
@peytondmurray
Copy link
Contributor Author

I tested the 3.10 artifact in colab just now. Looks like we successfully detect that we are running in an incompatible shell, and do not display the widget version. Instead we successfully fall back to the _repr_html_ version without the disconnect button.

image

The only small caveat with this is that google colab messes up jupyterlab styles, but I think that's a secondary concern here.

@peytondmurray peytondmurray marked this pull request as ready for review May 23, 2023 20:50
@peytondmurray peytondmurray changed the title [WIP] Add a disconnect button for RayContext in notebooks Add a disconnect button for RayContext in notebooks May 24, 2023
@xieus
Copy link

xieus commented May 25, 2023

@rkooo567 @amogkam @bveeramani could you review this PR? Thanks.

@bveeramani
Copy link
Member

I'm not sure if I'm the best person to review this. @ericl could you take a look?

@bveeramani bveeramani removed their request for review May 25, 2023 19:02
Copy link
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

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

Stamping, as the previously broken tests now seem to be passing in this version.

Copy link
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

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

Stamping, as the previously broken tests now seem to be passing in this version.

@ericl ericl merged commit 6f2424f into ray-project:master May 30, 2023
@peytondmurray peytondmurray deleted the fix-disconnect-button-tests branch May 31, 2023 05:47
scv119 pushed a commit to scv119/ray that referenced this pull request Jun 16, 2023
This PR reverts ray-project#35426, adding back in a nice button for disconnecting (i.e. calling `ray.shutdown()`) when `ray.init()` is called in a notebook.

## Testing

The main issue with the previous disconnect PR is that checking for the `ipywidgets` soft dependency at run time introduces a small performance penalty which bumps some test suites up beyond their timeout upper bound, breaking them. This happens in tests unrelated to where these changes were made previously, making troubleshooting more difficult.

Here, I've introduced some optimizations to avoid this penalty wherever possible:
* In `ray.widgets.util.in_notebook`, a significant performance penalty was previously being incurred by the `try-import/except` block upon each call. Now, we just check whether `"IPython" in sys.modules` is `True` before attempting an import. This is _much_ faster as `IPython` is pre-loaded for IPython kernels. I've also added this optimization to `repr_fallback_if_colab`.
* Secondly, `ensure_notebook_deps` now does dependency checking at function definition time, rather than on each function call.
* I've also reworked the logic for detecting the current shell, since there's really only one situation where displaying widgets is okay (when the user is running Jupyterlab). In all other cases, we fall back to simple reprs.
* The changes here also solve ray-project#35490.

Here's the artifact from the CI run below loaded onto google colab:

![image](https://github.com/ray-project/ray/assets/14017872/c1852128-9dce-4744-8bbc-1ed641ed9cce)

The rendered output has no dashboard URL row (evidently this isn't available when run on colab), but the HTML repr looks okay. The only graphical issue that I see is that colab is doing something weird to the "RAY" text that is supposed to appear by the Ray logo. Here's what this looks like in Jupyterlab:

![image](https://github.com/ray-project/ray/assets/14017872/7592f5f0-46d7-4fa3-8996-86371d39f3e2)
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
This PR reverts ray-project#35426, adding back in a nice button for disconnecting (i.e. calling `ray.shutdown()`) when `ray.init()` is called in a notebook.

## Testing

The main issue with the previous disconnect PR is that checking for the `ipywidgets` soft dependency at run time introduces a small performance penalty which bumps some test suites up beyond their timeout upper bound, breaking them. This happens in tests unrelated to where these changes were made previously, making troubleshooting more difficult.

Here, I've introduced some optimizations to avoid this penalty wherever possible:
* In `ray.widgets.util.in_notebook`, a significant performance penalty was previously being incurred by the `try-import/except` block upon each call. Now, we just check whether `"IPython" in sys.modules` is `True` before attempting an import. This is _much_ faster as `IPython` is pre-loaded for IPython kernels. I've also added this optimization to `repr_fallback_if_colab`.
* Secondly, `ensure_notebook_deps` now does dependency checking at function definition time, rather than on each function call.
* I've also reworked the logic for detecting the current shell, since there's really only one situation where displaying widgets is okay (when the user is running Jupyterlab). In all other cases, we fall back to simple reprs.
* The changes here also solve ray-project#35490.

Here's the artifact from the CI run below loaded onto google colab:

![image](https://github.com/ray-project/ray/assets/14017872/c1852128-9dce-4744-8bbc-1ed641ed9cce)

The rendered output has no dashboard URL row (evidently this isn't available when run on colab), but the HTML repr looks okay. The only graphical issue that I see is that colab is doing something weird to the "RAY" text that is supposed to appear by the Ray logo. Here's what this looks like in Jupyterlab:

![image](https://github.com/ray-project/ray/assets/14017872/7592f5f0-46d7-4fa3-8996-86371d39f3e2)

Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants