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

fix: LivenessStateException with static table in dx and ui #1074

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

Conversation

jnumainville
Copy link
Collaborator

Fixes https://deephaven.atlassian.net/browse/DH-18167

Static tables should not be managed, but they do not error, which leads to errors later, such as in the examples in the issue. Verified that those now work.

@jnumainville jnumainville requested review from niloc132, a team and mattrunyon and removed request for a team January 3, 2025 20:34
@jnumainville jnumainville changed the title Fix: LivenessStateException with static table in dx and ui fix: LivenessStateException with static table in dx and ui Jan 3, 2025
mattrunyon
mattrunyon previously approved these changes Jan 9, 2025
Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

Makes me wonder if the liveness scope manage method should handle non-refreshing tables

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this blank file needed? Can you put a comment in the file if it is needed for something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is standard for unittest directories for test discovery. That seems like something that wouldn't need a comment to me? There's others as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Guess I never noticed. I figured there would be something in these files

mattrunyon
mattrunyon previously approved these changes Jan 23, 2025
from deephaven.execution_context import get_exec_ctx
from deephaven.liveness_scope import liveness_scope

static_source = new_table([int_col("X", [1, 2, 3])])
Copy link
Member

Choose a reason for hiding this comment

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

From our earlier slack, can this be updated to do one with a live table, and a partitioned table too?

This test successfully validates that the figure can be created in this way, but does it check that the live objects are retained for consumption on the other side, or can we not test the plugin in that way?

Copy link
Collaborator Author

@jnumainville jnumainville Jan 24, 2025

Choose a reason for hiding this comment

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

I can add partitioned and live tables easily

I'm not exactly sure how I'd test if the objects are retained correctly. Is there a way to check if an object is being held by a liveness scope or something along those lines? Because I can definitely pull out the tables associated with this figure, simulating when they are sent to the client, but I need some way to check they're retained, unless it's sufficient to do something like dhpd.to_pandas(table) and call it a day

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think our e2e tests can functionally do that second part - I've added one with a live and partitioned table.

Copy link

github-actions bot commented Feb 5, 2025

plotly-express docs preview (Available for 14 days)

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.

3 participants