You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dstandish (via GitHub)" <gi...@apache.org> on 2023/05/25 21:33:40 UTC
[GitHub] [airflow] dstandish opened a new pull request, #31559: Remove found_descendents param from get_flat_relative_ids
dstandish opened a new pull request, #31559:
URL: https://github.com/apache/airflow/pull/31559
By the looks of it, this param is unused. Since the class is designated private, it is permissable to remove it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] XD-DENG commented on a diff in pull request #31559: Remove found_descendents param from get_flat_relative_ids
Posted by "XD-DENG (via GitHub)" <gi...@apache.org>.
XD-DENG commented on code in PR #31559:
URL: https://github.com/apache/airflow/pull/31559#discussion_r1208124909
##########
airflow/models/abstractoperator.py:
##########
@@ -154,37 +154,38 @@ def get_direct_relative_ids(self, upstream: bool = False) -> set[str]:
return self.upstream_task_ids
return self.downstream_task_ids
- def get_flat_relative_ids(
- self,
- upstream: bool = False,
- found_descendants: set[str] | None = None,
- ) -> set[str]:
- """Get a flat set of relative IDs, upstream or downstream."""
+ def get_flat_relative_ids(self, *, upstream: bool = False) -> set[str]:
+ """
+ Get a flat set of relative IDs, upstream or downstream.
+
+ Will recurse each relative found in the direction specified.
+
+ :param upstream: Whether to look for upstream or downstream relatives.
Review Comment:
Given we are adding the doctoring for it anyway, shall we add the line for type as well?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #31559: Remove found_descendents param from get_flat_relative_ids
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #31559:
URL: https://github.com/apache/airflow/pull/31559#discussion_r1208130322
##########
airflow/models/abstractoperator.py:
##########
@@ -154,37 +154,38 @@ def get_direct_relative_ids(self, upstream: bool = False) -> set[str]:
return self.upstream_task_ids
return self.downstream_task_ids
- def get_flat_relative_ids(
- self,
- upstream: bool = False,
- found_descendants: set[str] | None = None,
- ) -> set[str]:
- """Get a flat set of relative IDs, upstream or downstream."""
+ def get_flat_relative_ids(self, *, upstream: bool = False) -> set[str]:
+ """
+ Get a flat set of relative IDs, upstream or downstream.
+
+ Will recurse each relative found in the direction specified.
+
+ :param upstream: Whether to look for upstream or downstream relatives.
Review Comment:
hi @XD-DENG, i think that the types are now inferred from the annotations in the signature
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #31559: Remove found_descendents param from get_flat_relative_ids
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #31559:
URL: https://github.com/apache/airflow/pull/31559#discussion_r1206132697
##########
airflow/models/abstractoperator.py:
##########
@@ -154,30 +154,31 @@ def get_direct_relative_ids(self, upstream: bool = False) -> set[str]:
return self.upstream_task_ids
return self.downstream_task_ids
- def get_flat_relative_ids(
- self,
- upstream: bool = False,
- found_descendants: set[str] | None = None,
- ) -> set[str]:
- """Get a flat set of relative IDs, upstream or downstream."""
+ def get_flat_relative_ids(self, upstream: bool = False) -> set[str]:
Review Comment:
done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #31559: Remove found_descendents param from get_flat_relative_ids
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #31559:
URL: https://github.com/apache/airflow/pull/31559#issuecomment-1564980961
> This is technically breaking backward compatibility (the user _can_ technically access this function is DAGs) but I’m willing to take the chance and assume no-one is actually using it.
@uranusjr but then what does `meta private` mean in the class? i thought we took that to mean thou shalt not depend on anything in the class and we may change at any time?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #31559: Remove found_descendents param from get_flat_relative_ids
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #31559:
URL: https://github.com/apache/airflow/pull/31559#discussion_r1206086272
##########
airflow/models/abstractoperator.py:
##########
@@ -154,30 +154,31 @@ def get_direct_relative_ids(self, upstream: bool = False) -> set[str]:
return self.upstream_task_ids
return self.downstream_task_ids
- def get_flat_relative_ids(
- self,
- upstream: bool = False,
- found_descendants: set[str] | None = None,
- ) -> set[str]:
- """Get a flat set of relative IDs, upstream or downstream."""
+ def get_flat_relative_ids(self, upstream: bool = False) -> set[str]:
Review Comment:
Should we kwarg only it while we are at it? There is only 1 use that would need adjusting.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #31559: Remove found_descendents param from get_flat_relative_ids
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #31559:
URL: https://github.com/apache/airflow/pull/31559#discussion_r1206045262
##########
airflow/models/abstractoperator.py:
##########
@@ -154,30 +154,31 @@ def get_direct_relative_ids(self, upstream: bool = False) -> set[str]:
return self.upstream_task_ids
return self.downstream_task_ids
- def get_flat_relative_ids(
- self,
- upstream: bool = False,
- found_descendants: set[str] | None = None,
Review Comment:
removed this param here
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish merged pull request #31559: Remove found_descendents param from get_flat_relative_ids
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish merged PR #31559:
URL: https://github.com/apache/airflow/pull/31559
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a diff in pull request #31559: Remove found_descendents param from get_flat_relative_ids
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #31559:
URL: https://github.com/apache/airflow/pull/31559#discussion_r1206045382
##########
airflow/models/abstractoperator.py:
##########
@@ -154,30 +154,31 @@ def get_direct_relative_ids(self, upstream: bool = False) -> set[str]:
return self.upstream_task_ids
return self.downstream_task_ids
- def get_flat_relative_ids(
- self,
- upstream: bool = False,
- found_descendants: set[str] | None = None,
- ) -> set[str]:
- """Get a flat set of relative IDs, upstream or downstream."""
+ def get_flat_relative_ids(self, upstream: bool = False) -> set[str]:
+ """
+ Get a flat set of relative IDs, upstream or downstream.
+
+ Will recurse each relative found in the direction specified.
+
+ :param upstream: Whether to look for upstream or downstream relatives.
+ """
dag = self.get_dag()
if not dag:
return set()
- if found_descendants is None:
- found_descendants = set()
Review Comment:
thus no longer need this conditional
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #31559: Remove found_descendents param from get_flat_relative_ids
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #31559:
URL: https://github.com/apache/airflow/pull/31559#issuecomment-1565238229
> > This is technically breaking backward compatibility (the user _can_ technically access this function is DAGs) but I’m willing to take the chance and assume no-one is actually using it.
>
> @uranusjr but then what does `meta private` mean in the class? i thought we took that to mean thou shalt not depend on anything in the class and we may change at any time?
Yes. I think :meta private: works as designed.
And we actually have now a very good way to assess if something is breaking.
https://airflow.apache.org/docs/apache-airflow/stable/public-airflow-interface.html
Adding the page was one of the reasons to make such decisions quiickly
* we do not have abstractoperator package nor AbstractOperator there
* there is no found_descendents param described there
That technicall means it's not part of the public interface and we are free to change it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org