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

Adding TaskGroup support in chain() #17456

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

josh-fell
Copy link
Contributor

Related to: #17083, #16635

This PR ensures that TaskGroups can be used to set dependencies while calling the chain() method. Support for XComArgs and EdgeModifiers has been implemented in previous PRs: #16732, #17099


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@github-actions
Copy link

github-actions bot commented Aug 6, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Aug 6, 2021
@josh-fell josh-fell force-pushed the chain-with-taskgroup branch 2 times, most recently from bc08076 to 23d320b Compare August 17, 2021 00:32
@josh-fell
Copy link
Contributor Author

FYI - the two failing tests pass locally for me. Let me know if there is anything I can do to help these tests pass here.

MSSQL 2017-latest, Python 3.7
image

MSSQL 2019-latest, Python 3.9
image

@uranusjr
Copy link
Member

Those two (test_mark_success_on_success_callback and test_verify_integrity_if_dag_not_changed) have been flaky recently; they are likely not related to your changes.

@potiuk
Copy link
Member

potiuk commented Aug 17, 2021

Those two (test_mark_success_on_success_callback and test_verify_integrity_if_dag_not_changed) have been flaky recently; they are likely not related to your changes.

However any help to make them non-flaky is definitely most welcome :)

@josh-fell
Copy link
Contributor Author

josh-fell commented Aug 19, 2021

Those two (test_mark_success_on_success_callback and test_verify_integrity_if_dag_not_changed) have been flaky recently; they are likely not related to your changes.

However any help to make them non-flaky is definitely most welcome :)

Happy to take a look and help any way I can. Perhaps a silly question but is this test always failing for the same exception? If not, is there a set of logs I can look through to see any failure trends?

  >       assert success_callback_called.value == 1
  E       assert 0 == 1
  E        +  where 0 = <Synchronized wrapper for c_int(0)>.value

@potiuk
Copy link
Member

potiuk commented Aug 19, 2021

Happy to take a look and help any way I can. Is this test always failing for the same exception? If not, is there a set of logs I can look through to see any failure trends?

Only by looking at individual failed actions. I used to have a "quarantine" summary of tasks, but it has proven to be really a pain to maintain.

But that actually be a good idea, to keep such status of all failed tests somewhere. Maybe you could do that :). I used to keep quarantined tests status in GitHub Issue (as strange as it might seem) but that was a bit "clunky". Maybe we could keep it somewhere else. Google Spreadsheet might be a possibility for one.

We could push status of failed tests for every 'master' merge push. to make it more "robust" and only include stuff that should "work".

@kaxil kaxil merged commit 1945494 into apache:main Sep 9, 2021
@josh-fell josh-fell deleted the chain-with-taskgroup branch September 9, 2021 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants