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: task flow dynamic mapping with default_args #41592

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

Conversation

phi-friday
Copy link
Contributor

closes: #33600


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@phi-friday phi-friday marked this pull request as ready for review August 19, 2024 17:31
@eladkal eladkal requested a review from uranusjr August 19, 2024 18:27
@phi-friday phi-friday force-pushed the fix-task-flow-dynamic-mapping branch from 9ab775d to 0ca9fcd Compare August 19, 2024 23:50
@phi-friday phi-friday force-pushed the fix-task-flow-dynamic-mapping branch from 0ca9fcd to 1587f74 Compare August 20, 2024 10:36
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

One nit, otherwise looks good to me

airflow/decorators/base.py Outdated Show resolved Hide resolved
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Good enough for me

@shahar1 shahar1 added this to the Airflow 2.10.1 milestone Aug 23, 2024
@phi-friday phi-friday force-pushed the fix-task-flow-dynamic-mapping branch from de3f1a2 to a91b7b3 Compare August 26, 2024 03:20
@phi-friday phi-friday force-pushed the fix-task-flow-dynamic-mapping branch 2 times, most recently from 4b78351 to d9c9f2a Compare September 15, 2024 07:29
@phi-friday
Copy link
Contributor Author

After a recent rebase, I'm getting the following error, and I'm not sure why.

_________ TestOtelMetrics.test_timer_start_and_stop_manually_send_true _________
[gw2] linux -- Python 3.8.20 /usr/local/bin/python

self = <tests.core.test_otel_logger.TestOtelMetrics object at 0x7f0c14b7e370>
mock_time = <MagicMock name='perf_counter' id='139690224843072'>
name = 'test_stats_run'

    @mock.patch.object(time, "perf_counter", side_effect=[0.0, 3.14])
    def test_timer_start_and_stop_manually_send_true(self, mock_time, name):
        timer = self.stats.timer(name)
        timer.start()
        # Perform some task
        timer.stop(send=True)
    
        assert isinstance(timer.duration, float)
>       assert timer.duration == 3.14
E       assert 3140.0 == 3.14
E        +  where 3140.0 = <airflow.metrics.otel_logger._OtelTimer object at 0x7f0c2a6999a0>.duration

tests/core/test_otel_logger.py:327: AssertionError

@phi-friday phi-friday force-pushed the fix-task-flow-dynamic-mapping branch 2 times, most recently from 0a79c1a to d42437e Compare September 18, 2024 16:59
@bitomukesh
Copy link

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Bug Fix - Fix Task Flow Dynamic Mapping with Default Arguments

base.py - Updated handling of default arguments in task decorators

test_mapped.py - Added test for mapped task with arbitrary default arguments

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.

Dynamic tasks fail when default_args contains custom fields
6 participants