yuqian90 commented on pull request #10930: URL: https://github.com/apache/airflow/pull/10930#issuecomment-692603352
> > I think `get_roots()` and `get_leaves()` will be able to replace all the `isinstance` checks in set_upstream/set_downstream of `XComArg`, `TaskGroup`, `BaseOperator`. > > I must admit that I didn't get the `get_roots` and `get_leaves` idea fully. However, I implemented the `operator` property that should return `BaseOperator` and I think it serves the same purpose. WDYT @yuqian90 ? I see. For singular operators such as `XComArg` and `BaseOperator`, having a single `operator` property would suffice. However, when it comes to a group of operators with their own inter-dependencies, I don't think it's sufficient to return all the operators in the group. For example, when we do this, where operator1 is a singular operator, and group1 is a TaskGroup, ``` operator1 >> group1 ``` We only want to set all the "roots" in group1 as downstream of `operator1`. So the `operator1.set_downstream()` somehow needs to have a way to get all the "roots" of group1. Here, "roots" means all the tasks within group1 that do not have any upstream dependencies within group1. Same idea applies for "leaves" when we do this. Only the "leaves" in group1 needs to be pulled out and set upstream of operator1. ``` operator1 << group1 ``` I think what you have at the moment (operator property) is good enough though for this PR's purpose. The "roots" vs "leaves" problem is very specific to TaskGroup. I'm fine with having such logic live within TaskGroup. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
