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

Allow creating a new page per client #85

Closed
29 tasks done
falkoschindler opened this issue Sep 13, 2022 Discussed in #61 · 36 comments
Closed
29 tasks done

Allow creating a new page per client #85

falkoschindler opened this issue Sep 13, 2022 Discussed in #61 · 36 comments
Assignees
Labels
enhancement New feature or request

Comments

@falkoschindler
Copy link
Contributor

falkoschindler commented Sep 13, 2022

Discussed in #61

Originally posted by falkoschindler August 26, 2022
Idea: ui.page can not only be used as context, but also accepts a Callable argument for re-creating the page whenever a client connects.
If the path if / or omitted, the page replaces the main page.

This is related to #6, where @me21 posted a draft for a PrivatePage, which I haven't looked into, yet.

After discussing several concepts, we tend to break with with ui.page contexts and move towards @ui.page decorators. This simplifies defining page factory functions and is similar to how Flask and FastAPI define routes. In the end, pages are more like routes rather than hierarchical UI elements. Thus defining them like routes makes a lot of sense.

Also, in the flavor of Flask, FastAPI or JustPy, pages should be "private" by default, i.e. without shared state between different clients. A page should only be "shared" when an optional decorator argument is set True.

This change has interesting consequences:

  1. Pages are no longer built using (possibly nested!) with expressions. Thus, the (internal) page_stack is obsolete. This removes one source of confusion.
  2. The pre-evaluation of ui.run is no longer needed. Since the main script primarily defines functions, we don't need to bother about re-evaluation in case of auto-reload being activated. This complies with the traditional way of running uvicorn and removes another confusing "feature" of NiceGUI.
  3. The decorated page functions can be async.

To keep NiceGUI very accessible for new Python developers and to reduce boilerplate code for simple single-page applications, we plan to automatically create an index page if there is none. So the NiceGUI hello-world example should remain unchanged:

from nicegui import ui

ui.label('Hello, world!')

ui.run()

This will be equivalent to the following:

from nicegui import ui

@ui.page('/')
def index_page():
    ui.label('Hello, world!')

ui.run()

TODOs

  • get rid of page_stack
  • remove context enter/exit from pages class
  • use a default main page for ui elements created "globally"
  • find better place to create default main page (and maybe remove code duplication with ui.page factory)
  • find way to set head and body html for each page
  • allow configuring pages through parameters for ui.page
  • find way to apply default configurations passed in ui.run to default main page (which already has been created)
  • reloading implicitly created index page causes "No page to load" and other server errors
  • show 404 page for routes which do not exist (or at least avoid 500 error)
  • allow on_connect handler with sessions for non-shared pages (see sessions demo on main.py)
  • allow passing page builder functions to ui.link and ui.open
  • test/fix await/run java script per page
  • remove pre-evaluation of ui.run
  • how to exclude costly imports without pre-evaluation of ui.run?
  • let JustPy serve Highcharts and Aggrid on demand
  • create routes for additional libraries on demand
  • introduce environment variable "MATPLOTLIB" to avoid its costly import (don't import if it's "false")
  • how to dynamically add a first custom element? routes to js files are added on startup.
  • how to dynamically add a first chart or table? js files are excluded from the template.
  • update documentation
  • fix memory leak for private pages -> this is a starlette issue
  • what to do with page routes which are created after startup?
  • rethink the strategy of page-reload after adding js dependencies
  • NiceGUI serves wrong template folder
  • dependencies are not found when running with reload=False
  • do we trigger too many reloads after adding dependencies? --> we do not reload anymore
  • how to add dependencies on private pages that loose their state when reloading? --> make excludes explicit again
  • handle new UI elements in an event callback without container (see Color Theming and JavaScript examples)
  • fix wrong title and favicon
@falkoschindler falkoschindler added the enhancement New feature or request label Sep 13, 2022
@rodja rodja pinned this issue Sep 13, 2022
@rodja
Copy link
Member

rodja commented Sep 14, 2022

I made some progress on the feature but there is still a lot to do (see list in the issue description). Noteworthy is our "requirement" to have a default page where elements are added. I only got it to work with a "shared" page (see cc50294). Because otherwise we need to re-evaluate the main script again and again.

@rodja
Copy link
Member

rodja commented Sep 14, 2022

My current test script:

from nicegui import ui
from uuid import uuid4

@ui.page('/individual_page')
def individual_page():
    ui.label(f'your individual page with uuid {uuid4()}')

