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

Add pn.Feed to allow buffering of feed objects #6031

Merged
merged 38 commits into from
Feb 4, 2024
Merged

Add pn.Feed to allow buffering of feed objects #6031

merged 38 commits into from
Feb 4, 2024

Conversation

ahuang11
Copy link
Contributor

@ahuang11 ahuang11 commented Dec 12, 2023

Closes #6021

Sets up the plumbing to add a Logs layout, for the internal _chat_log inside ChatFeed.

See bottom for demo.

@ahuang11 ahuang11 changed the title Add logs plumbing Add pn.Log to allow truncating number Dec 12, 2023
@ahuang11 ahuang11 changed the title Add pn.Log to allow truncating number Add pn.Log to allow truncating entries Dec 12, 2023
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

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

Comparison is base (075b5ae) 84.19% compared to head (01c4538) 82.65%.
Report is 12 commits behind head on main.

Files Patch % Lines
panel/layout/feed.py 31.73% 71 Missing ⚠️
panel/models/feed.py 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6031      +/-   ##
==========================================
- Coverage   84.19%   82.65%   -1.55%     
==========================================
  Files         301      305       +4     
  Lines       45200    45419     +219     
==========================================
- Hits        38058    37542     -516     
- Misses       7142     7877     +735     
Flag Coverage Δ
ui-tests 38.54% <53.48%> (-2.18%) ⬇️
unitexamples-tests 71.71% <30.81%> (-0.12%) ⬇️

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.

panel/models/layout.py Outdated Show resolved Hide resolved
panel/models/column.ts Outdated Show resolved Hide resolved
panel/models/column.ts Outdated Show resolved Hide resolved
@hoxbro
Copy link
Member

hoxbro commented Dec 12, 2023

You likely need to update index.ts

@ahuang11
Copy link
Contributor Author

Hmm getting closer, but maybe I'm rebuilding it wrong or I need another command besides panel build panel
image

@hoxbro
Copy link
Member

hoxbro commented Dec 12, 2023

Try running panel bundle --all --verbose, hard refresh the browser and if you see no errors, update

this.define<Log.Props>(({ Int }) =>
    min_visible: [Int, 10],
}));

@ahuang11
Copy link
Contributor Author

You're magical :D that worked

@ahuang11
Copy link
Contributor Author

ahuang11 commented Dec 13, 2023

Works pretty well now~

import panel as pn
pn.extension()

def prepend(event):
    log.insert(0, log[0].object - 1)

def insert(event):
    log.insert(50, len(log))

def append(event):
    log.append(len(log))

def replace(event):
    log.objects = [i for i in range(30)]

log = pn.Log(*[i for i in range(100)], height=200, auto_scroll_limit=100, width=500)
prepend_button = pn.widgets.Button(name='Prepend', button_type='primary', on_click=prepend)
insert_button = pn.widgets.Button(name='Insert', button_type='primary', on_click=insert)
append_button = pn.widgets.Button(name='Append', button_type='primary', on_click=append)
replace_button = pn.widgets.Button(name='Replace', button_type='primary', on_click=replace)
pn.Row(pn.Column(prepend_button, insert_button, append_button, replace_button), log).servable()
Screen.Recording.2023-12-13.at.3.10.43.PM.mov

Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

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

I'm fine with this approach, just wondering at what point it makes sense to render only the currently visible models plus some buffer and then display a loading indicator if they aren't fetched quick enough. For large numbers of messages this will significantly reduce serialization time.

panel/models/log.ts Outdated Show resolved Hide resolved
@ahuang11
Copy link
Contributor Author

ahuang11 commented Dec 13, 2023

That sounds like a good idea. How do I subset the number of children rendered?

import panel as pn

pn.extension(sizing_mode="stretch_width")


def prepend(event):
    log.insert(0, log[0].object - 1)


def insert(event):
    log.insert(50, len(log))


def append(event):
    log.append(len(log))


def replace(event):
    log.objects = [i for i in range(30)]


def limit(event):
    log.loaded_entries = 10


log = pn.Log(*[i for i in range(100)], height=200, auto_scroll_limit=100, width=500)
prepend_button = pn.widgets.Button(name="Prepend", on_click=prepend)
insert_button = pn.widgets.Button(name="Insert", on_click=insert)
append_button = pn.widgets.Button(name="Append", on_click=append)
replace_button = pn.widgets.Button(name="Replace", on_click=replace)
limit_button = pn.widgets.Button(name="Limit", on_click=limit)
pn.Row(
    pn.Column(
        prepend_button,
        insert_button,
        append_button,
        replace_button,
        limit_button,
        pn.widgets.IntInput.from_param(log.param.loaded_entries),
        max_width=200
    ),
    log,
).servable()

@philippjfr
Copy link
Member

It's probably going to be pretty involved. It'd look something like this:

  1. JS watches each child to detect if it's in the viewport and updates a property with the range of currently visible children (probably throttled somehow).
  2. Python observes changes in this range property and if any of the children in the buffer zone are visible then the buffer has to be refilled with new children (and if we exceed some total threshold value we also delete any children that are farthest out-of-view).

