Install pyyaml in action composite for host Python orchestration - #2292
Conversation
#2279 added actions/setup-python@v5 to pin host Python to 3.14, but setup-python provides a clean interpreter without project deps. The orchestration command python3 -c "from egg_lib.gha_exec import gha_exec; ..." transitively imports shared/egg_config/config.py, which `import yaml`s and exits with "PyYAML is required" when the module is missing. Add a minimal `pip install pyyaml` step after setup-python. PyYAML is the only non-stdlib dep on the gha_exec → egg_config import path (verified by walking imports across egg_lib + egg_config), so we don't need the heavier `uv sync --extra dev` pattern used in test/lint workflows.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review
The fix is correct and the scope is right. I traced the import chain to confirm the PR's claim before approving the approach.
Verification of PR claim
I reproduced the bug locally and confirmed PyYAML is the only third-party dep on the from egg_lib.gha_exec import gha_exec path:
$ PYTHONPATH=sandbox:shared python3 -c "import sys; from egg_lib.gha_exec import gha_exec; ..."
Top-level non-stdlib modules: yaml
Concrete chain:
egg_lib.gha_exec→egg_lib.runtime→egg_container→from egg_config import GATEWAY_PROXY_PORT→egg_config/__init__.py→from .config import Config, get_local_repos, ...→shared/egg_config/config.py:11–15(thetry: import yaml / except ImportError: sys.exit(1)block).- Independently,
egg_lib.runtimealso pulls inegg_lib.docker(docker.py:19 import yaml) andegg_lib.gateway(gateway.py:23 import yaml) at module top, so even removing theegg_configsys.exitwould still require yaml.
So pip install pyyaml is the minimum viable fix. The choice to install one package rather than uv sync or a full requirements file is defensible — the import surface here is tiny and only depends on PyYAML.
Non-blocking observations
-
Unpinned version.
pip install pyyamlresolves to whatever the latest is at run time. PyYAML has been stable for years and PyYAML 6.0.3 added Python 3.14 wheels (which is whatsetup-python@v5withpython-version: "3.14"will pick up), so this is unlikely to bite, but a defensivepyyaml>=6,<7would protect the composite from a future breaking release without much cost. Optional. -
No pip cache. Every action invocation re-downloads PyYAML. The wheel is small (~700KB), so the latency is negligible — but
actions/setup-pythonsupportscache: pipwith arequirements.txt-style file. Not worth the bookkeeping for a single dep; flagging only for completeness. -
shared/egg_config/config.py:11–15doessys.exit(1)at import time when yaml is missing. That's an aggressive failure mode for a library module and it's why the composite sawError: PyYAML is requiredrather than a normalModuleNotFoundError. Out of scope for this PR, but worth noting: a library module shouldn'tsys.exiton import — it should let theImportErrorpropagate so callers can catch it. A future cleanup. -
No automated test added. The test plan is manual. That's reasonable for an action composite step that only runs in GHA — there isn't a clean unit-test surface for "did pip install succeed in a real GHA runner." Calling it out for awareness, not as a blocker.
Verdict
Self-authored PR (already merged), commenting only. No blocking issues. The fix is correct, minimal, and the import-path analysis in the PR description is accurate.
— Authored by egg
|
egg review completed. View run logs 1 previous review(s) hidden. |
Summary
actions/setup-python@v5to pin host Python to 3.14, butsetup-pythonprovides a clean interpreter with no project deps. The orchestration commandpython3 -c "from egg_lib.gha_exec import gha_exec; ..."transitively importsshared/egg_config/config.py, whichimport yamls and exits withError: PyYAML is required. Observed on PR Fix #2242: alive-signal gate on heartbeat/progress alerts; plan-phase post-ACK threshold #2268'segg-reviewer-reviewjob (run).pip install pyyamlstep after setup-python inaction/action.yml. PyYAML is the only non-stdlib dep on thegha_exec → egg_configimport path (verified by walking imports acrosssandbox/egg_lib/*.pyandshared/egg_config/*.py), so this is tighter than theuv sync --extra devpattern used in test/lint workflows.Test plan
egg-reviewer-reviewjob on PR Fix #2242: alive-signal gate on heartbeat/progress alerts; plan-phase post-ACK threshold #2268 (or push a new commit to it) once this lands and confirm it gets past=== Step 3: Python orchestration ===without the PyYAML error.