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

Bugfix pipe_output after config.when resolution #1221

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

jernejfrank
Copy link
Contributor

Addressing #1218

For pipe_output

@pipe_output(
    step(_foo).when(key="foo"),
    step(_bar).when(key="bar"),
)
def filtered_data(raw_data: pd.DataFrame) -> pd.DataFrame:
    return ...

in case no conditions are met, e.g. config={"key":"skip"}, it returns filtered_dataas ifpipe_output` isn't there.

I think this is better than raising an error since it leaves the ability to choose transforms at runtime (which is related to using config.when on any other function in the DAG).

I also amended tests to capture this.

In case no pipe_output functions meet confgi.when conditions return
original node and skip pipe_output.
Copy link
Contributor

sweep-ai bot commented Nov 7, 2024

Hey @jernejfrank, here is an example of how you can ask me to improve this pull request:

@Sweep Add a unit test specifically for the edge case where no conditions are met in the `pipe_output` decorator, testing the behavior with different config values that do not match any conditions.

📖 For more information on how to use Sweep, please read our documentation.

Copy link
Contributor

@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.

❌ Changes requested. Reviewed everything up to ff864e3 in 1 minute and 14 seconds

More details
  • Looked at 132 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. hamilton/function_modifiers/macros.py:1317
  • Draft comment:
    The logic in transform_node is complex and involves multiple steps and conditions. Consider adding comments to explain the process, especially where the original node is returned if no transforms are applied.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_DST30BFmXzRHGoVS


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

hamilton/function_modifiers/macros.py Show resolved Hide resolved
Copy link
Collaborator

@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.

Thanks @jernejfrank !

@skrawcz skrawcz linked an issue Nov 12, 2024 that may be closed by this pull request
@skrawcz skrawcz merged commit 239d142 into DAGWorks-Inc:main Nov 12, 2024
24 checks passed
@jernejfrank jernejfrank deleted the bug/pipe_output_config branch November 12, 2024 12:56
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.

fix: Better error for @pipe without step()
2 participants