Skip to content

Dont use TaskInstance in CeleryExecutor.trigger_tasks#16248

Merged
mik-laj merged 5 commits into
apache:mainfrom
mik-laj:celery-multiprocessing-2
Jul 27, 2021
Merged

Dont use TaskInstance in CeleryExecutor.trigger_tasks#16248
mik-laj merged 5 commits into
apache:mainfrom
mik-laj:celery-multiprocessing-2

Conversation

@mik-laj

@mik-laj mik-laj commented Jun 3, 2021

Copy link
Copy Markdown
Member

We don't use this object so we don't need to send it.

Sending TI by multiprocessing sometimes fails because TaskInstance is not always properly serialized

/home/airflow/.local/lib/python3.6/site-packages/airflow/models/dag.py:1720: in run
    job.run()
/home/airflow/.local/lib/python3.6/site-packages/airflow/jobs/base_job.py:237: in run
    self._execute()
/home/airflow/.local/lib/python3.6/site-packages/airflow/utils/session.py:70: in wrapper
    return func(*args, session=session, **kwargs)
/home/airflow/.local/lib/python3.6/site-packages/airflow/jobs/backfill_job.py:806: in _execute
    session=session,
/home/airflow/.local/lib/python3.6/site-packages/airflow/utils/session.py:67: in wrapper
    return func(*args, **kwargs)
/home/airflow/.local/lib/python3.6/site-packages/airflow/jobs/backfill_job.py:727: in _execute_for_run_dates
    session=session,
/home/airflow/.local/lib/python3.6/site-packages/airflow/utils/session.py:67: in wrapper
    return func(*args, **kwargs)
/home/airflow/.local/lib/python3.6/site-packages/airflow/jobs/backfill_job.py:602: in _process_backfill_task_instances
    executor.heartbeat()
/home/airflow/.local/lib/python3.6/site-packages/airflow/executors/base_executor.py:158: in heartbeat
    self.trigger_tasks(open_slots)
/home/airflow/.local/lib/python3.6/site-packages/airflow/executors/celery_executor.py:263: in trigger_tasks
    self._process_tasks(task_tuples_to_send)
/home/airflow/.local/lib/python3.6/site-packages/airflow/executors/celery_executor.py:272: in _process_tasks
    key_and_async_results = self._send_tasks_to_celery(task_tuples_to_send)
/home/airflow/.local/lib/python3.6/site-packages/airflow/executors/celery_executor.py:333: in _send_tasks_to_celery
    send_task_to_executor, task_tuples_to_send, chunksize=chunksize
/usr/local/lib/python3.6/multiprocessing/pool.py:266: in map
    return self._map_async(func, iterable, mapstar, chunksize).get()
/usr/local/lib/python3.6/multiprocessing/pool.py:644: in get
    raise self._value
/usr/local/lib/python3.6/multiprocessing/pool.py:424: in _handle_tasks
    put(task)
/usr/local/lib/python3.6/multiprocessing/connection.py:206: in send
    self._send_bytes(_ForkingPickler.dumps(obj))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'multiprocessing.reduction.ForkingPickler'>
obj = (0, 0, <function mapstar at 0x7f3d2c0c97b8>, ((<function send_task_to_executor at 0x7f3d0b8faf28>, ((TaskInstanceKey(d...sk: airflow.executors.celery_executor.execute_command of airflow.executors.celery_executor at 0x7f3d0b68dcf8>))),), {})
protocol = None

    @classmethod
    def dumps(cls, obj, protocol=None):
        buf = io.BytesIO()
>       cls(buf, protocol).dump(obj)
E       TypeError: cannot serialize 'EncodedFile' object

/usr/local/lib/python3.6/multiprocessing/reduction.py:51: TypeError

Additionally, we have wrong typehint/variable names. self.queued_tasks contains OrderedDict[TaskInstanceKey, QueuedTaskInstanceType] objects where QueuedTaskInstanceType is defined as Tuple[CommandType, int, Optional[str], TaskInstance].

self.queued_tasks: OrderedDict[TaskInstanceKey, QueuedTaskInstanceType] = OrderedDict()

QueuedTaskInstanceType = Tuple[CommandType, int, Optional[str], TaskInstance]

def order_queued_tasks_by_priority(self) -> List[Tuple[TaskInstanceKey, QueuedTaskInstanceType]]:

Unfortunately, we were reading the values and writing them to the variable simple_ti, but it's not valid. It should be ti.
sorted_queue = self.order_queued_tasks_by_priority()

key, (command, _, queue, simple_ti) = sorted_queue.pop(0)

I also confirmed it via the ipdb.
Screenshot 2021-06-03 at 04 41 56

I tested it in my E2E environment: mik-laj/airflow-testing-example#2

Supersede: #16231


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg Bot added the area:Scheduler including HA (high availability) scheduler label Jun 3, 2021
@mik-laj
mik-laj requested review from ephraimbuddy and potiuk June 3, 2021 16:22
@github-actions

github-actions Bot commented Jun 7, 2021

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 7, 2021
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 26, 2021
@kaxil kaxil added pinned Protect from Stalebot auto closing and removed stale Stale PRs per the .github/workflows/stale.yml policy file labels Jul 26, 2021
@kaxil

kaxil commented Jul 26, 2021

Copy link
Copy Markdown
Member

ping @mik-laj

@mik-laj

mik-laj commented Jul 26, 2021

Copy link
Copy Markdown
Member Author

Updated

@mik-laj
mik-laj merged commit 0801322 into apache:main Jul 27, 2021
nathadfield pushed a commit to king/airflow that referenced this pull request Jul 29, 2021
* Dont use TaskInstance in CeleryExecutor.trigger_tasks

* fixup! Merge branch 'main' into celery-multiprocessing-2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Scheduler including HA (high availability) scheduler full tests needed We need to run full set of tests for this PR to merge pinned Protect from Stalebot auto closing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants