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

Stop infinite loop calculating traces due to is_active event #2861

Closed
wants to merge 3 commits into from

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented May 8, 2024

Use cached trace when methods are triggered by is_active events rather than constantly recalculating. Previous behavior:

Screen.Recording.2024-05-07.at.3.08.31.PM.mov

@rosteen rosteen added bug Something isn't working specviz2d labels May 8, 2024
@rosteen rosteen added this to the 3.10.1 milestone May 8, 2024
@github-actions github-actions bot added the plugin Label for plugins common to multiple configurations label May 8, 2024
@@ -260,6 +260,9 @@ def __init__(self, *args, **kwargs):
default_text='From Plugin',
filters=['is_trace'])

# Cache the actual calculated trace
Copy link
Member

Choose a reason for hiding this comment

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

Can you please stress in an in-line comment (here and for the other cache) that this should only ever be used for the interactive previews and should always be recomputed when the user is calling a method (since the cache could be out of date if the user is updating traitlets with the plugin inactive).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Drive by comment: Does this cache never have to be invalidated?

@kecnry kecnry added the 💤backport-v3.10.x on-merge: backport to v3.10.x label May 8, 2024
@rosteen
Copy link
Collaborator Author

rosteen commented May 8, 2024

Drive by comment: Does this cache never have to be invalidated?

I think it will be recalculated any time that would be the case. @kecnry can you think of any time it would need to be invalidated?

@rosteen
Copy link
Collaborator Author

rosteen commented May 8, 2024

There is an extra blink where the trace previews briefly disappear with this PR, but that is probably better than getting stuck in an infinite loop.

@pllim
Copy link
Contributor

pllim commented May 8, 2024

@kecnry
Copy link
Member

kecnry commented May 8, 2024

can you think of any time it would need to be invalidated?

Any time a traitlet is changed and the plugin is inactive, the cache will be out-of-date. This is what skip_if_no_updates_since_last_active is supposed to handle, but clearly isn't doing a good job here. I can try to dig to see if I can fix the underlying cause, but since there is a rush for this, this might be a good incremental improvement and we can live with the edge-case problem.

Copy link

codecov bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 88.70%. Comparing base (bcea12f) to head (a52f9d9).
Report is 6 commits behind head on main.

Files Patch % Lines
...plugins/spectral_extraction/spectral_extraction.py 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2861      +/-   ##
==========================================
- Coverage   88.93%   88.70%   -0.24%     
==========================================
  Files         111      111              
  Lines       17165    17122      -43     
==========================================
- Hits        15266    15188      -78     
- Misses       1899     1934      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rosteen
Copy link
Collaborator Author

rosteen commented May 8, 2024

Closing this in favor of #2862 .

@rosteen rosteen closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working plugin Label for plugins common to multiple configurations specviz2d 💤backport-v3.10.x on-merge: backport to v3.10.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants