Skip to content

fix: treat an empty machine.ami_id as the fallback, not a missing value - #490

Merged
7174Andy merged 1 commit into
mainfrom
andrew/doctor-empty-ami-fallback
Sep 1, 2026
Merged

7174Andy merged 1 commit into
mainfrom
andrew/doctor-empty-ami-fallback

Conversation

@7174Andy

@7174Andy 7174Andy commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

lablink doctor fails the configuration #489 recommends.

#489 makes an empty machine.ami_id resolve AWS's Deep Learning Base AMI for the
deployment's region (data.aws_ami.default_client), which is how a lab runs somewhere
LabLink publishes no image of its own. But _check_ami, merged in #484, treats empty as a
missing value:

Client AMI  FAIL  No machine.ami_id set. For us-west-2, use ami-0601752c11b394251

I wrote both, and they disagreed. Empty is not a gap — it's a request.

What changes

Empty now resolves the same image the deployment would, picks the newest to match
most_recent, and reports it:

Client AMI  PASS  empty → ami-0d7a7be9ba394534e (Deep Learning Base OSS Nvidia Driver GPU AMI (Ubuntu 24.04) 20260828) in eu-west-1
  • fail only where AWS publishes no such image in that region to fall back to,
    suggesting the region's published LabLink image when there is one.
  • warn, not fail, when the lookup itself cannot run — no credentials, no
    ec2:DescribeImages, network trouble. Unverified is not the same as wrong.

The name filter is duplicated from the client terraform, with a comment saying so. That
duplication is deliberate: verifying a different image than the deployment resolves would
be worse than not checking at all, so the two have to be kept in step, and the comment is
where the next person finds that out. (Ubuntu 24.04) stays pinned because 26.04 is
published on the same dates — a new 26.04 appeared overnight while this was being written.

Second, smaller fix: a wrong-region AMI in a region with no published LabLink image now
suggests clearing the field, which is the one-step fix, rather than only suggesting an
aws ec2 copy-image job. That is the case a real operator hits:

Client AMI  FAIL  ami-0601752c11b394251 does not exist in eu-west-1 — clear machine.ami_id
                  to resolve a Deep Learning Base AMI for eu-west-1 automatically, or copy
                  an image from us-west-2, us-east-1, us-east-2 into eu-west-1

Testing

packages/cli: 822 passed, 1 deselected
ruff check packages/cli: All checks passed!

Four new cases: empty resolves the newest match (asserting the older candidate is not
chosen, so the sort is actually exercised), empty fails where nothing is published, empty
warns when unverifiable, and the wrong-region message names the one-step fix.

Related

This check and #489 disagreed about what empty means. #489 makes an empty
machine.ami_id resolve AWS's Deep Learning Base AMI for the deployment's region
(data.aws_ami.default_client), which is the recommended way to run somewhere
LabLink publishes no image. doctor still reported it as 'No machine.ami_id set'
and failed — so the recommended configuration failed its own preflight.

Empty now resolves the same image the deployment would, picking the newest to
match most_recent, and reports which one:

    Client AMI  PASS  empty → ami-0d7a7be9ba394534e (Deep Learning Base ...) in eu-west-1

It fails only where AWS publishes no such image to fall back to, and warns rather
than failing when the lookup itself cannot run. The filter is duplicated from the
terraform with a comment saying so, because verifying a different image than the
deployment uses would be worse than not checking.

Also: a wrong-region AMI in a region with no published LabLink image now suggests
clearing the field, which is the one-step fix, instead of only suggesting a
copy-image job.

Depends on #489 for the terraform side; merge this after it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@7174Andy
7174Andy merged commit dbf6e5a into main Sep 1, 2026
6 checks passed
@7174Andy
7174Andy deleted the andrew/doctor-empty-ami-fallback branch September 1, 2026 19:34
7174Andy added a commit that referenced this pull request Sep 8, 2026
Release prep. publish-pip.yml's version guardrail rejects a tag whose
version does not match pyproject.toml, so the bumps land on main before
the release tags are cut.

Allocator and client stay in lockstep at 0.4.0 as they have since 0.1.0;
the CLI is versioned independently and goes to 0.3.0.

The CLI's allocator pin is raised to >=0.4.0 this time: the CLI
re-exports MachineConfig, whose ami_id default became empty (= resolve
the per-region Deep Learning Base AMI, #489) in allocator 0.4.0. An
older allocator would silently reintroduce the stale hardcoded
us-west-2 AMI default that doctor's #490 fallback logic assumes gone.

CHANGELOG (CLI): new 0.3.0 section (#484, #485, #490, #491, #498), and
a backfilled 0.2.0 section — #481 tagged 0.2.0 without adding one
(#467, #472, #474, #479).

Also: README Docker <version> example moved to 0.4.0.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7174Andy added a commit that referenced this pull request Sep 8, 2026
Release prep. publish-pip.yml's version guardrail rejects a tag whose
version does not match pyproject.toml, so the bumps land on main before
the release tags are cut.

Allocator and client stay in lockstep at 0.4.0 as they have since 0.1.0;
the CLI is versioned independently and goes to 0.3.0.

The CLI's allocator pin is raised to >=0.4.0 this time: the CLI
re-exports MachineConfig, whose ami_id default became empty (= resolve
the per-region Deep Learning Base AMI, #489) in allocator 0.4.0. An
older allocator would silently reintroduce the stale hardcoded
us-west-2 AMI default that doctor's #490 fallback logic assumes gone.

CHANGELOG (CLI): new 0.3.0 section (#484, #485, #490, #491, #498), and
a backfilled 0.2.0 section — #481 tagged 0.2.0 without adding one
(#467, #472, #474, #479).

Also: README Docker <version> example moved to 0.4.0.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant