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

Prevent creation of large _stylesheet class variable #341

Merged
merged 4 commits into from
Mar 4, 2024

Conversation

nenb
Copy link
Contributor

@nenb nenb commented Mar 4, 2024

This PR fixes a bug (#337) whereby large stylesheets objects were passed to panel/bokeh to parse and render. The issue was that every time a new message was created in a chat, the same bunch of CSS rules were added to a class variable (_stylesheets), causing rapid growth in the size of the variable.

@nenb nenb requested a review from pmeier March 4, 2024 14:40
@nenb nenb force-pushed the improve-chat-toggle-performance branch from 3dd16cf to b243e87 Compare March 4, 2024 14:44
Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

To asses the situation correctly:

  • ChatMessage._stylesheets is a class variable coming from panel so there is little we can do about it
  • It is filled with some default values and we just want to append ours
  • Our style sheets are static and thus could also be appended statically

Is that correct? If so, can't we just do

class RagnaChatMessage(pn.chat.ChatMessage):
    _stylesheets = [
        *pn.chat.ChatMessage._stylesheets,
        *message_stylesheets,
    ]

? No need to ever touch them at runtime.

@nenb
Copy link
Contributor Author

nenb commented Mar 4, 2024

Is that correct? If so, can't we just do

class RagnaChatMessage(pn.chat.ChatMessage):
    _stylesheets = [
        *pn.chat.ChatMessage._stylesheets,
        *message_stylesheets,
    ]

? No need to ever touch them at runtime.

Makes sense to me (although I haven't tested it). Feel free to push this to my branch and use it instead.

Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Added and tested my suggestion from #341 (review).

@pmeier pmeier merged commit 91e603d into Quansight:main Mar 4, 2024
10 checks passed
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.

None yet

2 participants