feat: keep instance creation on the allocation notify push - #458
odesenfans wants to merge 4 commits into
Conversation
With the SDK's start_instance now targeting the owner's
/machine/{ref}/start route (a restart, not an allocation), instance
creation calls notify_allocation directly: it is the initial allocation.
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The functional change is correct: I verified that the pinned aleph-sdk-python 2.3.4 ships VmClient.notify_allocation (POST /control/allocation/notify) and that its start_instance currently just delegates to notify_allocation, so instance creation keeps hitting the allocation endpoint today while being future-proof for SDK PR #294, where start_instance becomes a restart against the owner's /control/machine/{ref}/start route. The allocate/start command also correctly keeps start_instance. However, the PR does not update the existing tests: create_mock_vm_client() in tests/unit/test_instance.py does not define a notify_allocation mock, so awaiting the auto-created AsyncMock attribute returns a bare MagicMock and the status/result unpacking in create() raises ValueError, and test_create_instance (line 543) still asserts start_instance was called, which is no longer true. The create-flow unit tests will fail as-is, so the mock and assertion must be updated before merging.
tests/unit/test_instance.py (line 339): The shared mock factory only configures start_instance; since create() now calls notify_allocation, the auto-created AsyncMock attribute returns a bare MagicMock, and status, result = await ... in create() raises "ValueError: not enough values to unpack". Add notify_allocation=AsyncMock(return_value=[200, MagicMock()]) here so the create-flow tests exercise the new call.
tests/unit/test_instance.py (line 543): create() no longer calls start_instance, so this assertion will fail for the superfluid/credit/confidential/GPU cases. It should assert mock_vm_client.notify_allocation.assert_called_once() instead. (The identical assertion at line 864 in test_allocate_instance is fine — the start command still calls start_instance.)
|
Failed to retrieve llama text: POST 502: Bad Gateway |
With create now calling notify_allocation directly, the mock VmClient exposes it and the CRN assertions check it; the allocate command test still asserts start_instance.
mypy runs with install_types = true and tried to pip-install the
missing stub packages at runtime, which fails in CI ("no mypy cache
directory").
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Minimal, correct change. I verified against the pinned aleph-sdk-python 2.3.4 wheel that VmClient.notify_allocation(vm_id) -> Tuple[int, str] exists and that start_instance in that version already delegates to it, so the swap in create() is behavior-neutral today, forward-compatible with the SDK's repointed start_instance, and matches the call-site signature. Tests are updated coherently (fresh mocks per parametrized case, test_allocate_instance correctly still asserts start_instance), and the added comment clearly explains why creation uses the allocation endpoint. Two non-blocking notes: (1) the types-PyYAML/types-ujson additions are unpinned while neighboring stub packages are pinned, and (2) the new start-route/404-fallback behavior only reaches users once the aleph-sdk-python pin is bumped to a release containing SDK PR #294 — worth tracking as a release blocker for this PR.
These dependencies now ship py.typed code using match statements, which mypy (python_version = 3.9) treats as a fatal syntax error — it then fails the install-types step with the misleading 'no mypy cache directory'. Skipping them leaves their types as Any.
foxpatch-aleph
left a comment
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The core change is correct and well-documented: with SDK PR #294 repurposing start_instance for the owner's /start route, switching instance creation to notify_allocation preserves the initial-allocation semantics, the explanatory comment is clear, the instance start command correctly keeps start_instance, and the test was updated to match. The blocker is that pyproject.toml still pins aleph-sdk-python[dns]==2.3.4, which has no notify_allocation method, so the diff as written would raise AttributeError on instance create for PAYG/credit/confidential/GPU instances once released against the declared dependency set. Since the unit test fully mocks VmClient, CI will stay green either way — the pin bump is the only real guard, and it should land with this PR (or immediately at SDK release time, before any aleph-client release). Minor notes: the mypy override patterns filelock./anyio. do not match the top-level modules, the new type stubs are unpinned unlike their neighbors, and the test could also assert start_instance is no longer called during creation.
pyproject.toml (line 32): Blocking as-diffed: the pin is still aleph-sdk-python[dns]==2.3.4, but notify_allocation only exists in the SDK build containing PR #294 — released 2.3.4 will raise AttributeError: 'VmClient' object has no attribute 'notify_allocation' on instance create. Please bump the pin in this PR so the two changes ship atomically. Note that tests/unit/test_instance.py fully mocks VmClient, so CI green does not protect against this — the dependency constraint is the only real guard.
pyproject.toml (line 297): Nit, mypy gotcha: the module pattern "filelock." matches submodules only, not the top-level filelock module (same for "anyio." vs anyio). For anyio the 3.10+ syntax lives in anyio.streams.* submodules so it is covered; if the offending filelock syntax is in init.py the skip will not apply. Using module = ["filelock", "filelock.*"] (and likewise for anyio) makes the skip airtight.
pyproject.toml (line 122): Nit: types-PyYAML and types-ujson are unpinned while the neighboring stubs (types-requests, types-setuptools) are pinned exactly — consider pinning these too for reproducible lint runs. Also worth double-checking whether these stubs are actually needed now that filelock/anyio are skipped (mypy's install_types = true would pull them in anyway), or whether they are leftovers from an earlier attempt to fix the lint.
tests/unit/test_instance.py (line 544): Nice-to-have: alongside notify_allocation.assert_called_once(), also add mock_vm_client.start_instance.assert_not_called() inside this branch, so a future regression back to start_instance in the create flow is caught by CI rather than only by the dependency pin failing at runtime.
src/aleph_client/commands/instance/__init__.py (line 1218): Minor doc nit: with start_instance now hitting the owner's authenticated /start route (plus 404 fallback per SDK #294), the docstring "Notify a CRN to start an instance" and the success message "VM allocated on CRN" no longer quite describe what happens — worth a small wording refresh once the SDK change lands.
2 participants
Summary
Companion to aleph-sdk-python #294: with the SDK's
start_instancenow targeting the owner's/control/machine/{ref}/startroute (a restart, not an allocation), instance creation callsnotify_allocationdirectly — creation is the initial allocation, not a restart.The
instance startcommand keeps callingstart_instanceand gains the authenticated route + 404 fallback through the SDK.