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

Use watchfiles library for detecting changed files #5894

Merged
merged 15 commits into from
Jan 12, 2024

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Nov 19, 2023

Using our own periodic callback handler sucked and turned out to be somewhat slow/unreliable. The watchfiles library on the other hand works exceptionally well, is very fast (since it is written in Rust) and simplifies our code. I think the dependency is worth it on balance, particularly since I have another usecase in mind.

@MarcSkovMadsen You may want to experiment.

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Nov 19, 2023

This is such a nice DevEx improvement

Works on Linux Jupyterhub

python==3.10

works-on-linux-jupyterhub.mp4
  • Even works if an exception is raised initially

@MarcSkovMadsen
Copy link
Collaborator

Issue: Does not pick up change in other file.

If I import from another file it does not pick up the change. It only reloads.

does-not-pick-up-other-change.mp4
import panel as pn
import pandas as pd
import datetime as dt
from other import spacer

pn.extension("tabulator", design="material", css_files=["https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.4.2/css/all.min.css"])
df = pd.DataFrame({
    'int': [1, 2, 34],
    'float': [3.14, 6.28, 9.42],
    'str': ['A', 'B', 'C'],
    'bool': [True, False, True],
    'date': [dt.date(2019, 1, 1), dt.date(2020, 1, 1), dt.date(2020, 1, 10)],
    'datetime': [dt.datetime(2019, 1, 1, 10), dt.datetime(2020, 1, 1, 12), dt.datetime(2020, 1, 10, 13)]
}, index=[1, 2, 3])

button_table = pn.widgets.Tabulator(df, buttons={
    'print': '<i class="fa fa-print"></i>',
    'check': '<i class="fa fa-check"></i>'
})

pn.Column(button_table, spacer).servable()
import panel as pn

spacer = pn.Spacer(height=200, width=200, styles={"background": "salmon"})

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Nov 19, 2023

Issue: Does not pick up change in package installed for editing

Most times I'm working on a package which contains one or more apps. My package is installed with pip install -e .. Its not very productive if the Panel autoreload does not pick up changes in my package.

If I try this with Panel it does not seem to work. I would expect the page to reload when I hard code the Tabulator width.

panel-autoreload-not-working-for-package.mp4

@philippjfr
Copy link
Member Author

The logic was a little off, so the above issues should be fixed now.

Copy link

codecov bot commented Nov 19, 2023

Codecov Report

Attention: 75 lines in your changes are missing coverage. Please review.

Comparison is base (445ff15) 84.70% compared to head (2847e7d) 82.95%.
Report is 9 commits behind head on main.

Files Patch % Lines
panel/io/reload.py 18.57% 57 Missing ⚠️
panel/tests/io/test_reload.py 29.41% 12 Missing ⚠️
panel/io/server.py 33.33% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5894      +/-   ##
==========================================
- Coverage   84.70%   82.95%   -1.76%     
==========================================
  Files         296      298       +2     
  Lines       44320    44620     +300     
