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

Lazy-install altair and tenacity #9

Draft
wants to merge 4 commits into
base: stlite-1.32.2
Choose a base branch
from
Draft

Conversation

whitphx
Copy link
Owner

@whitphx whitphx commented Mar 14, 2024

I came up with this lazy-install idea.
However, with this naive implementation, looks like the drawback is larger then the benefit such as

  • The lazy-install runs without any notification but only a blank page, so the user experience gets worse.

@whitphx
Copy link
Owner Author

whitphx commented Mar 14, 2024

Loading time comparison

Run the following in local.

  1. Build the streamlit wheel for each ver.
  2. Start the sharing page (cd packages/sharing & yarn start)
  3. Open the page with code importing altair and printing a log: http://localhost:3000/#code=import%20altair;print(%22Loaded%22).
    • This executes the following code that imports altair which triggers the lazy-install and prints "Loaded" indicating the first view appears.
      import altair; print("Loaded")
  4. Measure the performance. We assume when the first view is displayed is when the "Loaded" log appears on the console.

Result

https://docs.google.com/spreadsheets/d/1igepmz2APbnApCHzgQUOTiuUZzlsKmWZ8QR_b5qUNP8/edit#gid=0

Baseline (stlite-1.32.1)

  • From initialization to the first view: 8.7~10.8s
  • Initial installation: 1.2~2.5s

This PR (5698b52)

  • From initialization to the first view: 8.9-9.4s
  • Initial installation: 0.93-1.0s

Assesment

This PR introduces a small improvement on the performance.
I think it's ok to merge this as there is no big drawback.

@whitphx whitphx changed the title Lazy-install Lazy-install altair and tenacity Mar 14, 2024
@whitphx
Copy link
Owner Author

whitphx commented Mar 14, 2024

TODO:

  • Check if the desktop app bundler works. It pre-installs all the requirements, so these lazy-installed packages also should be managed by that pre-installer.

@whitphx whitphx changed the base branch from stlite-1.32.1 to stlite-1.32.2 March 15, 2024 04:11
@whitphx whitphx changed the base branch from stlite-1.32.2 to stlite-1.32.2-2 April 4, 2024 11:57
@whitphx whitphx changed the base branch from stlite-1.32.2-2 to stlite-1.32.2 April 4, 2024 11:57
@lukasmasuch
Copy link

lukasmasuch commented Apr 4, 2024

Is the code execution continued after the lazy install or does it require a rerun?

The lazy-install runs without any notification but only a blank page, so the user experience gets worse.

Is there a way to somehow communicate to the kernel about this installation and trigger a progress toast? Maybe we could use window.postMessage communication. Streamlit even has a built-in command to trigger custom post messages:

https://github.com/streamlit/streamlit/blob/c47348cb10e54e069d0ea8df571b3959efc06bd3/lib/streamlit/platform.py#L23

Another alternative might be to actually just create a toast via st.toast during the installation.

@lukasmasuch
Copy link

lukasmasuch commented Apr 4, 2024

In general, I think it might not be worth adding this just for altair and tenacity. But I think this could be a very interesting feature when it is opened up to be a bit more generic lazy-install solution.

In the context of a playground, our ideal requirements would be to 1) have only a single file people are modifying (-> no requirements file) 2) have a very fast initial loading time (-> only have the min. number of dependencies pre-installed).

With these requirements, probably the best option to allow users to use a variety of packages in the playground without impacting the overall performance is probably by having a smart auto-install feature. That means, as soon as there is an unknown import, stlite would try to auto-install this package.

@lukasmasuch
Copy link

Btw. after some chatting with ChatGPT and Gemini there might be an alternative solution for the auto-install. E.g. maybe something like:

import sys
import subprocess
import importlib.util

def install_and_import(package):
    # Install & import the package
    return True

class AutoInstallFinder:
    def __init__(self):
        self.installed = set()

    def find_spec(self, fullname, path, target=None):
        if fullname in self.installed:
            return importlib.util.find_spec(fullname)

        spec = importlib.util.find_spec(fullname)
        if spec is not None:
            return spec  # Module found, no need to install

        if fullname not in self.installed and install_and_import(fullname):
            self.installed.add(fullname)
            return importlib.util.find_spec(fullname)

        return None  # Module not found and installation failed or not attempted

sys.meta_path.insert(0, AutoInstallFinder())

But not sure how well that works in pyiodide. I did not have the time yet to explore in more depth.

@whitphx
Copy link
Owner Author

whitphx commented Apr 5, 2024

Is the code execution continued after the lazy install or does it require a rerun?

Yes, it requires rerun.

  1. At the first run, an ImportError error occurs and it interrupts the run.
  2. The error is caught and lazy-installation is triggered.
  3. Rerun the code.

Is there a way to somehow communicate to the kernel about this installation and trigger a progress toast? Maybe we could use window.postMessage communication.

Yes, technically possible. It might be something like this:

  1. Make changes or monkey-patch on the Python code around the scriptrunner to dispatch an event on the lazy-install.
    • FYI, monkey-patch would be something like this, which injects a callback into AppSession._on_scriptrunner_event.
  2. Catch the error in the worker JS code and send the message to the main process.

I thought it's not worth doing as it would introduce too much complexity for limited supported packages, just same as your opinion, but now think it's not going to be so complex maybe?

In general, I think it might not be worth adding this just for altair and tenacity.

Also totally agree with your playground example.

an alternative solution for the auto-install.

Yea, I also thought about it using a custom finder and loader, but it shouldn't work in stlite/Pyodide because the package installer (micropip.install()) is async but the custom importer can neither await it nor be async. In your example, install_and_import has to await micropip.install() but it's not possible on Pyodide alaik.
(So, this technique should work on the normal Streamlit, btw. It can be something fun?)

FYI, such custom finder/importer approach is already used to support lazy-"import" (or lazy-patching) of altair here, while it can't be extended for lazy-"install" due to the async problem stated above. (and after I implemented this, I found this library for this purpose, btw).
Also FYI, I'm thinking of using this approach for this feature where stlite will support lazy-install of user-defined modules, not PyPI-distributed packages. I think it's possible because it can be synchronous.

@whitphx
Copy link
Owner Author

whitphx commented Apr 5, 2024

For the playground example you provided, I think there is another approach:
When the user requests to save the code, stlite inspects the new code and installs the imported packages by pyodide.loadPackagesFromImports.

The differences in this case are

  1. the packages to install are imported from the user-written code, not the files in the streamlit package, which can be analyzed by pyodide.loadPackagesFromImports.
  2. due to the first point, we can hook this process before saving the file or triggering the script_runner, which leaves the room where we can handle async installation method, await micropip.install().

@whitphx
Copy link
Owner Author

whitphx commented Apr 5, 2024

I came to think it's nice to implement such auto-package-installation as a part of stlite's API.
-> whitphx/stlite#857

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.

2 participants