enhance instance metadata handling in API and placement logic

[jayy-77]

Motivation

#1426

Changes

Why It Works

Test Plan

Manual Testing

Automated Testing

[AlexCheema]

Code Review

Thanks for tackling issue #1426! The core fix (deriving instance_meta from the actual instance type after placement) is correct.

However, PR #1509 already addresses this same bug with a simpler approach. Here's a comparison:

Overlap with #1509

	This PR (#1476)	PR #1509
Files changed	4	1 (api.py only)
Lines added	~50	~12
CI	No checks	6/6 PASS
Approach	Helper function + model validator	Inline isinstance check

Issues

1. Model validator on PlacementPreview is risky — The validate_instance_meta_matches_instance validator raises ValueError when instance_meta doesn't match the instance type. But this is exactly the bug condition that existed before the fix. If the validator is somehow applied without the api.py fix (e.g., partial merge, or another code path constructing PlacementPreview), it would crash the endpoint entirely rather than returning a mismatched response. Validators should enforce invariants, but this one validates against an internal bug that should just be fixed at the source.

2. get_instance_meta() unreachable branch — The else: raise ValueError branch is unreachable since Instance = MlxRingInstance | MlxJacclInstance is exhaustive. basedpyright would likely flag this.

3. Over-engineered — A helper function and model validator for a two-variant enum with one call site adds complexity without proportionate benefit. PR Fix instance type mismatch in /instance/previews endpoint #1509 achieves the same result in 3 inline lines.

4. Helpful comment — The explanatory comment added to placement.py about why single-node forces MlxRing is genuinely useful. Worth keeping as a separate small PR.

Root cause note

Both PRs work around the real issue: place_instance() mutates command.instance_meta as a side effect (line 146). The command isn't frozen (TaggedModel → CamelCaseModel lacks frozen=True), so this mutation leaks back to the caller. A cleaner long-term fix would be to avoid mutating the command and instead determine instance_meta locally within place_instance().

Recommendation

This PR is superseded by #1509. I'd recommend closing in favor of #1509.

[AlexCheema]

Code Review — PR #1476: enhance instance metadata handling in API and placement logic

CI: No checks (fork PR — CI doesn't run automatically)

Overview

+61/-6 across 4 files. Fixes a bug where get_placement_previews reports the requested instance_meta instead of the actual one after placement overrides it (e.g., single-node always forces MlxRing, but the preview still says MlxJaccl if that's what was requested).

Changes:

1. instances.py — new get_instance_meta() helper mapping Instance type → InstanceMeta enum
2. api.py — uses get_instance_meta(instance) to derive actual instance_meta in get_placement_previews
3. api.py — adds model_validator on PlacementPreview enforcing instance/meta consistency
4. placement.py — adds explanatory comment about single-node MlxRing override

Assessment

The core fix is correct — deriving instance_meta from the actual instance type after placement resolves the mismatch. The root cause is that place_instance() mutates command.instance_meta as a side effect (line ~135 of placement.py), and the caller was using the pre-mutation value.

Issues

1. Superseded by PR #1509 — PR #1509 fixes the same bug with a simpler approach: 1 file, +17/-4, inline isinstance check, CI passing. This PR adds a helper function, a model_validator, and touches 4 files for what is fundamentally a 3-line fix.

2. Model validator is risky — validate_instance_meta_matches_instance raises ValueError on mismatch, which Pydantic converts to ValidationError. If triggered during API handling (e.g., from another code path constructing PlacementPreview without the fix), the endpoint returns a 500 error instead of a degraded-but-functional response. Validators should enforce data contracts, not catch internal bugs that should be fixed at the source.

3. get_instance_meta() unreachable else branch — Since Instance = MlxRingInstance | MlxJacclInstance is exhaustive, the else: raise ValueError(...) is dead code. basedpyright may flag this.

What's good

The explanatory comment in placement.py about why single-node forces MlxRing is genuinely useful. Worth preserving (e.g., cherry-pick into #1509).

Verdict

Superseded by #1509, which achieves the same fix with less complexity and passing CI. Recommend closing in favor of #1509.

[AlexCheema]

Code Review: PR #1476 — enhance instance metadata handling in API and placement logic

Summary

Same fix as PR #1509 — addresses the instance_meta mismatch in the previews endpoint. Adds get_instance_meta() helper and a model_validator on PlacementPreview.

Comparison with PR #1509

This appears to be the same PR as #1509 — identical changes:

• get_instance_meta() helper in instances.py
• validate_instance_meta_matches_instance model validator on PlacementPreview
• actual_instance_meta = get_instance_meta(instance) in api.py
• Comment on the single-node override in placement.py

This is a duplicate of #1509. One of these should be closed.

Verdict

Duplicate of PR #1509. See review on #1509 for detailed feedback.