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 hint when add rows is used on vega lite chart #8499

Closed
wants to merge 1 commit into from

Conversation

kmcgrady
Copy link
Collaborator

Describe your changes

We have a lightweight check that's supposed to detect whether the data is a subset of the original data that checks the first and last value of the set, which creates some less than intended consequences (see issue). This change essentially handles the two use cases:

  1. Disregard the check when a graph is changing.
  2. Allow the use case for add rows to successfully add rows if necessary.

We fix this by adding an add_rows hint that we set only on the frontend. We use the hint to do the lightweight check and update the data.

This needs more testing but it looks like the unintended consequences include the fact that rerenders of the graph with superset data (not via add_rows) will rerender the whole graph (which feels fine).

GitHub Issue Link (if applicable)

Closes #6689

Testing Plan

  • Explanation of why no additional tests are needed
  • Unit Tests (JS and/or Python)
  • E2E Tests
  • Any manual testing needed?

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@@ -197,6 +197,7 @@ export class ElementNode implements AppNode {
datasets: modifiedDatasets,
useContainerWidth: proto.useContainerWidth,
vegaLiteTheme: proto.theme,
needsAddRows: proto.needsAddRows,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the proto field here even required? Couldn't we just always set it to false here since the proto change seems to be never set to something else than false?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, the value needs to be set. Practically, this will always be false, but I figured it's best to maintain the protobuf's members (theoretically, people can create a custom message, and we can honor it). See example.

@LukasMasuch
Copy link
Collaborator

but it looks like the unintended consequences include the fact that rerenders of the graph with superset data (not via add_rows) will rerender the whole graph

Do you have an example of that? This only happens on rerun with different data or?

@kmcgrady
Copy link
Collaborator Author

My example was the following (note, I used line_chart for simplicity.

import streamlit as st
import numpy and np
import pandas as pd
import time

df = pd.DataFrame(np.random.randn(15, 3), columns=(["A", "B", "C"]))
foo = st.dataframe(df)
my_data_element = st.line_chart(df)

for tick in range(10):
    time.sleep(0.5)
    new_df = pd.concat(
        [df, pd.DataFrame(np.random.randn(1, 3), columns=(["A", "B", "C"]))],
        ignore_index=True,
    )
    foo.dataframe(new_df)
    my_data_element.line_chart(new_df)

Each new line chart is supposed to be the same with different. Upon testing, I have observed that a new line chart is generated, which is less than great, but at least makes this change less risky. Looks like the spec changes based on the id of the dataset. Seems like an opportunity to make this better, but this at least fixes the bug to make add_rows work as intended.

Copy link
Collaborator

@LukasMasuch LukasMasuch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -40,5 +40,7 @@ message ArrowVegaLiteChart {
// override the properties with a theme. Currently, only "streamlit" or None are accepted.
string theme = 6;

bool needs_add_rows = 7;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can you add a comment here describing the property?

@@ -85,6 +85,8 @@ export interface VegaLiteChartElement {

/** override the properties with a theme. Currently, only "streamlit" or None are accepted. */
vegaLiteTheme: string

needsAddRows: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can you also add a comment here describing the property?

@kmcgrady kmcgrady closed this May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vega_lite_chart displays wrong data due to premature caching optimization on the front end
2 participants