@ui.page('/shared_page', shared=True)
def shared_page():
    ui.label(f'a shared page with uuid {uuid4()}')

# @ui.page('/')
# def index():
ui.link('individual page', '/individual_page')
ui.link('shared page', '/shared_page')

ui.run()

@falkoschindler
Copy link
Contributor Author

show 404 page for routes which do not exist

@rodja I guess you primarily want to avoid getting a 500 Server Error ("UnboundLocalError: local variable 'func_to_run' referenced before assignment"), as it is with the current implementation. Returning 404 would be new behavior, but probably better than redirecting to "/".

@rodja
Copy link
Member

rodja commented Sep 15, 2022

Returning 404 would be new behavior, but probably better than redirecting to "/".

@falkoschindler exactly. If it is much simpler to avoid the 500 Server Errors be redirecting to /, we could create a new issue for the 404 page. But I guess it will be an equally amount of work...

@falkoschindler
Copy link
Contributor Author

@rodja Here you stopped calling jp.justpy() which creates the default route Route("/{path:path}", func_to_run, last=True, name='default'). We can simply add this line to our startup().

@falkoschindler
Copy link
Contributor Author

remove pre-evaluation of ui.run

I think we are loosing the ability to exclude costly imports if we remove the pre-evaluation... 🤔

@falkoschindler
Copy link
Contributor Author

...and it's getting tricky to instantiate a default page without ui.page decorator, because it should consider the title argument in ui.run, which is not known when creating the first elements. We'll need to create a page first and update attributes like title, classes, dark etc. later.

@falkoschindler
Copy link
Contributor Author

falkoschindler commented Sep 15, 2022

how to exclude costly imports without pre-evaluation of ui.run?

exclude played a roll in three different cases:

  1. Custom views with additional JavaScript libraries like three.js: Routes to such libraries are created at import time, but could easily be postponed. We could add them at startup if any instance of such element has been created. So we automatically exclude unused libraries and don't need the exclude argument. This should also work dynamically when creating an element interactively.
  2. Highcharts and Aggrid: These libraries are configured by setting environment variables. Right now I'm not sure when these variables are accessed. Maybe we can set them in ui.run. But maybe we have to require setting them before starting NiceGUI.
  3. Costly import of Matplotlib: Here we also might need an environment variable. Everything else comes too late for the import.

After all, NiceGUI might be even nicer, because most JS libraries will be automatically served on demand. And there are only a few optional exclusions left as environment variables.


Update:

Although the variables "HIGHCHARTS" and "AGGRID" are already read when importing JustPy, they are only written into a dictionary template_options for later use during a GET request. So we should be able to update this dictionary during ui.run. This leaves only one remaining environment variable for Matplotlib.

@falkoschindler
Copy link
Contributor Author

I thought I found a solution for injecting new dependencies into running applications. But it's probably more complicated than running

let script = document.createElement("script");
script.src = "{src}";
document.body.prepend(script);

and calling page.update(). When the script is large (like highcharts.js), it might not be fully loaded when the page update is called.

So I got a new, much simpler idea: In the rare case of dynamically introducing a new dependency, NiceGUI can simply trigger a location.reload(). Since the dependency is now part of the template, it is automatically served and we don't have to inject it ourselves.

@falkoschindler
Copy link
Contributor Author

falkoschindler commented Sep 16, 2022

I think we have a memory leak for private pages:

#!/usr/bin/env python3
import gc
import os
import psutil
from nicegui import ui

@ui.page('/')
async def index_page():
    process = psutil.Process(os.getpid())
    gc.collect()
    ui.label(f'memory: {process.memory_info().rss:,} bytes')

ui.run()

The memory usage keeps growing when refreshing the page.

@falkoschindler
Copy link
Contributor Author

Ok, the memory usage is similar for a plain JustPy app:

#!/usr/bin/env python3
import gc
import os
import justpy as jp
import psutil

def hello_world():
    wp = jp.WebPage()
    process = psutil.Process(os.getpid())
    gc.collect()
    wp.add(jp.Div(text=f'memory: {process.memory_info().rss:,} bytes'))
    return wp

jp.justpy(hello_world)

@me21
Copy link
Contributor

me21 commented Sep 16, 2022

See my implementation in #6. There's a line in on_disconnect_handler: self.individual_pages.pop(session_id).remove_page() which should reclaim used memory (didn't test though)

