kaxil commented on PR #65587:
URL: https://github.com/apache/airflow/pull/65587#issuecomment-5134099345
Have you been able to run the repro from #65482 against this branch? I
applied it locally and the reported scenario still hangs, so I don't think this
closes that issue.
What I ran: the script from the issue, with `system_site_packages=True` and
no PyPI requirements so the venv resolves to local sources, the variable stored
only in the metastore, under `dag.test()`.
On main the run hangs, and the log shows `Using Variable.get from
'airflow.models' is deprecated` coming from
`airflow-core/src/airflow/models/variable.py:170` with a `correlation_id`
attached, so the Execution API handler did take the Task SDK path. That part
matches your diagnosis.
With this branch applied the warning is gone and the handler resolves
through the metastore, so `should_use_task_sdk_api_path()` does what you
intend. The run still hangs. A faulthandler dump inside the venv child shows it
parked here:
```
File ".../sdk/execution_time/comms.py", line 332 in _read_frame
File ".../sdk/execution_time/comms.py", line 371 in _get_response
File ".../sdk/execution_time/comms.py", line 275 in send
File ".../sdk/execution_time/secrets/execution_api.py", line 119 in
get_variable
```
The child never gets a response, because `InProcessTestSupervisor.send_msg`
(`task-sdk/src/airflow/sdk/execution_time/supervisor.py`, line 2133 on main)
appends to the in-process deque rather than writing a response frame to the
socket that `_setup_subprocess_socket()` handed the child:
```python
def send_msg(self, msg, request_id, error=None, **dump_opts):
"""Override to use in-process comms."""
self.comms.messages.append(msg)
```
`_handle_request` answers every request through `send_msg`
(`supervisor.py:1918`), and the only thing writing to that socket directly is
`_send_new_log_fd`. That's why the logging-FD handshake added in #57212 works
from a venv task while `GetVariable` and `GetConnection` never come back.
If I route responses for socket-delivered requests back onto the socket, the
repro passes on plain main, without this branch. Applying this branch on top
produces the same value, minus one round trip through the SDK client and the
deprecation warning. In the in-process path the nested call is already covered
by `set_supervisor_comms(None)` in `InProcessSupervisorComms.send`, which is
why the detour still resolves to the right value today.
So as far as I can tell this is a robustness fix on that path rather than
the fix for #65482.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
