Add style checks and the minimal plugins UI #35
Conversation
I agree, installing plugins from the UI should be a WASM only thing |
I think we shouldn't add the plugins to the downloads. It is up to the user to remember to install all necessary plugins (although we should add error popups for missing plot types etc.). And if the user forgets, then they can just reupload the saved state after installing the necessary plugins. |
|
That would be an option … I’m not sure how easy it is to restart plotly and a dash server but we could try that. If so, there should be an option to install many plugins, list the ones which are installed but not loaded, and then provide a big red button with a warning confirmation message to reaload, but with the recommendation to download any plots first. However, supporting dynamically loading in plugins might be a bit neater and provide a nicer user experience |
Ok, we can do that. In that case, it might be nice to still remember the list of loaded plot plugins so that if there is an error loading a plot we can point to the plugin that is missing. |
|
@Aggrathon @TanakaAkihiro I've now finished a minimal implementation that I'd like to land so that the remaining work can be distributed. What works:
What is missing but should be implemented in follow-up PRs (and probably not by me since I won't have the time):
Since all of these are quality-of-life improvements but don't include any required functionality, I'd propose merging this PR and #34. |
|
That is a possibility and would simplify some code, though it would still be nice to have a list of loaded plugins in the non-WASM version.
If we hide the plugins in non-WASM, this issue is resolved in my opinion. We should provide uploads in WASM unconditionally, but not hide them right now because it serves as an implementation reminder.
Sounds good to me.
This is a fundamental limitation of our plugin discovery at the moment. We only know about plugins that provide us with the known xiplot hooks. While we could list every installed PyPi plugin, I don't think this would be useful. What could be done is to list all entry point targets, derive the package names from that, and then list
I don't like that this is effectively a silent plugin at the moment, i.e. installing it has an effect but we cannot detect really detect it, unless we special-case the plugin discovery to check for parquet support. I'd rather us publish a wrapper plugin that just imports the dependency. The bug fix is out of scope right now.
Sounds good to me, out of scope for now. |
A list of installed plugins in the non-WASM version is a nice touch. However, personally I don't think it is that valuable, especially if removing it would lead to improved UI and, possibly, a better list for the WASM version. |
…round bug in fastparquet
I've now moved the list to the settings menu if the plugins tab is not loaded because loading new plugins is unsupported. The problem with a better plugin list, which includes installed packages that don't register any hooks, is that we'd have to diff the micropip list after an install from before the launch to get all packages. This would include the lazy sklearn load, and any dependencies any plugin might have. I think it would be far too cluttered too quickly. |
|
The bug is in fastparquet, but I added a workaround for it. Essentially, I subclass |
Oh, that ended up being more convoluted than expected (and in hindsight maybe not that necessary to work around). But the solution you found is pretty cool! |
Leaving improvements to the plugin list for future work is okay, but it already looks pretty great (one nitpick would be to parse the path with What I would have done is have a dumb list that gets appended to whenever the user clicks "install" (after a successful install just add whatever was in the input, therefore ignoring dependencies), but this would not work for the non-WASM list. |
|
@Aggrathon @TanakaAkihiro This PR is now ready for final review and to be merged. |
|
I tried having two "recommended" plugins in the WASM branch, but as soon as I loaded one of them the list of recommended plugins was cleared. This is not a show stopping bug (since we don't have multiple recommended plugins) but it should at least be mentioned for future work. |
That is a bug. In the process I discovered another issue and fixed both. Thanks for catching it! I also renamed the |
|
I've found a major issue. Any dynamically added callback is not registered on the client side. Meaning any plugin type requiring callbacks, such as plots, do not work. Below is a simple modification to the test plugin that should demonstrate this: import pandas as pd
from dash import html, Input, Output, MATCH
def plugin_load():
def read(data):
print("Reading test data")
return pd.DataFrame({"x": [1, 2, 3]})
return read, ".test"
def plugin_write():
def write(df, file):
print("Writing test data")
return write, ".test", "example/none"
def create_global():
return html.Div("TEST PLUGIN", style={"display": "none"}, id="test-plugin-global")
def register_callbacks(app, df_from_store, df_to_store):
print("The test-plugin has registered a callback")
class Plot:
@classmethod
def name(cls) -> str:
return " TEST PLUGIN"
@staticmethod
def register_callbacks(app, df_from_store, df_to_store):
@app.callback(
Output({"index": MATCH, "type": "test_counter"}, "children"),
Input({"index": MATCH, "type": "test_button"}, "n_clicks"),
)
def counter(num):
return f"{num} clicks"
print("The test-plugin plot has registered a callback")
@staticmethod
def create_new_layout(index, df, columns, config=dict()):
return html.Div(
[
html.Button(["Test Plot"], id={"index": index, "type": "test_button"}),
html.Span(["No clicks"], id={"index": index, "type": "test_counter"}),
]
) |
|
@Aggrathon Well dang. I tried for a while to get it to work, but it doesn't (the problem is that the frontend would need to refetch the "/_dash-dependencies" endpoint to get the new callbacks - I have no idea how to force it to do that though). I have figured out a way to do the page reload though. So my suggestion is to remove all dynamic plugin update code, to track which plugins have been loaded since the last reload and are thus not accessible yet, and to give a big reload button for them. We should probably also disallow the read and write plugins from working immediately, just for consistency. |
|
I'll implement this next week. |
|
@Aggrathon I've now implemented the manual reload variant and stripped out the code for the dynamic plugin loads. I hope that this version is now ready to be merged. |
Co-authored-by: Anton Björklund <[email protected]>
Co-authored-by: Anton Björklund <[email protected]>
Adds style checks to CI with black, isort, and flake8
Fixes any style issues
Implements plugin loading into the playground
TODOs:
Future work: