From 259c527cfb907e121649390bd6c30535f47471b1 Mon Sep 17 00:00:00 2001 From: runpod-Henrik Date: Wed, 26 Aug 2026 16:46:02 -0700 Subject: [PATCH 1/3] fix(ci): run each matrix leg on its own Python version The quality-gates matrix declared 3.10/3.11/3.12/3.13 but every leg ran Python 3.11. `make dev` runs a bare `uv sync`, and uv resolves its interpreter from `.python-version` (pinned to 3.11) rather than from the one actions/setup-python just installed. Confirmed across four CI runs on three different branches, the oldest from 2026-08-10: every leg reports `Using CPython 3.11.x` and builds `.venv/lib/python3.11`. UV_PYTHON takes precedence over the pin file. Set at job level rather than on the install step because `make ci-quality-github` runs `uv run pytest`, which resolves the interpreter again. uv.lock is re-locked in the same commit because it was stale in two ways and 3.13 could not resolve without it: * `requires-python = ">=3.10, <3.13"` while pyproject.toml says `>=3.10,<3.14`, so the lock never covered 3.13 at all * `tomlkit>=0.13.0` is declared in pyproject.toml but was missing from the runpod-flash dependency and requires-dist lists `uv lock --check` fails against the previous lock and passes against this one, for each of 3.10/3.11/3.12/3.13. CI did not catch the drift because pre-check uses `--frozen` (which does not verify freshness) and `make dev` uses plain `uv sync`, which silently re-resolves on the runner. Verified: the full suite on a real 3.13 interpreter gives 2680 passed, with no 3.13-specific failures. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 11 +++++++++++ uv.lock | 6 ++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 522ff5ac..61a6aa6f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -231,6 +231,17 @@ jobs: python-version: ['3.10', '3.11', '3.12', '3.13'] timeout-minutes: 15 + # Without this the matrix is decorative. `make dev` runs a bare `uv sync`, + # and uv resolves its interpreter from .python-version (pinned to 3.11) + # rather than from whatever actions/setup-python just installed -- so every + # leg built a 3.11 venv and all four ran the same interpreter. UV_PYTHON + # takes precedence over the pin file, which restores real version coverage. + # + # Set at job level, not on the install step: `make ci-quality-github` runs + # `uv run pytest`, which resolves the interpreter again. + env: + UV_PYTHON: ${{ matrix.python-version }} + steps: - name: Checkout code uses: actions/checkout@v4 diff --git a/uv.lock b/uv.lock index a2e48d3f..4db98f5a 100644 --- a/uv.lock +++ b/uv.lock @@ -1,6 +1,6 @@ version = 1 revision = 3 -requires-python = ">=3.10, <3.13" +requires-python = ">=3.10, <3.14" resolution-markers = [ "python_full_version >= '3.13'", "python_full_version < '3.13'", @@ -824,7 +824,7 @@ name = "exceptiongroup" version = "1.3.1" source = { registry = "https://pypi.org/simple" } dependencies = [ - { name = "typing-extensions" }, + { name = "typing-extensions", marker = "python_full_version < '3.13'" }, ] sdist = { url = "https://files.pythonhosted.org/packages/50/79/66800aadf48771f6b62f7eb014e352e5d06856655206165d775e675a02c9/exceptiongroup-1.3.1.tar.gz", hash = "sha256:8b412432c6055b0b7d14c310000ae93352ed6754f70fa8f7c34141f91c4e3219", size = 30371, upload-time = "2025-11-21T23:01:54.787Z" } wheels = [ @@ -2669,6 +2669,7 @@ dependencies = [ { name = "rich" }, { name = "runpod" }, { name = "tomli", marker = "python_full_version < '3.11'" }, + { name = "tomlkit" }, { name = "typer" }, ] @@ -2698,6 +2699,7 @@ requires-dist = [ { name = "rich", specifier = ">=14.0.0" }, { name = "runpod", git = "https://github.com/runpod/runpod-python?rev=main" }, { name = "tomli", marker = "python_full_version < '3.11'", specifier = ">=2.0.0" }, + { name = "tomlkit", specifier = ">=0.13.0" }, { name = "typer", specifier = ">=0.12.0" }, ] From 95d3b2df4ea8d1465cdeb90d599270c39e984ff6 Mon Sep 17 00:00:00 2001 From: runpod-Henrik Date: Wed, 26 Aug 2026 16:46:17 -0700 Subject: [PATCH 2/3] fix(tests): stop test_deploy_all_background leaking a thread test_deploy_all_background was the source of a flaky failure that hit an unrelated test, most often FAILED tests/unit/test_regressions.py::TestREG008NonInteractiveEnvDelete ::test_undeploy_resource_force_remove_no_tty _pickle.PicklingError: args[0] from __newobj__ args has the wrong class deploy_all_background() starts a daemon thread and returns immediately, and the test never joined it. The `patch.object(..., "get_or_deploy_resource")` context was therefore lifted while the thread was still starting, so the thread ran against the unpatched manager and registered the fixture's MagicMock(spec=ServerlessResource) objects into the ResourceManager singleton -- during whichever unrelated test happened to be running at that moment. ResourceManager._save_resources() cloudpickles its entire state on every registration, and a MagicMock cannot be pickled: `obj.__class__` is the spec'd class while `type(obj)` is MagicMock, which is exactly the mismatch pickle.save_reduce rejects. The mock arrived as a dict *key*, via _migrate_to_name_based_keys() calling `resource.get_resource_key()` on it, so the offending key rendered as ``. Because it depended on thread scheduling and on xdist's dynamic worker assignment, the victim, the worker and the matrix leg all varied per run: the same failure appears on three unrelated branches, on legs 3.10, 3.12 and 3.13 and on workers gw0, gw2 and gw3. Forcing everything into one process reproduced it every time; `-n 4` reproduced it in 0 of 6 runs. threading.Thread is now stubbed, which keeps what this test actually asserts -- the call is non-blocking and spawns a daemon thread -- and lets nothing escape. The test previously asserted nothing at all; its own comment said "not much we can test here without waiting for thread". Verified: full suite single-process, 2680 passed, 0 PicklingError (the same run reproduced the failure before this change). The two remaining local failures are TestVersionFlag, which is colour-dependent and passes both in CI and locally under NO_COLOR=1. Two deeper faults are left alone here, as they are wider changes: ResourceManager._resources / _resource_configs are class variables that instance mutation writes through, and conftest's worker_flash_dir is scope="session", so every test in a worker shares one resources.pkl. Co-Authored-By: Claude Opus 5 --- tests/unit/test_deployment.py | 37 +++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/tests/unit/test_deployment.py b/tests/unit/test_deployment.py index 081ca0b9..15c479c6 100644 --- a/tests/unit/test_deployment.py +++ b/tests/unit/test_deployment.py @@ -195,19 +195,44 @@ async def slow_deploy(resource): assert results[0].duration >= 0.09 def test_deploy_all_background(self, mock_resources): - """Test background deployment doesn't block.""" + """Test background deployment starts a daemon thread without blocking.""" orchestrator = DeploymentOrchestrator() - with patch.object( - orchestrator.manager, "get_or_deploy_resource", new_callable=AsyncMock - ) as mock_deploy: + # threading.Thread is stubbed rather than left to run for real. + # + # deploy_all_background() starts a daemon thread and returns + # immediately, so a real thread outlives this test: the patch below + # would be lifted while the thread was still starting, and the thread + # would then call the *unpatched* manager and register these + # MagicMock(spec=ServerlessResource) objects into the ResourceManager + # singleton -- during whichever unrelated test happened to be running + # at that moment. ResourceManager._save_resources() cloudpickles its + # whole state on every registration, and a MagicMock cannot be pickled, + # so an arbitrary later test died with + # + # _pickle.PicklingError: args[0] from __newobj__ args has the wrong class + # + # Which test got hit depended on thread scheduling and on xdist's + # worker assignment, which is what made it flaky rather than simply + # broken. Stubbing the thread keeps the assertion this test actually + # makes -- that the call is non-blocking and spawns a daemon thread -- + # and lets nothing escape the test. + with ( + patch("runpod_flash.core.deployment.threading.Thread") as mock_thread_cls, + patch.object( + orchestrator.manager, "get_or_deploy_resource", new_callable=AsyncMock + ) as mock_deploy, + ): mock_deploy.side_effect = mock_resources # Should not block orchestrator.deploy_all_background(mock_resources) - # Background thread should be started - # (not much we can test here without waiting for thread) + # A daemon thread was started, and nothing ran inline. + mock_thread_cls.assert_called_once() + assert mock_thread_cls.call_args.kwargs["daemon"] is True + mock_thread_cls.return_value.start.assert_called_once() + mock_deploy.assert_not_called() def test_deploy_all_background_empty_list(self): """Test background deployment with empty list.""" From 0625cc9386a999ead1729f546973a93cc32f929a Mon Sep 17 00:00:00 2001 From: runpod-Henrik Date: Wed, 26 Aug 2026 16:58:52 -0700 Subject: [PATCH 3/3] fix(tests): repair three tests that only ever ran on 3.11 Making the version matrix real (previous commit) exposed three failures on Python 3.10 that had been masked for months. All three are test artifacts, not product bugs, and all three reproduce on a real 3.10 interpreter independently of this branch. tests/unit/cli/commands/test_run_server_helpers.py call_with_body -> _map_body_to_params calls inspect.signature(func), and signature() of a Mock is version-dependent: on 3.10 it raises `TypeError: 'Mock' object is not subscriptable` (Mock auto-creates a __signature__ child which inspect then tries to use), while on 3.11+ it reports (*args, **kwargs). Passing spec= does not help -- verified; the failure is in signature() itself. call_with_body catches Exception and converts it into a 500 JSONResponse, so the visible failure was the uninformative `assert == {'ok': True}`, with the real TypeError readable only inside the response body. The mock is now wrapped in a real `async def (*args, **kwargs)` -- the same signature 3.11+ inferred from the Mock, so both branches of _map_body_to_params behave exactly as before -- and delegating to the mock keeps `assert_called_once_with()`. tests/unit/core/resources/test_resource_manager_extended.py test_loads_legacy_dict_format cloudpickle.dump()s a MagicMock, which 3.10 rejects with "Could not pickle object as excessively deep recursion required". Replaced with a module-level _LegacyResource: only .config_hash is read (via _refresh_config_hashes), and the *absence* of get_resource_key is what keeps the legacy key un-migrated, which is what this test asserts. Module-level rather than defined in the test body, since cloudpickle serialises a function-local class by value. Verified on 3.10, 3.11 and 3.13: 40 passed in both affected files on each, where 3.10 previously failed all three tests. ruff format and check clean. Worth following up separately: the blanket `except Exception -> JSONResponse(500)` in call_with_body turns genuine errors into silent 500s, which is what made this take three steps to diagnose. Co-Authored-By: Claude Opus 5 --- .../cli/commands/test_run_server_helpers.py | 32 +++++++++++++++++-- .../test_resource_manager_extended.py | 19 ++++++++++- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/tests/unit/cli/commands/test_run_server_helpers.py b/tests/unit/cli/commands/test_run_server_helpers.py index 60ae9a14..80a3feb6 100644 --- a/tests/unit/cli/commands/test_run_server_helpers.py +++ b/tests/unit/cli/commands/test_run_server_helpers.py @@ -336,12 +336,34 @@ async def test_allows_populated_fields(self): assert result == {"echo": "hello"} func.assert_called_once_with(msg="hello") + # These two wrap the mock in a real async def instead of passing the mock + # straight to call_with_body. + # + # call_with_body -> _map_body_to_params calls inspect.signature(func), and + # signature() of a Mock is version-dependent: on 3.10 it raises + # `TypeError: 'Mock' object is not subscriptable` (Mock auto-creates a + # __signature__ child, which inspect then tries to use), while on 3.11+ it + # reports (*args, **kwargs). Passing spec= does not help -- the failure is + # in signature(), not in spec resolution. + # + # call_with_body catches Exception and converts it into a 500 JSONResponse, + # so on 3.10 these failed as `assert == {'ok': True}`, + # with the real TypeError visible only inside the response body. + # + # The wrapper declares (*args, **kwargs), the same signature 3.11+ inferred + # from the Mock, so both branches of _map_body_to_params behave exactly as + # before -- and delegating to the mock keeps the call assertions. + @pytest.mark.asyncio async def test_allows_plain_dict_body(self): """Non-empty plain dict passes through unchanged (no model_fields_set).""" body = {"key": "value"} - func = AsyncMock(return_value={"ok": True}) + mock = AsyncMock(return_value={"ok": True}) + + async def func(*args, **kwargs): + return await mock(*args, **kwargs) + result = await call_with_body(func, body) assert result == {"ok": True} @@ -354,8 +376,12 @@ async def test_allows_empty_plain_dict_body(self): """ body = {} - func = AsyncMock(return_value={"ok": True}) + mock = AsyncMock(return_value={"ok": True}) + + async def func(*args, **kwargs): + return await mock(*args, **kwargs) + result = await call_with_body(func, body) assert result == {"ok": True} - func.assert_called_once_with() + mock.assert_called_once_with() diff --git a/tests/unit/core/resources/test_resource_manager_extended.py b/tests/unit/core/resources/test_resource_manager_extended.py index 06f2ac1e..fe24a8f8 100644 --- a/tests/unit/core/resources/test_resource_manager_extended.py +++ b/tests/unit/core/resources/test_resource_manager_extended.py @@ -9,6 +9,17 @@ ) +class _LegacyResource: + """Minimal picklable stand-in for a resource in the pre-1.12 state file. + + Module-level, not defined inside a test: cloudpickle serialises a class + defined in a function body by value, which is the thing being avoided here. + """ + + def __init__(self, config_hash: str): + self.config_hash = config_hash + + @pytest.fixture(autouse=True) def reset_manager(isolate_resource_state_file, reset_singletons): """Reset singleton and state file between tests.""" @@ -56,7 +67,13 @@ def test_loads_legacy_dict_format(self, tmp_path): state_file = tmp_path / ".runpod" / "resources.pkl" state_file.parent.mkdir(parents=True) - resources = {"key1": MagicMock(config_hash="hash1")} + # A plain object rather than MagicMock: whether cloudpickle can pickle a + # Mock is version-dependent, and on 3.10 this failed with + # "Could not pickle object as excessively deep recursion required". + # Only .config_hash is read here (via _refresh_config_hashes), and the + # absence of get_resource_key is what keeps the legacy key un-migrated, + # which is exactly what this test is asserting. + resources = {"key1": _LegacyResource("hash1")} with open(state_file, "wb") as f: cloudpickle.dump(resources, f)