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

feat(maidr.show): support interactive maidr plots within Jupyter Notebooks, VS Code, Google Colab, and Quarto #64

Merged
merged 13 commits into from
Aug 21, 2024

Conversation

dakshpokar
Copy link
Collaborator

@dakshpokar dakshpokar commented Aug 6, 2024

This PR adds support for native and VSCode Jupyter notebooks for maidr.show feature. This is done so by rendering the HTML code inside an iframe.

closes #60, #61, #78, and #79

@jooyoungseo
Copy link
Member

@dakshpokar Please make sure this PR supports Jupyter Notebook within VSCode as well. I will give a hint below. The following interactivity works in VSCode:

test.ipynb:

from ipywidgets import HTML, Layout
from IPython.display import display, Javascript

html = HTML("""
<div id='sound-navigator'>
    Use left and right arrow keys to navigate. Current position: <span id="position">0</span>
</div>
""", layout=Layout(height='50px'))

js = Javascript("""
require(['jquery'], function($) {
    var audioContext = new (window.AudioContext || window.webkitAudioContext)();
    var oscillator = null;
    var currentPosition = 0;

    function playSound(frequency) {
        if (oscillator) {
            oscillator.stop();
        }
        
        oscillator = audioContext.createOscillator();
        oscillator.type = 'sine';
        oscillator.frequency.setValueAtTime(frequency, audioContext.currentTime);
        
        oscillator.connect(audioContext.destination);
        oscillator.start();
        oscillator.stop(audioContext.currentTime + 0.2);  // Play for 200ms
    }

    $(document).on('keydown', function(event) {
        if (event.key === 'ArrowRight' || event.key === 'ArrowLeft') {
            if (event.key === 'ArrowRight') {
                currentPosition += 1;
            } else {
                currentPosition = Math.max(0, currentPosition - 1);
            }
            $('#position').text(currentPosition);
            
            // Play sound
            playSound(currentPosition * 100 + 200);  // Frequency increases with position
        }
    });

    // Ensure audio context is resumed on user interaction
    $('#sound-navigator').on('click', function() {
        if (audioContext.state === 'suspended') {
            audioContext.resume();
        }
    });
});
""")

display(html, js)
print("Click on the output area and use left/right arrow keys to navigate and hear sounds.")

@dakshpokar
Copy link
Collaborator Author

dakshpokar commented Aug 7, 2024

Professor @jooyoungseo I tested the iframe logic on Jupyter Notebook within VSCode. The interactivity is working with the iframe logic. This is also working for a normal Jupyter notebook.

@dakshpokar dakshpokar requested a review from jooyoungseo August 7, 2024 22:13
@dakshpokar dakshpokar marked this pull request as ready for review August 7, 2024 22:13
@dakshpokar dakshpokar requested a review from SaaiVenkat August 7, 2024 22:19
@jooyoungseo
Copy link
Member

@dakshpokar Some initial comments:

  1. Please use lowercase present tense verb after colon space (:) for your commits. For example,

feat(maidr.show): support native and VSCode Jupyter Notebooks

We will use a squash merge so you can make your PR title, following the conventional commits.

  1. Include your PR description for reviewers. Also at the end, you need to use closes #[ISSUE_NUMBER]` like below:

This PR does [the PR description]
...
closes #61

@dakshpokar
Copy link
Collaborator Author

dakshpokar commented Aug 7, 2024

Sure Professor, will address these comments. I will take care to include those for the next PRs

@dakshpokar dakshpokar changed the title fix: issue with interactivity in ipython notebooks feat(maidr.show): support native and VSCode Jupyter Notebooks Aug 7, 2024
@jooyoungseo
Copy link
Member

@dakshpokar I was wondering if this PR could potentially address #60 as well. Could you please test this PR in Google Colab?

@dakshpokar
Copy link
Collaborator Author

Sure thing, I will check this and will let you know.

@dakshpokar
Copy link
Collaborator Author

dakshpokar commented Aug 7, 2024

Professor @jooyoungseo this does address #60. I ran the example on Google Colab, here is link to a test notebook - https://colab.research.google.com/drive/1x3GM1IGj4Spdc5-v0FiXhalo-LgcJQVl?usp=sharing

Copy link
Collaborator

@SaaiVenkat SaaiVenkat left a comment

Choose a reason for hiding this comment

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

Hi @dakshpokar. This solution looks pretty simple, which is good. I thought of integrating using ipywidgets. That is because, we need an architecture compatible for all kinds of outputs, like GUI window, html, interactive shell, regular shell & script. I am not sure whether this handles all the use cases, but it does cover most of them. We can address scripting compatibility in a different PR for #55 , as it requires good amount of design discussion.

