Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes state writes with private variables #504

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

elijahbenizzy
Copy link
Contributor

@elijahbenizzy elijahbenizzy commented Jan 24, 2025

This drops private variables when written by an action. Note we do not add tests for this as it is undefined behavior -- we may error out later on.


Important

_state_update in application.py now drops private variables when written by an action, with a warning added in state.rst about undefined behavior.

  • Behavior:
    • _state_update in application.py now drops private variables (keys starting with __) when written by an action.
    • Modifying private variables is considered undefined behavior and may result in errors in the future.
  • Documentation:
    • Added warning in state.rst about undefined behavior when modifying private variables starting with __.

This description was created by Ellipsis for 00f17f4. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 8a9c3c5 in 27 seconds

More details
  • Looked at 70 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. burr/core/application.py:194
  • Draft comment:
    The parameter state_subset_pre_update in the docstring does not match the actual parameter name state_to_modify. Please update the docstring to reflect the correct parameter name.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_tax4fUCpzioXToNM


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

github-actions bot commented Jan 24, 2025

A preview of 00f17f4 is uploaded and can be seen here:

https://burr.dagworks.io/pull/504

Changes may take a few minutes to propagate. Since this is a preview of production, content with draft: true will not be rendered. The source is here: https://github.com/DAGWorks-Inc/burr/tree/gh-pages/pull/504/

@elijahbenizzy elijahbenizzy requested a review from skrawcz January 24, 2025 21:34
This drops private variables when written by an action. Note we do *not*
add tests for this as it is undefined behavior -- we may error out later
on.
@elijahbenizzy elijahbenizzy force-pushed the fix-state-write-underscores branch from 8a9c3c5 to 00f17f4 Compare January 24, 2025 21:45
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 00f17f4 in 30 seconds

More details
  • Looked at 70 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. docs/concepts/state.rst:33
  • Draft comment:
    The documentation should mention that private variables (starting with __) are automatically dropped during state updates to prevent undefined behavior.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. burr/core/application.py:194
  • Draft comment:
    The comment mentions "undefined behavior" but does not specify that private variables are dropped during state updates. Consider clarifying this in the comment.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_Q4ZdyXPDmeLSVdSk


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

LGTM. but are you sure there's no test to be written? At least to break if we modify the current behavior?

@elijahbenizzy
Copy link
Contributor Author

LGTM. but are you sure there's no test to be written? At least to break if we modify the current behavior?

Yes, it's specifically undefined (as in we want to address it further but not now), so I'm comfortable not adding tests for now as to not encode a contract we don't need.

@elijahbenizzy elijahbenizzy merged commit 55ecfb8 into main Jan 25, 2025
11 checks passed
@elijahbenizzy elijahbenizzy deleted the fix-state-write-underscores branch January 25, 2025 19:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants