-
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
Implement Metadata to emit runtime extra #38650
Conversation
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.
Nice!
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.
I understand the desire to add the option to yield
but I do not get the relation why the execution internals of PythonOperator implementation needs a change in method signatures for this. I feel that DataSetEvents should be the same approach like other context information and do not see why the callable execution needs a signature change.
The dataset URI coercing logic has been extracted into its own (internal) function for reuse.
Also reverted check for task.pre_execute and task.post_execute since those are defined on BaseOperator and shouldn't be a generator function. Instead, those functions need to individually check if the hooks wrapped inside are generator functions.
The key is not always available. If it isn't, we just create an accessor object on the fly.
c3cd16d
to
5b929fc
Compare
5b929fc
to
2329a61
Compare
The key is not always available (mostly in tests). Create a placeholder if it's not.
eb5a34b
to
a076ae5
Compare
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.
Thanks for the rework. Now I really "like" it. Before I would have stepped back from review but would have not blocked merge :-D
One minor thing would be great to also try to use it in one of the example DAGs for example in example_producer_1 or 2? Does it make sense to add it one of the examples? (I propose this on top of docs which are good, sometimes people seek for inspiration or examples in the example code. Not many people read docs before designing or coding.
Close #37810. This implements the
yield
syntax and theMetadata
class, and mechanism to collect the yielded values in the worker.