Fix edge detection iteration #26188
Fix edge detection iteration #26188
Conversation
jedcunningham
requested review from
ryanahamilton,
ashb and
bbovenzi
as code owners
jedcunningham
added
the
changelog:skip
jedcunningham
force-pushed
the
fix_edge_detection_iteration
branch
from
7a6bb1e to
486b54f
Compare
| def downstream_list(self) -> Iterable["DAGNode"]: | ||
| def downstream_list(self) -> Iterable["Operator"]: |
There was a problem hiding this comment.
Oh I don’t think you can do this… Many had tried this before and failed 🙂
There was a problem hiding this comment.
Interesting, get_task is apparently returning an Operator?
There was a problem hiding this comment.
Yes but… well this thing is a mess.
There was a problem hiding this comment.
@uranusjr Does this need changing or is it fine as it is? (It's passing type checks)
| tasks_to_trace_next: List[Operator] = [] | ||
| for task in tasks_to_trace: | ||
| for child in task.downstream_list: | ||
| edge = (task.task_id, child.task_id) |
There was a problem hiding this comment.
Use node_id here to avoid task and child needing to be an Operator.
|
Curious, I wonder why that PR passed it's tests |
In apache#26175, edge detection was changed to use iteration instead of recursion, however it wasn't keeping track of the parent tasks. This changes the set we iterate over to be `(task, {children})` pairs instead of just adding all the children tasks themselves.
ashb
force-pushed
the
fix_edge_detection_iteration
branch
from
3a5fb97 to
cfb7baa
Compare
|
@ashb Because the change was only in "www" and "Core" tests" were skipped with "Selective checks" because of that. Apparently the "core tests" as defined now have some implicit dependency on www code. I will trace it and fix it. |
|
That also help to track such cases where either the tests or the code have some implicit dependencies or testing unrelated code. |
|
Yes. In this case there are two solutions (the first is way worse):
or (probably way better solution)
I am happy to add PR to do the latter - in this case, this failure simply uncovered an implicit dependency that we missed that should be removed. |
|
Fix (moving dag_edges and task_groups_to_dict in #26212. It depends on this one, so it should be merged after. |
|
Ahhhh yeah that makes sense. The price we pay for having quicker tests is we sometimes miss things like this. (Totally worth it!) |
|
This is passing it's tests, if you want to fix up types @uranusjr we can do that in a follow up PR. |
The methods were implemented in "view" but they were used in other places (in dot_renderer) so they conceptually belong to common util code. Having those in a wrong package (airflow/www) caused the tests to pass because selective checks did nor realise that change in "airflow/www" also requires running Core tests. This PR moves the methods to "airflow/utils". The methods are imported in the "views" module so even if someone used them from there, they will stil be available there, so the change is fully backwards compatible (even if those are not "public" airflow API methods. Follow up after apache#26188
…26212) The methods were implemented in "view" but they were used in other places (in dot_renderer) so they conceptually belong to common util code. Having those in a wrong package (airflow/www) caused the tests to pass because selective checks did nor realise that change in "airflow/www" also requires running Core tests. This PR moves the methods to "airflow/utils". The methods are imported in the "views" module so even if someone used them from there, they will stil be available there, so the change is fully backwards compatible (even if those are not "public" airflow API methods. Follow up after #26188
In #26175, edge detection was changed to use iteration instead of recursion, however it wasn't keeping track of the parent task.