-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Make _run_raw_task AIP-44 compatible #38992
Make _run_raw_task AIP-44 compatible #38992
Conversation
Generally it's what I would expect - let's get the test pass and merge the depending PR and we can review that one in detail |
df8cb7b
to
9fc852b
Compare
checks passed @potiuk |
I am moatly out till Friday and this one needs quite a bit more review - if it can wait. |
no worries, enjoy |
I reviewed it quite a bit more thoroughly. I think it's the right direction, but we should - I think - complete it (i.e. turn handle_reschedule into internal_api, merge commiting changes to defer task and avoid keeping duplicated _run_raw_task for DB/DB_isolation mode (unless there is a good reason for now to keep it, but can't think of any). |
7149484
to
7b625c8
Compare
converting to draft while i work through a few more aspects of this. |
9673464
to
2f625e7
Compare
(cherry picked from commit 994d6d119f5058972bfaf89eed1f375a99d609a2)
ce1921b
to
6db95e0
Compare
ok @potiuk @vincbeck @uranusjr -- i think this is ready for another look. This "base" PR is the first 4 commits from the mothership PR (#37851) After this main PR there will be 5 other PRs (each of the remaining commits in #37851 will be a distinct PR) that will follow to complete the job, by which I mean being able to run a task entirely without db connectivity, including mapped tasks, xcom, and async. Using the mothership branch I have done live testing to ensure that these commits work properly for the other changes which follow, and which can be reviewed and merged separately. I'll also mention one more thing -- in this PR there are 4 distinct commits each covering a different part of this, and it may be helpful to review them one at a time |
@dstandish - I am currently at PyCon US, and I have a busy plan of attending talks and meeting people here :) - > I plan to take a very serious look at the AIP-44 related changes right after I come back (next week Wednesday). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While elaborating on more details for AIP-69 I stumbled over this PR and assume this is one of the things which need to be merged before my (naive) attempts for AIP-69 will be possible.
By reading over the code some comments - just recommendations - do not consider them as full/real review.
If it helps let me know then I could make a full test as I am failing exactly on this point maight be easy. Otherwise I assume some more pytests are missing? Wondering why I see no changes on test code?
can you clarify what you mean here? |
Uuups, too many typos. I wanted to say: |
Thanks yeah you mean like help with practical testing like trying things out. I’ve done a lot of it so I feel ok about it so not necessary unless you want to. If you do I recommend checking out the “mothership” pr branch. I think I referenced it somewhere. There you can run this with helm in full isolation mode with dedicated rpc server. Re unit tests, I think with many of these aip44 changes it’s meant to be a no behavior change refactor so I think that’s why we haven’t historically added many tests. But I can add if you have specific areas of concern. Re the naming suggestions you made above, I sort of stuck with the pattern that was established before me, which was just to keep the same name as the method. I don’t personally think it is likely to create confusion, since they are namespaced by the module anyway. We could make this more explicit in the rpc module by importing modules and not functions, thereby making things more explicit, but maybe I’d leave that for another pr. Note so that in all cases the functions are private which makes the choice much less consequential as not user facing and changeable at any time. Incidentally I feel like we maybe should find a way so we don’t have to explicitly add those imports in the rpc module. Seems it shouldn’t be necessary given that we already decorate the func. But again that’s a different pr. |
thanks @jscheffl -- which branch were you on? there are more changes re mapping on the "mothership" PR (#37851) that are excluded from this PR just for easier review -- that's the branch you should be on for end-to-end testing. i cherry pick from that PR branch to make the smaller prs. i've been doing little bits at a time and this PR is bigger than i'd like, but it's still not the whole enchalada. this individual PR is still an incremental addition. |
Fully acknowledge. As I was trying to get started into PoC for AIP-69 I struggled the same and AIP-44 clearly solves all basic problems that I run into as well. Seems to be this rework is quite complex. Good that you have split up the "mothership" for incremental review. As proposed by you I tested on the "mothership" PR #37851 with GIT hash c1b78c035bf6e8250a12e1a7a0f83f78fcbeaa4e ("run-a-full-task-with-internal-api"). No rebase/merge with main before tests. So as you cherry-pick into smaller PRs and leaving complexity manageable... if all is green might still be good to merge the pieces and I assume to make it working then a few other API calls need to be reworked. |
etc ah yeah those scenarios are good to add to my testing dag. maybe you could share your dag? and maybe i could add them as integration tests with xfail for API. like i said, not everything is implemented in this branch (or in the other branch for that matter) but many things are and it's all a work in progress. pretty soon i am going to have to roll off onto other things and will ticket out remaining known AIP-44 issues for the next person to take over. for example we have not done anything re triggerer yet. so just to be extra clear, this PR is not the final AIP-44 PR. this PR is just "get it working for the simplest cases" |
Yeah that's sorta how i've been thinking about it. and i think that ultimately we should probably compile a dag with the known important scenarios and set it up as an integration test. |
also for reference @jscheffl here's the PRs that came before on this one 😅 https://github.com/apache/airflow/pulls?q=is%3Apr+project%3Aapache%2F169+is%3Aclosed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making thunbs-up here. Knowing there are glitches to be fixed but this PR is a bit of progress as increment then to take next steps.
@potiuk i'll just go ahead and merge this now so i can close out all the PRs waiting on it. if you later want to look i'm happy to take your suggestions and make changes afterwards. thanks |
Migrate _run_raw_task to work with AIP-44. Also, _handle_reschedule, defer_task, xcom_pull and xcom_push.
Implement core functionality in _run_raw_task for AIP-44 / database isolation.
Followed by other PRs from #37851.