potiuk commented on PR #70456: URL: https://github.com/apache/airflow/pull/70456#issuecomment-5149160463
Thanks for digging into this, and sorry to close it — but the premise doesn't hold up. The description says passing `shell=True` "is forwarded to `subprocess.Popen`, where it conflicts with the value supplied internally and raises a `TypeError`". That isn't what happens. `shell` is an explicit keyword-only parameter on `BaseEventLoop.subprocess_shell`, defaulting to `True`, so it binds to that parameter and never reaches `**kwargs` or `Popen`: ```python >>> import asyncio, subprocess >>> async def main(): ... p = await asyncio.create_subprocess_shell( ... "echo hi", shell=True, ... stdout=subprocess.PIPE, stderr=subprocess.PIPE, close_fds=True) ... out, _ = await p.communicate() ... print(out.strip()) >>> asyncio.run(main()) b'hi' ``` No exception. The async Beam path was never broken, so there's no bug here to fix. That leaves the change as removing an argument that duplicates the default. That's mildly tidier, but not worth carrying on its own, and two things argue against merging it as written: - The description would become the squash commit message, so we'd ship a changelog entry claiming a crash fix for a crash that doesn't occur. - `assert_awaited_once_with(...)` pins the exact keyword set of a third-party call. Any future legitimate argument — `env=`, say — breaks the test without anything being wrong, and it guards a cosmetic property rather than behaviour. One thing worth taking from this beyond the PR itself: the Testing section shows the new test was run, but not the `TypeError` itself. When a fix is premised on an error, the most valuable thing to check first is that the error actually reproduces on `main` — that would have caught this before the work went in. Especially worth doing when an assistant supplied the diagnosis; they are confident about failure modes that don't exist. Closing this one. Please don't let it put you off — the investigation was well written up, and that habit is worth keeping. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting -- 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]