Here’s an overview of the review.

  • Investigate the root cause of why htmltools fails rendering with the JS, even though it internally supports. In that way, we can try to fix it without any workaround from our side.
  • In the meantime, discuss with the Professor @jooyoungseo regarding the urgency of the issue. If it is required to be fixed immediately, then modify the existing PR to include the following changes.
    • Instead of using the html version to embed the IFrame, we could use htmltools’s iframe tag. Also, we can embed it conditionally in the inject_plot() instead of the show()
    • Reuse _unique_id() to create the id of the IFrame.
    • Refactor the checking of the Interactive Shell into the Utils/Enviroment class as a static function, so that Maidr's responsibility won’t be over-scoped.
    • get_python() isn’t globally available in IPython, so we can use it’s internal logic to identify the interactive shell as below
        try:
            from IPython.core.interactiveshell import InteractiveShell
            return InteractiveShell.initialized() and InteractiveShell.instance() is not None
        except ImportError:
            return False

These are my opinions. Please feel free to add further comments on your thoughts.

cc: @jooyoungseo

@dakshpokar
Copy link
Collaborator Author

@SaaiVenkat Yes, simplicity was the idea. Thanks for this, I will look into the mentioned feedback.

@dakshpokar dakshpokar requested a review from SaaiVenkat August 15, 2024 00:28
@dakshpokar
Copy link
Collaborator Author

@SaaiVenkat I cleaned up some code as requested. However, I couldn't use HTML tools to render the iframe.

I tried to do so by following but the interactivity stops working with this -

tags.iframe(
   srcdoc=base_html.get_html_string(),
   id=random_id,
   style="width: 100%; height:100%",
)

There is also an alternative approach where I save the HTML file and then load it by passing file in src attribute. That works but not sure, we should go ahead with that -

def _inject_plot(plot: HTML, maidr: str) -> Tag:
  ....
  ....
  ....
  file = tags.html(....).save_html(random_id + ".html")

  return tags.iframe(src=file)

Copy link
Collaborator

@SaaiVenkat SaaiVenkat left a comment

Choose a reason for hiding this comment

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

@dakshpokar

Follow up

  • Since py-htmltools has issue rendering this, please create an issue against their repo stating the issue.

IFrame

  • I was able to bring the interactivity with this

            html = tags.html(
                ...
            )
    
            if Environment.is_interactive_shell():
                html = tags.iframe(
                    srcdoc=str(html.get_html_string()),
                    width="100%",
                    height="100%",
                    scrolling="auto",
                    style="background-color: #fff",
                    onload="this.style.height=this.contentWindow.document.body.scrollHeight + 'px';"
                )
    
            return html
    
    Screenshot 2024-08-15 at 12 51 54 PM
  • get_html_string() returns a HTML Tag element, not the direct string representation of it. But srcdoc expects a string to render properly. When given the HTML tag element, the conversion to string messes with the iFrame rendering.

Interactive Shell

  • For checking the python interactive shell, please move the functionality to the maidr/utils/Environment.py with the following contents

    class Environment:
        @staticmethod
        def is_interactive_shell() -> bool:
            """Return True if the environment is an interactive shell."""
            try:
                from IPython.core.interactiveshell import InteractiveShell
    
                return (
                    InteractiveShell.initialized()
                    and InteractiveShell.instance() is not None
                )
            except ImportError:
                return False
    
  • and import it

    ...
    from maidr.core.context_manager import HighlightContextManager
    from maidr.core.plot import MaidrPlot
    from maidr.utils.Environment import Environment
    

Refactoring

  • One more quick change, maidr/maidr.py and maidr/core/maidr.py is confusing. Could you please rename maidr/maidr.py to maidr/api.py for better understanding?

JavaScript Dependency

  • Also, regarding the JavaScript code, I would suggest to include them in the upstream repo because it isn't our responsibility to handle them.
  • We can set the id here like we do for maidr.id and let the upstream library take care of the resizing.
  • What's your thought about this?

Keybinding Issue

  • Professor @jooyoungseo, @ellvix, Jupyter notebook overrides all the general keybindings, be Chrome or any IDE.
  • So, some of our keybindings don't work. For example, we use b for Braille, but Jupyter uses it to create a new block.

cc: @jooyoungseo

@dakshpokar
Copy link
Collaborator Author

dakshpokar commented Aug 15, 2024

@SaaiVenkat Thanks for the feedback. That seems clean. I think moving interactive_shell detection logic to a different environment file will be a lot cleaner. I have also updated the file name to api.py in maidr/. Also, I don't need the resizing logic for iframe; given the approach that you suggested seems to resize the iframe properly.

I will be creating an issue with py-htmltools regarding this issue, will link here once I do that.

@dakshpokar dakshpokar requested a review from SaaiVenkat August 15, 2024 22:48
@SaaiVenkat
Copy link
Collaborator

@dakshpokar Did you test for #60 after the latest patch?

@jooyoungseo
Copy link
Member

Keybinding Issue

  • Professor @jooyoungseo, @ellvix, Jupyter notebook overrides all the general keybindings, be Chrome or any IDE.
  • So, some of our keybindings don't work. For example, we use b for Braille, but Jupyter uses it to create a new block.

cc: @jooyoungseo

@SaaiVenkat When I tested this PR last time, I was able to toggle on and off the braille within the iframe via B key. It worked in native Jupyter as well as the VSCode web view. @dakshpokar Do you see the key conflict issue on your end?

@ellvix -- [from maidr.js upstream] Could we override the global keybinding when the maidr plot area gets focused?

