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

Performance improvement on (pattern-matching) callbacks #3009

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Spriteware
Copy link

@Spriteware Spriteware commented Sep 19, 2024

This PR is related to the issue I described here : #3008
If I encountered performance limitation with pattern-matching callbacks, I think this PR can be beneficial for regular callbacks as well.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • task 1
    • task 2
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

@gvwilson gvwilson added performance something is slow P2 needed for current cycle fix fixes something broken community community contribution labels Sep 19, 2024
@chriddyp
Copy link
Member

Awesome investigation @Spriteware , and an impressive first PR! Thank you for digging in here 🌟

@T4rk1n
Copy link
Contributor

T4rk1n commented Sep 23, 2024

There seems to be a side effect on the test:

def test_grva008_shapes_not_lost(dash_dcc):

The calls on lines

dash_dcc.wait_for_text_to_equal("#output", "0")
# Draw a shape
dash_dcc.click_and_hold_at_coord_fractions("#graph", 0.25, 0.25)
dash_dcc.move_to_coord_fractions("#graph", 0.35, 0.75)
dash_dcc.release()

happens before the graph render.
If you add a time.sleep before the Draw a shape, it passes the test.

I think before this fix the dash_dcc.wait_for_text_to_equal("#output", "0") may have taken longer which allowed the graph to render.

@T4rk1n
Copy link
Contributor

T4rk1n commented Sep 23, 2024

Let's just add: dash_dcc.wait_for_element("#graph .js-plotly-plot") after

dash_dcc.wait_for_text_to_equal("#output", "0")
and it will fix the test.

Then it's just missing a changelog entry and it's good to go 🚀

@Spriteware
Copy link
Author

Spriteware commented Sep 23, 2024

Hey @T4rk1n thank you for digging into the failing dcc tests. I wasn't sure what to do, as the main dash tests were passing locally. And not sure if I had the time and energy to go through this as well.

Excited if this gets merged!

@chriddyp Thank you for your supportive comment. If you need more investigations for optimizations, my DMs on plotly forum are indeed opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution fix fixes something broken P2 needed for current cycle performance something is slow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants