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

Exposes current action name through action context #506

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

elijahbenizzy
Copy link
Contributor

@elijahbenizzy elijahbenizzy commented Jan 27, 2025

This allows you to get the current action name along with other application-level metadata (sequence ID, partition_key, etc...)

See #501 for details

Changes

  • added .action_name to ApplicationContext

How I tested this

  • unit tests

Notes

  • decided to add action name rather than action to limit API

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Important

Adds action_name to ApplicationContext to expose the current action name, with updates to documentation and tests.

  • Behavior:
    • Adds action_name attribute to ApplicationContext in application.py to expose the current action name.
    • Updates _context_factory() in application.py to set action_name.
  • Documentation:
    • Updates actions.rst and state-persistence.rst to include action_name in examples of accessing application-level metadata.
  • Tests:
    • Adds assertions for action_name in test_application.py to verify the correct action name is set in the context.
    • Updates test_parallelism.py to ensure action_name is correctly handled in parallel execution contexts.

This description was created by Ellipsis for 929ad84. 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 d400926 in 14 seconds

More details
  • Looked at 35 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. docs/concepts/actions.rst:113
  • Draft comment:
    Ensure that the documentation clearly explains how to access action_name from ApplicationContext, similar to how app_id, partition_key, and sequence_id are documented.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR adds action_name to the ApplicationContext, but the documentation in actions.rst and state-persistence.rst does not mention how to access it. This should be clarified for consistency and completeness.
2. docs/concepts/state-persistence.rst:46
  • Draft comment:
    Ensure that the documentation clearly explains how to access action_name from ApplicationContext, similar to how app_id, partition_key, and sequence_id are documented.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR adds action_name to the ApplicationContext, but the documentation in actions.rst and state-persistence.rst does not mention how to access it. This should be clarified for consistency and completeness.

Workflow ID: wflow_OksUkwj4FPw9dKve


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

Copy link

github-actions bot commented Jan 27, 2025

A preview of 929ad84 is uploaded and can be seen here:

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

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/506/

@elijahbenizzy elijahbenizzy force-pushed the add-action-name-to-context branch from d400926 to 575ba94 Compare January 27, 2025 17:43
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 575ba94 in 37 seconds

More details
  • Looked at 113 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. tests/core/test_application.py:1356
  • Draft comment:
    Consider adding a test case for async context to ensure action_name is correctly set and accessible in async scenarios.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR adds the action_name attribute to the ApplicationContext class, which is used in various parts of the codebase. The test cases in test_application.py and test_parallelism.py verify that the action_name is correctly set and accessible. However, there is a missing test case for the async context in test_parallelism.py. Adding a test case for async context would ensure comprehensive coverage.

Workflow ID: wflow_kvP7G7osTAuIxDOO


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

@elijahbenizzy elijahbenizzy requested a review from skrawcz January 27, 2025 17:47
burr/core/application.py Outdated Show resolved Hide resolved
This allows you to get the current action name along with other
application-level metadata (sequence ID, partition_key, etc...)

See #501 for details
@elijahbenizzy elijahbenizzy force-pushed the add-action-name-to-context branch from 575ba94 to 929ad84 Compare January 27, 2025 20:42
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 929ad84 in 1 minute and 17 seconds

More details
  • Looked at 104 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. burr/core/application.py:867
  • Draft comment:
    The check for action being None is unnecessary since action is always provided. Remove the if action else None condition.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    Looking at the code:
  1. The _context_factory method is called from the context property on line 1922
  2. The context property gets the next action via get_next_action()
  3. get_next_action() can return None according to its return type annotation
  4. Therefore action CAN be None when passed to _context_factory
  5. The null check is actually necessary for safety
    Am I sure that get_next_action() is the only way this method is called? Could there be other callers that guarantee a non-null action?
    Looking at the code, _context_factory is only used in two places:
  6. As a dependency factory value on line 822
  7. Called directly from the context property on line 1928
    In both cases the action comes from get_next_action() which can return None.
    The comment is incorrect. The null check is necessary because action can be None when passed to this method since it comes from get_next_action() which has an Optional return type.

Workflow ID: wflow_PAyVMP75DYMAZZ2s


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

@elijahbenizzy elijahbenizzy merged commit e80ffe0 into main Jan 27, 2025
11 checks passed
@elijahbenizzy elijahbenizzy deleted the add-action-name-to-context branch January 27, 2025 21:15
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.

2 participants