==========================================
- Hits        37542    37015     -527     
- Misses       6778     7605     +827     
Flag Coverage Δ
ui-tests 38.40% <20.18%> (-2.33%) ⬇️
unitexamples-tests 72.27% <31.19%> (-0.34%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hoxbro
Copy link
Member

hoxbro commented Nov 19, 2023

I will be somewhat careful with adding watchfiles as a required dependency, as it is not absolutely needed for running Panel.

Also, as it is a rust app, it can mean we can't support new minor versions of Python before a new app release has been compiled with the ABI. As Panel has become a dependency of many of our other packages, it will be harder to test other packages.

It will also take time before it is published on the main / default conda channel. As far as I can see here, the latest version only supports up to Python 3.10.

Lastly (though very minor). This adds an extra 1.2 MB, which is unnecessary for production apps in a Docker container or running an app in Pyoidide.

another usecase in mind

My points above can change depending on the other use cases.

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Nov 19, 2023

Good considerations @hoxbro . Maybe its worth considering whether the built in autoreload can be kept but watchfiles used if it is installed? I can see this is the approach taken by uvicorn. https://www.uvicorn.org/settings/#reloading-without-watchfiles.

Version 0.21 supporting python 3.12 seems to have been released on pypi since october https://x.com/samuel_colvin/status/1713131500592316758?s=20

@philippjfr
Copy link
Member Author

Thanks @hoxbro, we should definitely carefully evaluate all new dependencies. Overall none of that concerns me overly much.

Also, as it is a rust app, it can mean we can't support new minor versions of Python before a new app release has been compiled with the ABI.

For PyPI this doesn't seem a major issue, watchfiles is a very popular package with 250k downloads a day and is used by various major libraries and it's used heavily at Pydantic (the company) so I trust Samuel to stay on top of it.

It will also take time before it is published on the main / default conda channel. As far as I can see here, the latest version only supports up to Python 3.10.

We have control over this, we can ask for the recipe to be updated if/when we needed so this doesn't concern me very much. In fact Anaconda in general should make sure that we update Samuel's projects in a timely manner because he has a strong voice in the community. It is true that this is yet another thing for us to track though.

Lastly (though very minor). This adds an extra 1.2 MB, which is unnecessary for production apps in a Docker container or running an app in Pyoidide.

1.2 MB is a non-issue from my perspective.

On balance I'd rather offer users a uniform experience but let's mull it over a little bit.

@MarcSkovMadsen
Copy link
Collaborator

Is it possible to White or blacklist dependencies for Pyodide?

setup.py Outdated Show resolved Hide resolved
@samuelcolvin
Copy link

Hi, author of watchfiles here, pleased to hear you're thinking of using watchfiles.

Watchfiles is on conda forge as well as anaconda proper, bit it looks like the pr for the most recent release is failing, conda-forge/watchfiles-feedstock#23 I'll try to get this merged asap.

As a company we use watchfiles (via uvicorn) ever day, so we'll definitely continue to maintain the package. It's pretty stable so all it needs is updates for new versions of python.

@philippjfr
Copy link
Member Author

Thanks @samuelcolvin, really appreciate you chiming in. Really appreciate all your work. I'll start the process of getting watchfiles updated on conda defaults independently of any decision we make here.

@philippjfr
Copy link
Member Author

philippjfr commented Nov 30, 2023

Quick update here, thanks to the great conda-forge folks watchfiles 0.21 is now uploaded there and I was able to get an update on conda defaults merged as well. For now I will make watchfiles optional and issue a FutureWarning to indicate that eventually we will drop the old autoreload code and rely entirely on watchfiles.

setup.py Outdated Show resolved Hide resolved
@maximlt
Copy link
Member

maximlt commented Nov 30, 2023

I looked a little on Github and watchfiles is definitely a direct dependency of a number of frameworks. As Panel users are not used to install extras to get some additional functionalities (probably because we rely on conda a lot and conda doesn't the exact same feature and setting something equivalent is a bit of a pain), I agree a warning is sensible right now and hope we can soon make watchfiles a direct dependency.

panel/io/reload.py Outdated Show resolved Hide resolved
panel/io/reload.py Outdated Show resolved Hide resolved
panel/io/reload.py Outdated Show resolved Hide resolved
panel/io/reload.py Outdated Show resolved Hide resolved
panel/io/reload.py Show resolved Hide resolved
Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

I just tested this PR serving (panel serve foo.py --autoreload --show) a very simple script and after a couple of changes and saves ('test1', 'test2', 'test3', etc.), the page no longer reloads. This bug occurs with and without watchfiles.

import panel as pn

txt = 'test1'

pn.panel(txt).servable()

@philippjfr
Copy link
Member Author

Thanks @maximlt! This indeed has been a problem with autoreload since the very beginning. The issue is that autoreload relies on sending an update to the Location object, but for it to have an effect this event has to be sent after the page is fully loaded. I've made two changes to improve the handling:

  1. We now only run only a single autoreload handler per server (previously they were created per session which added unnecessary overhead)
  2. If a session is not loaded we now register a 'document_ready' event which will trigger the reload which means the page will reload once it is ready.

@philippjfr
Copy link
Member Author

pre-commit.ci autofix

@philippjfr
Copy link
Member Author

Took me weeks to figure out but I finally realized that when you shut down a server you briefly have to restart the event loop so that watchfiles can correctly catch the stop event and shut down the thread, otherwise you end up with a zombie thread. This should now finally be ready to merge.

@philippjfr philippjfr merged commit 0482e43 into main Jan 12, 2024
13 of 15 checks passed
@philippjfr philippjfr deleted the autoreload_watchfiles branch January 12, 2024 14:49
@MarcSkovMadsen
Copy link
Collaborator

I noticed on windows with panel==1.3.6 the jupyter panel preview is really slow to reload. I hope this PR solves the issue.

autoreload-slow-windows.mp4

@MarcSkovMadsen
Copy link
Collaborator

Its actually quite slow on linux too.

jupyter_panel_preview_slow_linux.mp4

(I don't remember it like that ??)

@philippjfr
Copy link
Member Author

It's been relatively slow for a while. To make it robust we have to start a new kernel for each session. We might be able to look into reusing a kernel for reloads though.

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Jan 18, 2024

Would it be an idea to use autoreload by default but add a button to manually "restart the preview" (i.e. kernel) if something goes wrong?

@philippjfr
Copy link
Member Author

Will have to look into what's feasible, but note that this is entirely separate from "autoreload" functionality. The autoreload is triggered directly by JupyterLab file content manager events.

@philippjfr
Copy link
Member Author

Seems very likely that this is possible, could you open an issue?

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.

5 participants