Repository navigation
Optimisation: Allow prefetch of LazyXComSequence in next method of LazyXComIterator #57975
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
Changes from all commits
15effd9
a7c1240
d116f99
c8839ac
521b112
afb417c
8e01303
3c0e567
42f6cb4
44367f8
55c1eb8
dbc4d37
0390a36
a087bf0
72dfc4d
822ed58
d8013ab
b8be901
ed3856e
67f41af
964c34a
bf7052f
861bf0b
25d8e08
18b9a1b
65270a9
7e4694f
299cf40
dca59f3
af6ec60
40e3bd7
1384e1a
03a92c2
76ded73
849599e
b66275f
5968835
2d4676f
d8406ce
7447f9a
71f076b
b2a3e1d
37957a7
d4588b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,12 +19,16 @@ | |
|
|
||
| import collections | ||
| import itertools | ||
| from collections import deque | ||
| from collections.abc import Iterator, Sequence | ||
| from typing import TYPE_CHECKING, Any, Literal, TypeVar, overload | ||
|
|
||
| import attrs | ||
| import structlog | ||
|
|
||
| from airflow.configuration import conf | ||
| from airflow.sdk.definitions._internal.logging_mixin import LoggingMixin | ||
|
|
||
| if TYPE_CHECKING: | ||
| from airflow.sdk.definitions.xcom_arg import PlainXComArg | ||
| from airflow.sdk.execution_time.task_runner import RuntimeTaskInstance | ||
|
|
@@ -35,25 +39,40 @@ | |
| # ``XCom.deserialize_value``. We don't want to wrap the API values in a nested | ||
| # {"value": value} dict since it wastes bandwidth. | ||
| _XComWrapper = collections.namedtuple("_XComWrapper", "value") | ||
|
|
||
| log = structlog.get_logger(logger_name=__name__) | ||
|
|
||
|
|
||
| def _deque_factory() -> deque: | ||
| return deque(maxlen=conf.getint("core", "parallelism")) | ||
|
dabla marked this conversation as resolved.
|
||
|
|
||
|
|
||
| @attrs.define | ||
| class LazyXComIterator(Iterator[T]): | ||
| class LazyXComIterator(LoggingMixin, Iterator[T]): | ||
| seq: LazyXComSequence[T] | ||
| index: int = 0 | ||
| dir: Literal[1, -1] = 1 | ||
| _buffer: deque[T] = attrs.field(factory=_deque_factory, init=False) | ||
|
|
||
| @property | ||
| def prefetch_size(self) -> int: | ||
| return self._buffer.maxlen or conf.getint("core", "parallelism") | ||
|
|
||
| def __next__(self) -> T: | ||
| if self.index < 0: | ||
| # When iterating backwards, avoid extra HTTP request | ||
| raise StopIteration() | ||
| try: | ||
| val = self.seq[self.index] | ||
| except IndexError: | ||
| raise StopIteration from None | ||
|
|
||
| # If the buffer is empty, fetch the next chunk | ||
| if not self._buffer: | ||
| chunk = list(self.seq[self.index : self.index + self.prefetch_size]) | ||
| if not chunk: | ||
| raise StopIteration() | ||
| self._buffer.extend(chunk) | ||
| self.log.debug("Buffered %s XCom's", len(self._buffer)) | ||
|
|
||
| val = self._buffer.popleft() | ||
| self.index += self.dir | ||
| self.log.debug("Popped buffered XCom for index %s: %s", self.index, val) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has of course a drawback of consuming too many of those. I think before we merge it, we need to consider certain scenarios and better way of controlling it so that users can control prefetching. And of course explain in the docs all kinds of usages.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and there I have to come back to me above response regarding your why 'core.paralellis' question? I don't see directly how we could for example derive the 'max_active_tis_per_dagrun' parameter from the operator from where the Xcom is being processed, as that would make it already less rigid.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or we introduce the 'BufferedXComIterator' I posted above, so that you can programmatically decide if you want to do buffered iteration or not. But then it's not out of the box, but you can decide the buffer size used for paging.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel it’d be a good idea to have a separate interface. The appropriate value here varies per-task or even per-XCom, so a deployment-wide config wouldn’t be too useful for anyone. Maybe a very small default would kind of make sense, and if the amount of entries is small enough we could even just send everything regardless, but I feel this should be done more automatically at the server side instead, where more context is available to make a better decision.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fully agree on that, I tried to search for better solution but couldn't come up with one that easily
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Don't see how you could do that on server side, as it is the client that determines how many iterations it will do, and thus how many XCom's it wants to retrieve, that just the part I'm struggling with.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about a kwarg on BaseOperator? |
||
| return val | ||
|
|
||
| def __iter__(self) -> Iterator[T]: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| import pytest | ||
|
|
||
| import airflow | ||
| from airflow.configuration import conf | ||
| from airflow.sdk.bases.xcom import BaseXCom | ||
| from airflow.sdk.exceptions import ErrorType | ||
| from airflow.sdk.execution_time.comms import ( | ||
|
|
@@ -74,35 +75,75 @@ def test_len(mock_supervisor_comms, lazy_sequence): | |
|
|
||
|
|
||
| def test_iter(mock_supervisor_comms, lazy_sequence): | ||
| stop = conf.getint("core", "parallelism") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of reading the conf, use |
||
| it = iter(lazy_sequence) | ||
|
|
||
| mock_supervisor_comms.send.side_effect = [ | ||
| XComSequenceIndexResult(root="f"), | ||
| ErrorResponse(error=ErrorType.XCOM_NOT_FOUND, detail={"oops": "sorry!"}), | ||
| XComSequenceSliceResult(root=["f"]), | ||
| ] | ||
| assert list(it) == ["f"] | ||
| mock_supervisor_comms.send.assert_has_calls( | ||
| [ | ||
| call( | ||
| msg=GetXComSequenceItem( | ||
| key=BaseXCom.XCOM_RETURN_KEY, | ||
| dag_id="dag", | ||
| task_id="task", | ||
| run_id="run", | ||
| offset=0, | ||
| ), | ||
| ), | ||
| call( | ||
| msg=GetXComSequenceItem( | ||
| key=BaseXCom.XCOM_RETURN_KEY, | ||
| dag_id="dag", | ||
| task_id="task", | ||
| run_id="run", | ||
| offset=1, | ||
| ), | ||
| ), | ||
| ] | ||
| ) | ||
|
|
||
| assert mock_supervisor_comms.send.call_args_list == [ | ||
| call( | ||
| GetXComSequenceSlice( | ||
| key=BaseXCom.XCOM_RETURN_KEY, | ||
| dag_id="dag", | ||
| task_id="task", | ||
| run_id="run", | ||
| start=0, | ||
| stop=stop, | ||
| step=None, | ||
| ) | ||
| ), | ||
| call( | ||
| GetXComSequenceSlice( | ||
| key=BaseXCom.XCOM_RETURN_KEY, | ||
| dag_id="dag", | ||
| task_id="task", | ||
| run_id="run", | ||
| start=1, | ||
| stop=stop + 1, | ||
| step=None, | ||
| ) | ||
| ), | ||
| ] | ||
|
|
||
|
|
||
| def test_iter_when_xcom_not_found(mock_supervisor_comms, lazy_sequence): | ||
| stop = conf.getint("core", "parallelism") | ||
| it = iter(lazy_sequence) | ||
|
|
||
| mock_supervisor_comms.send.side_effect = [ | ||
| XComSequenceSliceResult(root=["f"]), | ||
| ErrorResponse(error=ErrorType.XCOM_NOT_FOUND, detail={"oops": "sorry!"}), | ||
| ] | ||
| with pytest.raises(TypeError): | ||
| list(it) | ||
|
|
||
| assert mock_supervisor_comms.send.call_args_list == [ | ||
| call( | ||
| GetXComSequenceSlice( | ||
| key=BaseXCom.XCOM_RETURN_KEY, | ||
| dag_id="dag", | ||
| task_id="task", | ||
| run_id="run", | ||
| start=0, | ||
| stop=stop, | ||
| step=None, | ||
| ) | ||
| ), | ||
| call( | ||
| GetXComSequenceSlice( | ||
| key=BaseXCom.XCOM_RETURN_KEY, | ||
| dag_id="dag", | ||
| task_id="task", | ||
| run_id="run", | ||
| start=1, | ||
| stop=stop + 1, | ||
| step=None, | ||
| ) | ||
| ), | ||
| ] | ||
|
|
||
|
|
||
| def test_getitem_index(mock_supervisor_comms, lazy_sequence): | ||
|
|
||
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.
This should import from
airflow.sdkinstead.