@jooyoungseo
Copy link
Member

@krishnanand5 -- You may want to review this PR as it is also related to your PR.

Copy link
Member

@jooyoungseo jooyoungseo left a comment

Choose a reason for hiding this comment

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

Please add Jupyter Notebooks (*.ipynb) to each of the example folders. You can recycle the existing example scripts.

@dakshpokar
Copy link
Collaborator Author

Sure Professor @jooyoungseo , will add the example notebooks.

style="background-color: #fff",
frameBorder=0,
onload="""
this.style.height = this.contentWindow.document.body.scrollHeight + 100 + 'px';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey Daksh,

When I worked on the py-shiny renderer, I faced a UI/UX issue when displaying the generated MAIDR plot on the shiny component. The major problem with the behaviour I observed on Shiny was that the overflow properties were triggered if pre-defined styles were used.

Can you share your findings on how Python notebooks renders the cells? After looking at the documentation, I can see that style customizations are allowed for Python notebooks. Does overflow get applied to the plot if pre-defined styles are employed?

Regards,
Krishna Anandan Ganesan.

@jooyoungseo jooyoungseo changed the title feat(maidr.show): support native and VSCode Jupyter Notebooks feat(maidr.show): support interactive maidr plots within Jupyter Notebooks, VS Code, Google Colab, and Quarto Aug 18, 2024
@jooyoungseo
Copy link
Member

@dakshpokar Have you tested your latest PR and ipynb examples within VSCode? The maidr plot interactivity is no longer working in VSCode webview on my end.

@dakshpokar
Copy link
Collaborator Author

dakshpokar commented Aug 19, 2024

Yes Professor @jooyoungseo. I tested each of the .ipynb examples. The interactivity is working for me in VSCode. Can you share the following things to help me debug this issue -

  1. Are you facing this issue only on VSCode or is the interactivity lost in Jupyter Notebook/Google Colab...
  2. VSCode Version
  3. Can you also try to run this Google Colab example -

https://colab.research.google.com/drive/1x3GM1IGj4Spdc5-v0FiXhalo-LgcJQVl?usp=sharing#scrollTo=wFx659JcKqWQ

Also, I added all the example notebooks apart from the stacked plot as I am facing an issue while running the provided example. While sharing the with @SaaiVenkat he mentioned that it was broken with the new API design. I will be creating a new issue for this.

Copy link
Collaborator

@SaaiVenkat SaaiVenkat left a comment

Choose a reason for hiding this comment

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

@dakshpokar , since iframe solves major integration issues even outside of interactive shell, do you think supporting iframe natively will be a better implementation? Like

        base_html = tags.html(
            tags.head(
                tags.meta(charset="UTF-8"),
                tags.title("MAIDR"),
                tags.link(
                    rel="stylesheet",
                    href="https://cdn.jsdelivr.net/npm/maidr/dist/maidr_style.min.css",
                ),
                tags.script(
                    type="text/javascript",
                    src="https://cdn.jsdelivr.net/npm/maidr/dist/maidr.min.js",
                ),
            ),
            tags.body(
                tags.div(plot),
            ),
            tags.script(maidr),
        )

        # If running in an interactive environment (e.g., Jupyter Notebook),
        # display the HTML content using an iframe to ensure proper rendering
        # and interactivity. The iframe's height is dynamically adjusted
        base_html = tags.iframe(
            srcdoc=str(base_html.get_html_string()),
            width="100%",
            height="100%",
            scrolling="auto",
            style="background-color: #fff",
            frameBorder=0,
            onload="""
                this.style.height = this.contentWindow.document.body.scrollHeight + 100 + 'px';
            """,
        )

        return base_html

@dakshpokar
Copy link
Collaborator Author

dakshpokar commented Aug 20, 2024

@SaaiVenkat Supporting native iframe integration is, in my opinion, a better strategy. Not only does running the HTML content in a sandboxed iframe guarantee correct rendering and interactivity across many settings, but it also isolates the content, lowering the possibility of conflicts with IPython Notebooks and strengthening the implementation.

I read this article that also shares how few JS libraries do this in Jupyter Notebooks - https://blog.ouseful.info/2021/09/30/a-simple-pattern-for-embedding-third-party-javascript-generated-graphics-in-jupyter-notebools/

But I am just thinking about what are the possible scenarios that could break something going with the native iframe integration. One that I can immediately think of is the resizing issue. If the content inside an iframe is changed in runtime then the iframe will not resize automatically, it will only resize on first load.

cc: @jooyoungseo

Copy link
Member

@jooyoungseo jooyoungseo left a comment

Choose a reason for hiding this comment

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

LGTM!

@SaaiVenkat SaaiVenkat merged commit 620ddc9 into xability:main Aug 21, 2024
12 checks passed
krishnanand5 pushed a commit to krishnanand5/py_maidr that referenced this pull request Aug 22, 2024
@dakshpokar
Copy link
Collaborator Author

I have created a issue related to this on py-htmltools' repository: posit-dev/py-htmltools#93

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.

feat: support maidr interactivity within Google Colab
4 participants