+1(not binding) On Wed, Jun 17, 2020 at 3:03 AM Gerard Casas Saez <gcasass...@twitter.com.invalid> wrote:
> Hi everyone, > > Sending an email here to consolidate an update to the AIP-31 that has > happened while we have been implementing this. > > When AIP-31 was proposed, the proposal mentioned that all operators would > have a __call__ method which would be used to define DAG in a functional > manner. While implementing this with the SIG Functional Ops team, we > decided to change that requirement and replace it with a better integration > with templated_fields and a new property in BaseOperator called output. The > idea is that (see AIP DAG example for variable reference) get_ip() == > get_ip.output. > > I updated the AIP-31 doc to reflect those changes. Also took the chance to > clean the doc a bit to reword some smaller stuff, in order to make it > easier for consumption by people outside of this group in the future. > Update diff: > https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=148638736&selectedPageVersions=12&selectedPageVersions=11 > Updated AIP-31: > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=148638736 > > Regarding the output property change, you can follow this discussion in > the current Airflow PR between me and Ash, to understand better the > motivations behind it: > https://github.com/apache/airflow/pull/8962/files#r441004317. > > A short summary: > The main reason for this change was that sometimes templated args > in operators are required on initialization > (example: subject on EmailOperator). This meant that you needed to add a > placeholder value on initialization, and then specify the XComArg that > needed to be used instead in the __call__ method. This seemed a bit > confusing, so when implementing we decided to rollback > the __call__ addition to BaseOperator. > > This made it difficult to get XComArg from an operator as the > initializer returns an instance of the operator instead of > an XComArg compared to the proposed __call__ method. For that, we proposed > to add a new property output to BaseOperator that would generate an XComArg > for the operator with return_value as the default key. > > An example of how different the DAG looks between the previous AIP and the > new update can be found here: > https://github.com/apache/airflow/pull/8962/files#r441013469 > > With that said, I will follow Ash recommendation and try to get a lazy > consensus on this change: > Allow 48h for committers and developers to comment/bring up > objections on the change. > > If no objection is raised by Thursday June 18th at 1PM MST, we will move > forward with the change and merge > https://github.com/apache/airflow/pull/8962 PR. > > Thanks everyone for all the work on this change! 🎉 > > Gerard Casas Saez > Twitter | Cortex | @casassaez >