In practice that means overriding Log._get_objects and implement a version that handles adding and removing items as the visible items change.

@ahuang11
Copy link
Contributor Author

ahuang11 commented Dec 20, 2023

Needs cleanup, tests, and maybe some logic fixes with the timeout, but now it only renders the loaded_entries.

Screen.Recording.2023-12-19.at.5.50.19.PM.mov

@ahuang11 ahuang11 marked this pull request as ready for review December 27, 2023 01:44
@ahuang11
Copy link
Contributor Author

Will add reference gallery tomorrow.

Screen.Recording.2023-12-26.at.5.45.04.PM.mov

Strangely, it takes a long time to serve:

import panel as pn

pn.extension()

pn.Log(
    *list(range(10000)), scroll=True, height=250, width=250
).servable()

vs one order of magnitude less so I'm not sure if it's still rendering everything first then cutting out the pieces.

import panel as pn

pn.extension()

pn.Log(
    *list(range(1000)), scroll=True, height=250, width=250
).servable()

@ahuang11
Copy link
Contributor Author

Some things I noticed, but not sure how to address:

  1. it takes similar time as Column to execute on the backend, but on the browser, it's significantly faster to render and see the webpage:
image 2. The scrollbar bounces around, due to the objects being replaced I think and my algorithm to figure out the original scroll position with new objects prepended 3. If height isn't set, there's no "personal" scrollbar so it doesn't load more.
import panel as pn
pn.extension()

pn.Log(*[pn.chat.ChatMessage(i) for i in range(1000)], height=500).show()

@philippjfr
Copy link
Member

The current implementation doesn't really seem like what we discussed, specifically I thought the idea was that we would keep track of the items that are in the viewport and load (and unload) a buffer around that but here it seems like we are just continuously loading more models and only ever loading items from the bottom. I'll have a go at changing the implementation.

@ahuang11
Copy link
Contributor Author

ahuang11 commented Jan 16, 2024

Right, I wanted to get the loading part right before trying to add the unload/buffer part, but if I'd appreciate you having a go at the implementation.

@philippjfr
Copy link
Member

philippjfr commented Jan 17, 2024

Okay, I think this implementation works pretty well. There's a couple things left to do:

  • Scroll button should allow scrolling to the very top or the very bottom (instead of the top/bottom of the currently loaded objects)
  • Do not load new objects until you are half way into the buffer.
  • Update docs and add docstring
  • Move Log out of layout/base.py
  • Add/Update tests

@ahuang11
Copy link
Contributor Author

ahuang11 commented Jan 18, 2024

Nevermind... I refreshed the panel dir and it worked.


Strangely, it works in notebook, but in the browser, everything is hidden:
image

image

@ahuang11
Copy link
Contributor Author

ahuang11 commented Jan 18, 2024

Another todo(?) is to connect setting visible objects and scrolling to those visible objects, e.g. log.visible_objects = [929, 928, 927, 926, 925, 924]

But I'm wondering what happens if log.visible_objects = [910, 950, 999]? Should visible_objects be set to read only?

@philippjfr
Copy link
Member

Cannot reproduce your browser issues, and yes, visible_objects should be readonly.

@ahuang11
Copy link
Contributor Author

ahuang11 commented Jan 18, 2024

Another idea is to set an int param last_visible_index to scroll to that entry.

@philippjfr
Copy link
Member

Another idea is to set an int param last_visible_index to scroll to that entry.

What are you trying to solve for with that? At minimum we would need the first and last.

@ahuang11
Copy link
Contributor Author

ahuang11 commented Jan 18, 2024

I guess we already have a scroll_position, but perhaps we should have a scroll_index.
#6083

Do we need the first and last? I thought it's just this._last_visible.el.scrollIntoView(true)

panel/layout/feed.py Outdated Show resolved Hide resolved
panel/layout/feed.py Outdated Show resolved Hide resolved
@ahuang11 ahuang11 changed the title Add pn.Log to allow truncating entries Add pn.Feed to allow buffering of feed objects Feb 2, 2024
@ahuang11
Copy link
Contributor Author

ahuang11 commented Feb 2, 2024

After testing integrating Feed to ChatFeed, I had to cover some edge cases where:

  1. the number of visible objects was less than the load buffer and it would never buffer again, trapping the user
  2. the visible range exceeded the number of objects available which caused all objects to disappear

I think this is ready to be merged after your final review.

Here's the trapping in action (before the fixes):

Screen.Recording.2024-02-02.at.11.31.16.AM.mov

The after fix:
#6296

@philippjfr philippjfr merged commit 1a0a670 into main Feb 4, 2024
12 of 15 checks passed
@philippjfr philippjfr deleted the add_logs branch February 4, 2024 16:15
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.

Add pagination to ChatFeed
3 participants