@falkoschindler
Copy link
Contributor Author

@me21 Ok, but since I'm not holding the pages in a dictionary like you did, I'd expect the garbage collector to reclaim the memory. And especially JustPy's the bare hello-world example shouldn't have this issue.

I opened an issue over there: justpy-org/justpy#547

@WolfgangFahl
Copy link

Route("/{path:path}", func_to_run, doesn't exist with the path param any more.

@me21
Copy link
Contributor

me21 commented Sep 16, 2022

@falkoschindler remove_page is a JustPy method, not mine. Try calling it.

@falkoschindler
Copy link
Contributor Author

Looks like an issue with starlette: tiangolo/fastapi#4649

@me21 I tried, but it doesn't work. Apparently JustPy isn't the problem anyway, since its jp.WebPage.instances and other lists and dictionaries don't grow.

@rodja
Copy link
Member

rodja commented Sep 17, 2022

I think the code is ready for merging and hence I created the pull request #86. Please merge it, if you agree @falkoschindler.

@rodja
Copy link
Member

rodja commented Sep 17, 2022

Ok, the pull request has been merged. But maybe we have still some work to do on main. Currently I'm struggling with the dependency-reload at

if asyncio.get_event_loop().is_running():
# NOTE: if new dependencies are added after starting the server, we need to reload the page on connected clients
async def reload() -> None:
for page in get_current_view().pages.values():
assert isinstance(page, Page)
await page.await_javascript('location.reload()')
create_task(reload())

It seems that the page.await_javascript never returns because the location.reload() drops the connection. While investigating the issue I realised that the libs like tree.js etc are also working if I remove the code completely. @falkoschindler maybe we only need to reload the page if dependencies are added after the page is created, not after startup has completed?

@rodja
Copy link
Member

rodja commented Sep 17, 2022

And it seems we also have a problem with pages which are created after startup.

@rodja
Copy link
Member

rodja commented Sep 19, 2022

I've started with selenium tests with the pull request #88. The motivation was to document a simple reproduction for page builders which are created after startup (the first user.open() calls ui.run() internally):

async def test_creating_new_page_after_startup(user: User):
user.open()
@ui.page('/late_page')
def page():
ui.label('page created after startup')
user.open('/late_page')
user.should_see('page created after startup')

Real world scenarios can be found in RoSys. For example the KPI page: it should only be available if the final application calls the constructor. Which is normally after the startup of NiceGUI.

@rodja
Copy link
Member

rodja commented Sep 21, 2022

Ok, the problem with page generators added after server start is fixed with d5e8e12. I'll try to find a minimal reproduction for the dependency-reload problem I saw a few days ago.

@rodja
Copy link
Member

rodja commented Sep 21, 2022

The exception for the dependency-reload looks like this:

2022-09-21 10:51:08.985 [ERROR] nicegui/task_logger.py:54: Task raised an exception
Traceback (most recent call last):
  File "/Users/rodja/Projects/nicegui/nicegui/task_logger.py", line 48, in _handle_task_result
    task.result()
  File "/Users/rodja/Projects/nicegui/nicegui/routes.py", line 92, in reload
    await page.await_javascript('location.reload()')
  File "/Users/rodja/Projects/nicegui/nicegui/page.py", line 122, in await_javascript
    raise TimeoutError('JavaScript did not respond in time')
TimeoutError: JavaScript did not respond in time

@rodja
Copy link
Member

rodja commented Sep 21, 2022

It's quite simple to reproduce the error:

@ui.page('/')
def page():
    ui.keyboard()

ui.run()

This will print the TimeoutError to the console after startup. I also created a selenium test (which is still on the pytest branch):

def test_automatic_loading_of_keyboard_dependency(user: User):
@ui.page('/')
def page():
ui.keyboard()
user.open('/')
assert any(('keyboard.js' in s) for s in user.get_attributes('script', 'src'))
user.sleep(2) # NOTE we need to sleep to wait for the js error to be printed (start pytest with -s to see it)

@falkoschindler
Copy link
Contributor Author

Strange. The TimeoutError can be easily avoided using run_javascript instead of await_javascript. But with shared=True the page is not loading correctly and remains blank.

from nicegui import ui

@ui.page('/', shared=True)
def page():
    ui.label('hello world')
    ui.keyboard(on_key=lambda e: print(e))

ui.run()

@falkoschindler
Copy link
Contributor Author

Oh, I see! The get_current_view() in this line

for page in get_current_view().pages.values():
has a significant side effect: It is usually used to auto-create the index page. Here this messes up everything, because the index page is created explicitly with the @ui.page decorator. I'll try to avoid using get_current_view in this case.

@falkoschindler
Copy link
Contributor Author

@rodja I think I fixed the issue with the above-mentioned get_current_view in route.py. But one test is still red, unveiling a fundamental problem with dynamically creating elements.

In line

ui.button('activate keyboard', on_click=lambda: ui.keyboard())

it is unclear how ui.keyboard should know where to place itself. The view_stack is empty at this point, which causes several problems:

  • add_dependencies does not know which page to reload.
  • The Element initializer does not know which parent view to assign.
  • Even worse: get_current_view() creates a new index page.

@falkoschindler
Copy link
Contributor Author

falkoschindler commented Sep 23, 2022

There is another issue with reloading the page for new dependencies.

The following example should add a joystick when pressing the button:

from nicegui import ui

@ui.page('/')
def index():
    def add():
        with row:
            ui.joystick()
    row = ui.row()
    ui.button('Add', on_click=add)

ui.run()

But the joystick appears only after pressing the second time. This is unexpected, but explainable:
The button click adds a joystick to the DOM. The new dependency triggers a page reload. Because the page is private, the client obtains a new instance without the joystick. The next click adds a joystick again. This time, the dependency already exists, so there is no reload and the joystick remains visible.

The question is: How to add dependencies on private pages that loose their state when reloading.

@falkoschindler
Copy link
Contributor Author

Dependencies are not found when running with reload=False:

from nicegui import ui

ui.joystick()

ui.run(reload=False)

nipple.min.js and joystick.js are not found (HTTP 404).

@falkoschindler
Copy link
Contributor Author

raise exception instead of reloading the page

This does not work, since a simple private page won't be able to add a dependency:

@ui.page('/')
def page():
    ui.keyboard()

I guess we want to support such a trivial example.

@falkoschindler
Copy link
Contributor Author

I removed the automatic dependency management, because I think it's almost impossible to accomplish for private pages. When a new Vue component (e.g. "keyboard") is added to the DOM, we might be able to make connected clients load the required libraries (e.g. "keyboard.js"). But we would also need to trigger Vue to interpret the new DOM element (e.g. "") accordingly. This is pretty hard - at least without deeper understanding of Vue and JustPy. That's why I decided to simply reload the page for connected clients. But private pages loose their state, making this approach impractical.

Instead I now re-introduced the exclude argument for ui.run that allows the user to explicitly specify a number of UI elements that should be excluded from the app. In ui.run it's too late to impact the import process. But we can simply create routes for the corresponding libraries that yield empty response bodies. This way we save traffic without messing with the import process.

@me21
Copy link
Contributor

me21 commented Sep 23, 2022

I remember the import of some Python packages on the Beaglebone Black computer was heavy (up to 50 seconds on startup).
Would it be possible to exclude certain Python packages from import?

@falkoschindler
Copy link
Contributor Author

falkoschindler commented Sep 23, 2022

@me21

I remember the import of some Python packages on the Beaglebone Black computer was heavy (up to 50 seconds on startup). Would it be possible to exclude certain Python packages from import?

Avoiding the costly Matplotlib import is now controlled with a new environment variable "MATPLOTLIB". You can set it to "false" to disable it:

MATPLOTLIB=false python3 your_script.py

@falkoschindler
Copy link
Contributor Author

On branch https://github.com/zauberzeug/nicegui/tree/auto-context I'm experimenting with finding the right parent for new UI elements. But it easily fails with async functions like the following example. "A" and "B" should be added to separate cards, but are mixed up completely:

async def add_a():
    await asyncio.sleep(0.1)
    ui.label('A')

async def add_b():
    await asyncio.sleep(0.1)
    ui.label('B')

with ui.card():
    ui.timer(1.0, add_a)

with ui.card():
    ui.timer(1.1, add_b)

@falkoschindler
Copy link
Contributor Author

In ee0fb9b I introduced separate view stacks per task. This solves the problem with async UI manipulation.

@falkoschindler
Copy link
Contributor Author

@rodja It looks like this epic issue is finally ready for review, integration tests in some productive contexts and release.

@rodja
Copy link
Member

rodja commented Sep 24, 2022

@rodja rodja closed this as completed Sep 24, 2022
@rodja rodja unpinned this issue Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants