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 support for - middleware for component level interactions + custom name for logging purposes #5273

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

nishikantparmariam
Copy link

@nishikantparmariam nishikantparmariam commented Jul 17, 2023

Solves #5213. See #5213 (comment) also.

@nishikantparmariam nishikantparmariam changed the title Add support for middleware for component level interactions & custom name for logging purposes Add support for - middleware for component level interactions + custom name for logging purposes Jul 17, 2023
panel/io/state.py Outdated Show resolved Hide resolved
@nishikantparmariam nishikantparmariam marked this pull request as ready for review July 21, 2023 08:17
@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #5273 (c09a04a) into main (8128ca2) will increase coverage by 33.68%.
The diff coverage is 48.38%.

@@             Coverage Diff             @@
##             main    #5273       +/-   ##
===========================================
+ Coverage   38.60%   72.28%   +33.68%     
===========================================
  Files         290      292        +2     
  Lines       42318    42380       +62     
===========================================
+ Hits        16337    30635    +14298     
+ Misses      25981    11745    -14236     
Flag Coverage Δ
ui-tests ?
unitexamples-tests 72.28% <48.38%> (+49.43%) ⬆️

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

Files Coverage Δ
panel/io/state.py 69.30% <57.14%> (+30.74%) ⬆️
panel/io/middlewares.py 71.42% <71.42%> (ø)
panel/reactive.py 78.92% <50.00%> (+43.68%) ⬆️
panel/tests/ui/io/test_middlewares.py 36.36% <36.36%> (ø)

... and 222 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@nishikantparmariam
Copy link
Author

@philippjfr let me know what else would be required here e.g. tests, more documentation etc. Please point to any references. Thanks!

panel/param.py Outdated Show resolved Hide resolved
@nishikantparmariam
Copy link
Author

@philippjfr please let me know any review comments here. Thanks!!

@nishikantparmariam
Copy link
Author

Hi, @philippjfr @MarcSkovMadsen please add your review comments / thoughts here so we can push these changes. Thanks!

@philippjfr
Copy link
Member

I'll do a quick review shortly but also wanted to hear from @maximlt since he's been thinking about using this for some logging.

panel/viewable.py Outdated 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.

Hi @nishikantparmariam and sorry for the late reply. I have some interest in some similar functionality so here are some questions:

  • would you have an example use case for watching property changes?
  • do you think the preprocess/postprocess model is going to be convenient enough to for instance time how long it took to process an event? I see for instance that FastAPI has a different model for setting up middlewares so just checking we're going down the right path
    (https://fastapi.tiangolo.com/tutorial/middleware/#create-a-middleware)

I'm going to do more testing on a larger app that would require some middleware logging and will report back when done.

panel/viewable.py Outdated Show resolved Hide resolved
@nishikantparmariam
Copy link
Author

nishikantparmariam commented Sep 3, 2023

Hi @maximlt,

would you have an example use case for watching property changes?

Yes, there are many e.g. I want to log if user changed tab of pn.Tabs (active property would change), user interacted with an pn.widgets.IntSlider (value property will change). Also, if my components have attached callback (added by slider.param.watch(my_callback, ['value'])) which does some computation, I am able to time all of those using the middleware. The postprocess part gets executed only after all callbacks have finished processing. One use-case here would be to log the timing of each such UI interaction, and find out which is the slowest of them to improve UX.

do you think the preprocess/postprocess model is going to be convenient enough to for instance time how long it took to process an event?

Thanks for pointing this out! Yes, it should be convenient.

Here is how one may write a timing middleware:
class TimingMiddleware(PropertyChangeEventMiddleware):
    def __init__(self):
         """Do any initialization"""

    def preprocess(self, syncable, doc, events):
        self.start_time = time.time()
        self.syncable = syncable # Save for postprocess        

    def postprocess(self):
        process_time = time.time() - self.start_time
        # Do something with self.syncable now

pn.state.add_property_change_event_middleware(TimingMiddleware())

I also explored django and expressjs middlewares.

Then, I realized that in pre/postprocess model, for more advanced cases there is an issue: If one wants to run pre of middleware1 before pre of middleware2, but want that post of middleware1 runs after post of middleware2.

See Explanation

Preprocess/postprocess model (current implementation):

pre of M1
pre of M2
pre of M3
...main processing...
post of M1
post of M2
post of M3

Whereas the other models e.g. FastAPI, django works like this (Reference) -

pre of M1
  call_next 
  > pre of M2
     call_next
     > pre of M3
         call_next 
             > ...main processing...
     > post of M3
  > post of M2
post of M1

To fix this in pre/postprocesss model, it seems correct thing would be to run postprocess in reverse order:

for bokeh_event_middleware in state._bokeh_event_middlewares[::-1]:
      bokeh_event_middleware.postprocess()

And, we should mention in the docs that postprocess of current middleware will run after the postprocess of the next added middleware.

After this, the pre/postprocess model seems as powerful as the other models. Implementation-wise, I found pre/postprocess to be quite easy and intuitive. Let me know your thoughts, if you feel we should for go other type of model, I'll implement that.

Thanks!

@ndmlny-qs
Copy link
Contributor

@nishikantparmariam I'm looking at the middleware you have proposed, and I've run into a few issues that would be solved with an minimum example using your changes. Do you mind posting one?

@nishikantparmariam
Copy link
Author

nishikantparmariam commented Oct 11, 2023

@maximlt @philippjfr let me know your comments on #5273 (comment) so we can push this forward. I'm happy to go with either model.


@ndmlny-qs you can try this example and notice that we can find out processing time of button click callback.

import time
import panel as pn
pn.extension()

class TimingMiddleware(BokehEventMiddleware):
    def preprocess(self, syncable, doc, event):
        self.start_time = time.time()      

    def postprocess(self):
        process_time = time.time() - self.start_time
        print('Took :', process_time)

pn.state.add_bokeh_event_middleware(TimingMiddleware())

btn = pn.widgets.Button(name='Foo')

def callback(event):
    # Some computation
    time.sleep(2)

btn.on_click(callback)

pn.Row(btn).servable()

@ndmlny-qs
Copy link
Contributor

@nishikantparmariam I can see button timings showing up in my terminal. My mouse isn't visible for some reason, but the timings are showing up. Thank you very much for a MRE, it was very helpful.

button-timing.webm

If this feature looks good, and it's a feature Panel would like to have, then let me know and I will write documentation and examples for it.

@ndmlny-qs
Copy link
Contributor

I've got a playwright test for this feature. Still need to make a few more tests, and documentation around it.

@ndmlny-qs
Copy link
Contributor

I rebased against the main branch and added playwright tests for the middleware components. The new tests are passing locally and in CI.

@philippjfr I can add documentation around using this feature. As @nishikantparmariam mentioned, there are some caveats to how the pre/post processing work that will need to be captured in examples and documentation.

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