chore: migrate from Poetry to uv - #172
Conversation
- pyproject.toml: convert to PEP 621 with `[tool.uv].package = false` (this repo runs scripts + tests, never publishes a wheel). - pyproject.toml: bring `pytest` + `pytest-cov` into the dev group so the unit-tests job no longer needs the separate `poetry run pip install --upgrade pytest-cov` shim. - Drop the workflow's `poetry run pip install --upgrade packaging` override. uv's universal resolver already picks up packaging 26.2 for us (verified in uv.lock); the old `--upgrade` line was working around Poetry's narrower selection from open-aea constraints and is no longer needed. - python-tests.yml: replace every `Install Poetry` / `Configure Poetry` / `poetry env use` / `poetry install` / `poetry run` block across all 5 jobs (e2e-test-run-service, e2e-test-staking, changes detector, e2e-test-migrate-to-pearl, unit-tests) with `astral-sh/setup-uv@v6` (pinned to 0.11.15) + `uv sync --all-groups --frozen` + `uv run pytest`. - changes-detector: update the diff allowlist from `poetry\.lock$` to `uv\.lock$` so the migration job still re-triggers on any dep bump. - Drop the workflow's "Clean Python cache" stanzas — they existed to wipe stale poetry venvs / poetry.lock before fresh installs; irrelevant under uv where `uv sync --frozen` is hermetic. - Drop poetry.lock; commit uv.lock. Local verification: - `uv lock --check` - `uv sync --all-groups --frozen` - runtime deps importable (operate, web3, docker, halo, gql, dotenv, tqdm, psutil, autonomy, aea) - unit-tests: 359 passing on macOS Python 3.14. 11 pre-existing failures in tests/test_scripts/test_pearl_migration.py are unrelated — `@dataclass(frozen=True) class _Unmigratable(Exception)` trips contextlib's `__traceback__` assignment on macOS Python 3.13+. Recent CI runs on the same workflow + matrix (last green: chore/deps-bump-starlette on 2026-05-18) pass on Linux. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The committed lockfile got pinned to `requires-python = ">=3.13"` from an ad-hoc `uv lock --python 3.13` I ran during local verification. That made the CI unit-tests matrix (3.10-3.14) fail with "Missing workspace member quickstart" because uv wouldn't accept the lock for 3.10/3.11/3.12 workers. Regenerated with the project's actual `>=3.10, <3.15` range; same 145 packages resolve. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 5 e2e jobs were red because they spawn `bash ./run_service.sh`, `./stop_service.sh`, and `./migrate_to_pearl.sh`, which still called `poetry install` + `poetry run`. CI no longer installs poetry, so every shelled-out command died at the `command -v poetry` check. Move the user-facing scripts to uv: - All 9 wrapper scripts: `poetry install --only main --no-cache` -> `uv sync --no-default-groups --no-cache --frozen`, `poetry run python -m ...` -> `uv run python -m ...`. The `--no-default-groups` flag picks only `[project].dependencies` (matches Poetry's `--only main` since this repo's `default-groups = "all"` would otherwise also install the dev group). - `run_service.sh` + `migrate_to_pearl.sh`: replace the `command -v poetry` prerequisite check with `command -v uv` and point at the uv install docs. - `run_service.sh`: drop the `poetry run pip install --upgrade packaging` line — uv's universal resolver already pulls `packaging==26.2` (verified in uv.lock). - README.md: bump the System Requirements list, the Pearl-migration description, and the Windows install steps from Poetry to uv; renumber the Windows steps after collapsing the two-step Poetry install into a single uv install. Local: `bash ./run_service.sh configs/config_predict_trader.json --help` no longer fails the prerequisite checks; `uv sync --no-default-groups --frozen --dry-run` confirms the lock satisfies the main-only set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`uv sync --no-default-groups` removed any packages not in the main set
between invocations — including the test driver's dev deps
(pexpect, pytest, pytest-cov). The CI flow is:
1. uv sync --all-groups --frozen (workflow setup: installs everything)
2. bash ./run_service.sh (script: ran uv sync --no-default-groups,
wiping pexpect/pytest from the venv)
3. bash ./stop_service.sh (same wipe again)
4. test asserts containers stopped (fails — driver dep state corrupted)
Poetry's `install --only main` was non-destructive; uv's `sync` is. Add
--inexact to the shell-script syncs so they install/upgrade the main
deps without pruning anything else. Also drop `--no-cache`: uv's
--no-cache rebuilds the whole resolve from a temp dir on every call
(much slower than Poetry's source-cache bypass, no value in a fast
local script).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… race `operate.cli quickstop` returns as soon as it issues the docker stop calls, without waiting for Tendermint + ABCI containers to flush and exit. Containers actually take 30-45s to come down cleanly. The 20s wait passed under Poetry only because each `poetry install --only main --no-cache` inside stop_service.sh added 30-60s of resolve+install overhead before the assertion fired. uv's sync is ~1s when the venv is warm, so we now hit the assertion before the containers finish stopping (visible across all e2e jobs: agents.fun, optimus, predict_trader, staking — all flagged "Containers ... still running" 20s after quickstop returned). Bumping the wait to 60s unmasks the test reliably. Proper fix lives in middleware (have `quickstop` block on `docker compose down --wait` or equivalent) and is out of scope for the uv migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tainers
`shutil.copytree('.', tempdir, ignore=...)` was including `.venv` in
the copy. A copied venv's `pyvenv.cfg` / shebangs still reference
the source path, so any in-tempdir `uv sync --inexact` preserves the
broken state — and `operate.cli quickstop` then runs from a
malformed interpreter that silently fails to enumerate the running
deployment, leaving the docker containers up after the test thinks
it stopped them.
Mirror the trick `test_migrate_to_pearl.py:_copy_repo` already uses:
exclude `.venv` from the copy and symlink it to the source venv so
the in-tempdir scripts reuse the project's actual interpreter.
This is what was actually causing the "Containers with name X are
still running" failure across every e2e job (agents.fun, optimus,
predict_trader, staking); bumping CONTAINER_STOP_WAIT in 477e4ae
was treating the symptom — the real issue was that quickstop was
never reaching the docker daemon.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DIvyaNautiyal07
left a comment
There was a problem hiding this comment.
Ripple sweep — concept-changed-but-not-everywhere
The main README.md is updated, but the migration left stale Poetry references in sub-trees and in a sibling test file's rationale. After this merges, a developer following these docs hits the same poetry: command not found the main script now avoids.
Sub-directory READMEs (P1):
scripts/optimus/README.mdlines 18-19 —poetry install,poetry run python -m scripts.optimus.migrate_legacy_optimus ...scripts/predict_trader/README.mdlines 16, 22, 28, 197-198 — multiplepoetry run/poetry install
Stale rationale in tests/test_migrate_to_pearl.py (P1):
- Line 95: the
_copy_repo_todocstring still says symlinking.venvavoids "a freshpoetry installin dest [that] triggers PEP 517 sdist rebuilds (halo, python-baseconv) that fail under Poetry 2.4 + setuptools >= 80." Under uv, the symptom may be different (or absent). The rationale should be re-justified or rewritten in uv terms — the new symlink fix intest_run_service.pycites a different reason (stalepyvenv.cfg), and the two docstrings now disagree. - Lines 342-343: comment says "We drop VIRTUAL_ENV so poetry inside the spawned shell creates / uses its own env" — should be "so uv inside the spawned shell...".
…guards Three follow-ups from DIvyaNautiyal07's review of the venv-symlink fix: - Replace the flat `time.sleep(CONTAINER_STOP_WAIT)` post-stop wait with a 2s-interval poll loop (`_wait_for_containers_stopped`) bounded by `CONTAINER_STOP_WAIT` (now a max-timeout, not a fixed sleep). Green CI is faster — exits as soon as docker confirms the containers are gone — and failures fail decisively at the timeout instead of racing. Bumped the timeout to 120s for headroom on loaded runners; the in-loop poll keeps that cheap on green. - Stop reusing CONTAINER_STOP_WAIT as the startup retry interval at line 933 (the old code happened to sleep `CONTAINER_STOP_WAIT * retries` waiting for docker to come up); use the dedicated `STARTUP_WAIT` constant so the stop bump doesn't leak into startup (50s instead of 600s of retry). - Reject the silent `if src_venv.is_dir():` skip in setup_class. If the venv is missing we'd land back at the broken-pyvenv-cfg state the symlink was meant to avoid, so raise `RuntimeError` with the exact `uv sync` command to fix it. - Skip the test class on non-POSIX platforms. `Path.symlink_to` needs Developer Mode / admin on Windows; we don't run e2e there (CI is ubuntu-only) but local Windows invocations would now exit with a clear `pytest.skip` instead of a confusing OSError mid-class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OjusWiZard
left a comment
There was a problem hiding this comment.
Additional findings from a multi-agent review pass over the diff at 870a0ba. Most-load-bearing items are the two unchecked _wait_for_containers_stopped returns (HIGH) and the unquoted $@ across the user-facing scripts (MEDIUM). The blocking dep-bump finding from @LOCKhart07 and the stale-Poetry sweep from @DIvyaNautiyal07 are not duplicated here.
Two additional observations on lines outside this PR's diff (so left here rather than inline):
tests/test_run_service.py:836—cls.temp_env.pop('POETRY_ACTIVE', None)is now dead, butUV_PROJECT_ENVIRONMENT(andUV_VENV) are not popped. A contributor running tests inside auv runshell will have those vars leak into thepexpect-spawned subprocesses, causinguv runinside them to target the outer project's venv instead of the tempdir-symlinked one — silently defeating the symlink isolation added at line 1050.tests/test_run_service.py:1107–1116—TestAgentService.teardown_classis a verbatim copy ofTempDirMixin.teardown_class(lines 1075–1084); since it inherits fromTempDirMixinthe override adds nothing and can be deleted. Independently, the innertry/except Exception:in the parent callscleanup_directory(...)with identical args in both branches — thetry/exceptis a no-op.
LOCKhart07's non-blocking follow-ups: - pyproject.toml: drop the `pytest>=8` / `pytest-cov>=5` ranges, pin exactly to the currently-resolved versions (`pytest==9.0.3`, `pytest-cov==7.1.0`). Matches the freeze policy main adopted in 7991670 — keeps a future `uv lock` from silently drifting the dev tooling alongside framework updates. - python-tests.yml unit-tests matrix: pass `--python ${{ matrix.python-version }}` to both `uv sync` and `uv run`. Without it uv picks any interpreter that satisfies `requires-python` (>=3.10,<3.15), so if a different compatible Python sits earlier on PATH the entire matrix collapses to that single version and the 3.10/3.11/3.12/3.13/3.14 coverage guarantee silently disappears. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Surface `_wait_for_containers_stopped` timeouts: return False from `ensure_service_stopped` and raise from `BaseTestService.teardown_class` so callers don't see a false success on poll exhaustion. - Quote `"$@"` in the seven sibling shell scripts so arguments with whitespace (e.g. a config path with a space) don't word-split. - Rewrite the `.venv` exclusion rationale on `TempDirMixin.setup_class` to match the broken-`pyvenv.cfg` explanation already present a few lines down, and refresh the `test_migrate_to_pearl._copy_repo_to` docstring so the cross-reference resolves to current (uv-era) text. - Move the POSIX guard above `shutil.copytree` so unsupported platforms skip cleanly without copying the repo first.
LOCKhart07
left a comment
There was a problem hiding this comment.
Review — uv migration
Most prior-round issues are genuinely resolved (verified below). One blocking item remains in the latest commit (8eb24c8).
🔴 Blocking — the teardown_class "raise on timeout" fix (3266762896) is non-functional
tests/test_run_service.py:874. The new raise RuntimeError(...) on poll timeout is lexically inside the inner try: (line 863), whose broad except Exception as e: at line 888 catches it (RuntimeError ⊂ Exception) and downgrades it to a single logger.error(...). Traced control flow:
- The exception never propagates out of
teardown_class→ pytest sees teardown return normally → the test class does not fail. This contradicts the resolution reply on 3266762896 ("the test class fails decisively"). - Because the raise is at line 874, before the force-stop fallback at lines 879–887 (
client.containers.list(...)→container.stop()/remove()), that cleanup is skipped on the timeout path. Straggler containers are neither force-removed nor surfaced — only a log line. This is the exact "containers leak into the next CI job" scenario the comment was meant to prevent, and a regression vs. the pre-8eb24c8flat-sleep behavior (which always fell through to force-stop).
Fix: keep the force-stop loop running on timeout, and make the failure decisive outside the swallowing except — e.g. set a flag in the inner block, then raise after line 891 (outside the inner try), so cleanup still runs and the timeout actually fails the class.
🟡 Non-blocking — same root cause in ensure_service_stopped
tests/test_run_service.py:487-488. if not _wait_for_containers_stopped(...): return False returns before the force-stop fallback at lines 491–500. On main the flat sleep always fell through to force-stop+remove leftovers; now on timeout they're left running and leak into the next parametrized config / CI job. The caller does raise on False (good signal), but the cleanup backstop is lost. Suggest force-stopping remaining containers before returning False.
🟡 Non-blocking — PR description "Note on lockstep" is stale
It says olas-operate-middleware==0.15.21 is "currently in flight via the middleware PR valory-xyz#442". It's already pinned on origin/main (commit 29b7e59). Reword or drop so it doesn't imply an unmerged dependency.
Verified resolved (no action needed)
- 3266447547 (LOCKhart07, "blocking" dep upgrades) is invalid — confirmed
pyproject.tomldeps are byte-for-byte identical to currentorigin/main(617f813); it was compared against a stale main snapshot. DhairyaPatel7's rebuttal is correct. "$@"quoting fixed across all 9 shell scripts; all usecommand -v uv.- Matrix
--python ${{ matrix.python-version }}applied to bothuv syncanduv run. - Dev pins exact (
pytest==9.0.3,pytest-cov==7.1.0);uv.lockconsistent (requires-python >=3.10,<3.15, virtual workspace member, all versions matchpyproject.toml). setup_classPOSIX-guard reorder is safe — pytest does not invoketeardown_classwhensetup_classraisesSkipped, so noAttributeErroron the unsetself.temp_dir.
`teardown_class`: the previous `raise RuntimeError` on `_wait_for_containers_stopped` timeout sat lexically inside an `except Exception as e:` block, which swallowed it and downgraded the signal to a log line. It also short-circuited the force-stop `client.containers.list(...)` / `container.stop()` fallback below, so stragglers leaked into the next CI job. Track the timeout via a flag instead, let the force-stop loop run regardless of poll outcome, and raise outside both swallowing `except` blocks so a timeout actually fails the test class. `ensure_service_stopped`: same shape — `return False` returned before the force-stop fallback at lines 491–500, leaving live containers behind on timeout. Set `poll_timed_out`, run the fallback, then `return not poll_timed_out` so the caller still gets the failure signal but cleanup completes first.
|
@LOCKhart07 — thank you, the swallowed- 🔴 Blocking — 🟡 Non-blocking — same root cause in 🟡 Non-blocking — PR description stale "in flight via valory-xyz#442". Reworded: Diff for the teardown rework: 07c33fa |
LOCKhart07
left a comment
There was a problem hiding this comment.
Verified 07c33fa — all three items from the prior review are properly resolved:
- Blocking (teardown_class): the
raise RuntimeError(...)is now at method-body indentation, outside both swallowingexcept Exceptionblocks, so a poll timeout propagates to pytest and the class fails decisively. The force-stop fallback (containers.list→stop()/remove()) runs regardless of poll outcome, so the cleanup backstop is preserved. ensure_service_stopped: same shape — force-stop loop runs, thenreturn not poll_timed_out; caller still raises onFalsebut cleanup runs first.- PR description: the lockstep note no longer implies an unmerged dependency.
Earlier-round issues remain resolved (deps byte-identical to origin/main, "$@" quoting across all 9 scripts, matrix --python pinning, exact dev pins, consistent uv.lock). No outstanding blocking concerns. LGTM.
Eight findings from DhairyaPatel7's review, all verified against the actual repo: - Add `linter_checks` to `all-checks-passed.needs` so the new lint job actually gates branch protection. - Pull the 7 stale `poetry` invocations in scripts/*/README.md into scope as commit 6 (PR #172 missed them). - Fix .sh count: 10 files total (was claiming 8). 9 use uv, extract_private_keys.sh is pure bash. - Add `tox-uv` to dev deps + note that `[cli]` already includes `tox` (so local `uv run tomte tox` works out of the box). - Pin action versions to checkout@v4 + setup-python@v5 (matches rest of workflow; was v6/v6). - Replace "four existing jobs" with the actual six. - Replace "Expected drop: ~344 lines" wording with the correct "Net delta: ~+22 lines" (the plan is additive, nothing drops). - Note the sequencing constraint: commit 4 (CI gate) only after commit 2 (lint green locally), else the branch lands red on its own gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CONTRIBUTING.md trim: The generic "open an issue, branch, commit, push, review" workflow section (~60 lines) is near-identical to the equivalent block in every other Valory repo. Replaced with a short stub linking to the canonical open-autonomy/CONTRIBUTING.md, plus repo-specific notes on where to file contributions (configs/, scripts/, tests/) and the exact local commands for `tomte tox` lint envs and unit tests. The config.json schema reference stays in place per reviewer feedback on PR #175 — it's quickstart-specific and useful where it is. Doc cleanup for the uv migration: PR #172 swapped the root .sh scripts from poetry to uv but missed the same commands inside two sub-READMEs: - scripts/predict_trader/README.md lines 16, 22, 28, 197, 198 - scripts/optimus/README.md lines 18, 19 All 7 instances rewritten from `poetry install` / `poetry run` to the equivalent `uv sync` / `uv run` form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
pyproject.tomlto PEP 621 with[tool.uv].package = false(this repo runs scripts + tests, never publishes a wheel).pytest+pytest-covinto the dev group so the unit-tests job no longer needs the separatepoetry run pip install --upgrade pytest-covshim.poetry run pip install --upgrade packagingoverride — uv's universal resolver already picks uppackaging==26.2(verified inuv.lock). The old--upgradewas working around Poetry's narrower selection from open-aea constraints and is no longer needed.python-tests.yml: replace theInstall Poetry/Configure Poetry/poetry env use/poetry install/poetry runblocks across all 5 jobs (e2e-test-run-service, e2e-test-staking, changes detector, e2e-test-migrate-to-pearl, unit-tests) withastral-sh/setup-uv@v6(pinned to0.11.15) +uv sync --all-groups --frozen+uv run pytest.poetry\.lock$touv\.lock$so the migration job still re-triggers on any dep bump.poetry.lockbefore fresh installs; irrelevant under uv whereuv sync --frozenis hermetic.poetry.lock; commituv.lock.Verified locally
uv lock --checkuv sync --all-groups --frozenoperate, web3, docker, halo, gql, dotenv, tqdm, psutil, autonomy, aeatests/test_scripts/test_pearl_migration.pyare unrelated to this PR —@dataclass(frozen=True) class _Unmigratable(Exception)trips contextlib's__traceback__assignment on macOS Python 3.13+. The same workflow on Linux passes (most recent green:chore/deps-bump-starletteon 2026-05-18 across the same 3.10–3.14 matrix).Note on lockstep
olas-operate-middleware==0.15.21matches the pin already onorigin/main(commit29b7e59), so this PR introduces no drift. Any future middleware bump will need a coordinated bump here.Test plan
setupdiscovers agent configse2e-test-run-servicematrix passese2e-test-stakingpasseschangescorrectly skips/triggerse2e-test-migrate-to-pearlon this PR (touchesuv.lock+.github/workflows/python-tests.yml, so it should run)e2e-test-migrate-to-pearlpassesunit-testsmatrix passes on 3.10 → 3.14 (Linux)