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

Fixes __tracer for class based actions #452

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

skrawcz
Copy link
Contributor

@skrawcz skrawcz commented Dec 4, 2024

Refactors __context treatment to also handle __tracer. This now enables one to pass through/request the tracer object in a class based action.

Changes

  • fixes class based actions to be able to request __tracer as input.

How I tested this

  • unit tests

Notes

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

Refactor handling of __context and __tracer parameters in class-based actions by introducing _remap_dunder_parameters.

  • Behavior:
    • Refactor _remap_context_variable to _remap_dunder_parameters in application.py to handle __context and __tracer parameters.
    • Update _run_function to use _remap_dunder_parameters for __context and __tracer.
  • Tests:
    • Update tests in test_application.py to reflect changes in parameter remapping.
    • Add test cases for __tracer handling in test_remap_context_variable_with_mangled_contexttracer().
  • Misc:
    • Rename _remap_context_variable to _remap_dunder_parameters.

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

Refactors __context treatment to also handle __tracer.
This now enables one to pass through/request the tracer
object in a class based action.
@skrawcz skrawcz requested a review from elijahbenizzy December 4, 2024 05:33
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 2da67a9 in 58 seconds

More details
  • Looked at 132 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:109
  • Draft comment:
    The docstring for _remap_dunder_parameters could be improved for clarity. Consider specifying that it handles multiple dunder parameters and remaps them to their mangled versions if present in the function signature.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The function _remap_dunder_parameters is designed to remap dunder parameters like __context and __tracer to their mangled versions in the function signature. The test cases provided cover various scenarios, including when the parameters are present, absent, or mangled. The function seems to be working as intended, as the test cases pass successfully. However, the docstring of _remap_dunder_parameters could be improved for clarity.

Workflow ID: wflow_0aXbVT5V6DVOWKcp


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

Copy link

github-actions bot commented Dec 4, 2024

A preview of is uploaded and can be seen here:

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

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

Copy link
Contributor

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

There are a few more cases I think (streaming, etc...) but we'll want to deal with them as they come up. Good for now, thanks!

@elijahbenizzy elijahbenizzy merged commit 86f4cb2 into main Dec 4, 2024
11 checks passed
@elijahbenizzy elijahbenizzy deleted the fix_class_tracer branch December 4, 2024 16:57
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