feat(serve): list/filter jobs, deploy from recommendation row, compare benchmarks, DataFrame views

[ZealSV]

Adds inference-recommender usability features to sagemaker.serve:

• list_benchmarks(endpoint=...) / list_recommendations(model=..., model_package=...): the ListAI* APIs cannot filter by endpoint/model server-side (those fields are on Describe, not the list summary), so filtering is client-side (list -> describe -> match) bounded by max_results. Chosen because it is the only option the boto APIs allow, and a bounded describe fan-out keeps a broad list from being unbounded.

• ModelBuilder.deploy(recommendation=): accept a recommendation row (mb.recommendations.best or [i]) and resolve it to the existing spec-name/index selection. Chosen as an additive, back-compat param so callers deploy the best row without hand-copying a magic index; existing index/spec_name kwargs still work.

• compare_benchmarks(*results): N-way comparison, first result = baseline, one row per metric x one column per run + a signed delta% column oriented so + is always better. Chosen N-way (not strictly 2-way) because it is barely more code and the delta vs a baseline is the whole point of a comparison utility.

• to_dataframe() on every tabular surface (customer-requested DataFrame view): BenchmarkMetrics, BenchmarkResult, BenchmarkComparison, and both recommendation views (_RecommendationView, _RecommendationsView). Each returns a pandas DataFrame mirroring its printed table, but carrying the extra stats (min/max/p95/stddev) the width-limited text tables drop, and keeping numeric values native (deltas numeric, NaN where undefined) instead of preformatted strings. pandas is imported lazily via _require_pandas() so result.py stays dependency-light (its printed tables are stdlib-only); it is present transitively via sagemaker-core in any real install. str and to_dataframe() share ordering/row-building helpers (_ordered_metric_pairs, _row_records, _delta_value) so the printed table and the frame never drift. BenchmarkResult.to_dataframe() raises on a search/sweep result (no single profile).

Unit: 45 new tests (11 listing, 5 deploy-row, 12 compare, 17 to_dataframe); full recommender suite 199 pass. Integ: test_ai_inference_recommender_enhancements_integration.py chains rec -> deploy(rows.best) InService -> 2 benchmarks -> compare_benchmarks; verified live (1 passed, 70 min) plus a no-GPU listing-plumbing test.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[Lokiiiiii]

F1 (blocker): this discards the row's index, so deploy(recommendation=row) can provision a different row than the one passed.

The row's _index is read only in the else branch below — when the row has no spec name. Whenever it does have one, recommendation_index keeps its signature default of 0 and the row's true position is never forwarded. _deploy_recommendation then resolves a spec name by taking matches[0] (line 5215).

If two rows share an inference_specification_name, the wrong one is deployed. Reproduced with two rows differing in ModelPackage and instance type:

deploy(recommendation=view[1])    # caller wants pkg/2, ml.p4d.24xlarge x8
  WARNING recommendation_spec_name='dup-spec' matched 2 recommendations; deploying the first.
  SELECTED -> model-package/g/1   # row 0: ml.g5.2xlarge x1

deploy(recommendation_index=1)    # same intent, index path
  SELECTED -> model-package/g/2   # correct

rec drives ModelPackage, container, env vars, instance type, instance count and copy count (lines 5228-5363), so all of those come from the wrong row. The endpoint reaches InService on hardware the caller did not choose.

Duplicate spec names are not hypothetical — AIRecommendationModelDetails carries instance_details as a list of instance configurations per spec, so one spec fanning out to N rows is the normal shape. This PR's own TestDeployRecommendationSpecNameMultiMatch asserts the duplicate case, and TestDeployRecommendationRowObject._make_builder uses distinct spec names "A"/"B" — so both halves are tested and never composed, which is why all 199 tests pass.

Worth noting the warning at line 5209 advises "Use recommendation_index to pick a specific one", but recommendation= and recommendation_index= are mutually exclusive (line 5533), so the remedy it suggests is unreachable from the path that emits it.

Suggested direction: forward recommendation_index = getattr(recommendation, "_index", 0) unconditionally and let the index win when a row object was passed, using the spec name only as a fallback. The caller handed you an unambiguous identifier; keeping it makes the feature exact.

[Lokiiiiii]

F2 (major): this is a second Describe per candidate — the iterator already refreshed the object.

sagemaker-core's ResourceIterator.__next__ refreshes every object it yields, gated only on hasattr:

# sagemaker/core/utils/utils.py:464-465
if hasattr(resource_object, "refresh"):
    resource_object.refresh()

AIBenchmarkJob.refresh and AIRecommendationJob.refresh both exist and neither caches, so objects arriving here are already hydrated. Measured with the real get_all and only the boto client patched, attributing each call by stack frame:

5 candidates -> 10 DescribeAIBenchmarkJob calls
by origin: {'ITERATOR.__next__': 5, '_collect': 5}

This also means the comment above ("The list summary lacks the nested field") no longer describes reality, and the predicate is None path still pays a full Describe per job inside the iterator — so the "hydrate only when filtering" optimisation the module docstring describes does not hold either.

Combined with F3 this doubles an already-unbounded fan-out. Suggested direction: drop this call and note in the docstring that hydration is the iterator's job, or if you want control over it, describe explicitly through the session's client instead (which would also address F5).

[Lokiiiiii]

F3 (major): max_results bounds the number of matches, not the Describe fan-out — contrary to three docstrings.

This breaks on len(matches), so when the predicate matches rarely the loop keeps describing. Measured against _collect directly:

stream of 500 candidates, max_results=2, only the last matches
  matches returned : 1
  refresh() calls  : 500

The module docstring states "max_results bounds that Describe fan-out" (line 22), and both public functions repeat it as "(and, when endpoint is set, on how many are described)" (lines 132-134, 168-170). None of that holds.

scanned (line 90, incremented at line 111) is assigned and never read anywhere — which reads like the scan budget this was meant to have, left unwired. It also sits after the break, so it would under-count by one even if something did read it.

Practical impact: list_benchmarks(endpoint="typo-in-name") on an account with 5k jobs issues 5k Describes with F2 doubling it to ~10k — minutes of latency and near-certain throttling, for a call that returns [].

Suggested direction: bound the scan as well as the results — if scanned >= max_scan: break with max_scan either derived from max_results or its own parameter — and log the scanned count so the cost is visible. If instead the intent is that max_results caps results only, the three docstrings need to say so, and callers need some other way to bound the fan-out.

A unit test asserting refresh.call_count for a non-matching filter would pin this; the current test_max_results_caps_output uses filter-free jobs, so it exercises the one path where the cap and the fan-out coincide.

[Lokiiiiii]

F4 (major): the first Describe happens on this line, outside the try below — so one inaccessible job aborts the whole listing.

Because ResourceIterator.__next__ refreshes each object as it yields it (sagemaker/core/utils/utils.py:465), the initial Describe for every job executes at this for statement, not at the guarded job.refresh() on line 98. Verified against the real iterator:

RAISED OUT of list_benchmarks: AccessDeniedException ...
  File ".../listing.py", line 91, in _collect      <- this line
  File ".../sagemaker/core/utils/utils.py", line 465, in __next__
  File ".../sagemaker/core/resources.py", line 383, in refresh

The remaining jobs are never examined. Control comparison with a plain iter(list) that does not refresh on yield: the bad job is skipped and the others are returned, i.e. the try/except works — but only for iterators that do not hydrate, which the real one does.

So the except Exception on line 99 is structurally unreachable for the failures most likely to occur, while reading as though it makes this function resilient to them. One job the caller lacks DescribeAIBenchmarkJob on anywhere in the account makes list_benchmarks() raise instead of returning the other N−1.

Suggested direction: wrap the iteration itself, e.g. drive it with next() inside the try, so a per-job Describe failure is handled in one place regardless of which layer issued the call.

[Lokiiiiii]

F5 (major): passing the type this parameter is annotated with raises a ValidationError.

The annotation resolves to sagemaker.core.helper.session_helper.Session (imported line 28), but get_all is wrapped in pydantic validate_call and requires boto3.session.Session. These are unrelated types — isinstance(Session(), boto3.session.Session) is False. Verified:

>>> list_benchmarks(sagemaker_session=sagemaker.core.helper.session_helper.Session())
ValidationError: 1 validation error for AIBenchmarkJob.get_all
session
  Input should be an instance of Session [type=is_instance_of,
    input_value=<sagemaker.core.helper.session_helper.Session object ...>]

>>> list_benchmarks(sagemaker_session=boto3.session.Session(region_name="us-west-2"))
   accepted

The docstring says only "Optional session; a default is created if omitted", so a reader has nothing to warn them off the annotated type — and sagemaker.Session() is the idiomatic thing to reach for. The default None path works, so this only bites callers who use the parameter at all.

Same on list_recommendations (line 162).

Suggested direction: annotate Optional[boto3.session.Session] and say so in the docstring, or accept both and unwrap a sagemaker Session to its boto_session before handing it to get_all.

Related caveat worth documenting either way: SageMakerClient is a SingletonMeta keyed only on the class, so if the singleton was already built earlier in the process, the session passed here is ignored and the call targets the earlier one.

[Lokiiiiii]

F10 (minor): this is the one column that carries a display sentinel into the DataFrame.

_safe_str maps None/"" to "-", and _short_container_tag passes "-" straight through, so the record gets the string "-" where every sibling column keeps None. That contradicts this method's own docstring two lines up ("numbers stay numbers, None stays None"). Verified with image_uri=None:

container repr    : ['-']       container isna    : [False]
spec_name repr    : [None]      instance_type isna: [True]

Impact is on the frame, not the table: df[df.container.notna()], groupby("container") and value_counts() all treat "no container" as a container named -.

Suggested direction: pass the raw value — _short_container_tag(getattr(dc, "image_uri", None) if dc else None) — and let __str__'s existing "-" if value in (None, "") branch handle display. That branch already exists and already covers this.

[Lokiiiiii]

F11 (minor): this comment is describing why the test cannot exercise the line it is about.

Because every stand-in is constructed as BenchmarkJob/RecommendationJob already, job.__class__ = subclass in _collect is a genuine no-op in all 11 listing tests — deleting that line keeps the suite green. Yet it is the reason _collect takes a subclass parameter at all, and the reason the Returns: docstrings can promise "each with show_result".

The behaviour does work: feeding base AIBenchmarkJob instances through list_benchmarks does yield BenchmarkJob objects with show_result. Nothing guards it.

Suggested direction: build the stand-ins as base AIBenchmarkJob / AIRecommendationJob and assert isinstance(out[0], BenchmarkJob). That gives the retype its first real assertion and matches what get_all actually yields, since it hardcodes resource_cls=AIBenchmarkJob rather than cls.

While you're here: the autouse refresh stub on lines 45-51 uses a lambda. A MagicMock would cost nothing and would make the Describe call count assertable, which is what F3 needs.

[Lokiiiiii]

F12 (minor): this is the only integ test that runs on PR checks, and it triggers the F3 fan-out against a live account.

The slow_test / gpu_intensive markers on line 89-90 belong to the next function, so this one collects into any integ run. sagemaker-serve/tox.ini:66 says gpu_intensive "runs on scheduled CI, not PR checks" — so the e2e test that actually covers deploy-from-row and compare_benchmarks is excluded, while this one is not.

Lines 78-84 then call list_benchmarks(endpoint="no-such-endpoint-<uuid>") and list_recommendations(model="s3://no-such-bucket-<uuid>/model/"). Both are guaranteed zero-match by construction, which per F3 means a full-account Describe sweep of both job types — doubled by F2. Under pytest -n auto that is a reliable way to throttle the whole suite for everyone sharing the account.

The intent is good and the test is cheap in principle; it is the interaction with F3 that makes it expensive. Fixing F3 largely fixes this too.

Separately, the assertion style in the e2e test is worth a look: for job in found: assert ... passes vacuously when found is empty, so a filter regressing to always-False would not be caught. Asserting found is non-empty first — or accepting the raciness and asserting on a job you created — would make it load-bearing.

[Lokiiiiii]

F13 (minor): a permissions or throttling failure silently shortens the result list, at DEBUG.

except Exception plus logger.debug plus continue means a job the caller cannot describe is dropped with no visible trace at default log level. If the role lacks sagemaker:DescribeAIBenchmarkJob, or the API throttles under the F3 fan-out, every candidate is skipped and list_benchmarks(endpoint=...) returns []. The customer concludes no benchmark ever targeted their endpoint; the on-call has nothing to correlate. Under throttling it presents as "the filter intermittently finds nothing".

This is the same hazard the max_results log on line 105 was added to prevent — the docstring says it "logs when the scan is truncated so a silent cap never reads as 'all matches'" — and the two cases deserve the same treatment.

Suggested direction: logger.warning, and count the skips so the caller can be told "N jobs were skipped because they could not be described". Throttling arguably should not be swallowed at all, since retrying is the right response rather than silently returning a short list.

[Lokiiiiii]

F15 (minor): this re-derives the ordering rule that __str__ already implements, so "never drift" is not enforced here.

__str__ (lines 91-94) buckets on name.startswith("http_") in a single pass; these two comprehensions derive the same partition independently. They agree today — verified alpha, zeta, http_a, http_x from both — but by coincidence rather than by construction, and the docstring below asserts "Rows are ordered exactly as the printed table".

BenchmarkResult does this properly 160 lines down: _ordered_metric_pairs() is called by both. Same gap in _recommendation_view.py, where _perf_records() was added as "the rows of the printed table" but __str__ still builds perf_rows inline — and there the values already differ (None/"" in the frame vs "-" in the table).

So the commit message's claim that the shared helpers keep table and frame in step holds for 2 of the 5 surfaces. The "http_" literal is now a live predicate in four places (91, 117, 118, 292).

Suggested direction: give BenchmarkMetrics an _ordered_metric_pairs() equivalent and have both callers use it, and refactor _RecommendationView.__str__ onto _perf_records(). A single parametrised test comparing __str__ row order against to_dataframe().index across all five surfaces would turn the invariant from convention into something enforced.

[Lokiiiiii]

F16 (major): the index is a proxy for the row, and it goes stale — so this can still deploy a different row than the one passed.

This correctly fixes the duplicate-spec-name case I raised (verified: two rows sharing "dup", passing mb.recommendations[1] now provisions row 1's ModelPackage and ml.p4d.24xlarge x4). But the row object is discarded here and only an integer survives, and that integer is minted from a different list than the one it indexes into:

• minted at model_builder.py:5087 — rows = list(job.recommendations or []) snapshots the rows when .recommendations is accessed
• read at model_builder.py:5177-5180 — rows is re-read at deploy time, and :5179 may call refresh(), replacing the list wholesale from a later service response

Nothing ties the two together. A verifier asked to disprove the fix reproduced five silent misroutes:

input	deploys
row from a different ModelBuilder	that builder's row N — unrelated ModelPackage and instance type
stale row across a refresh() that reordered rows	wrong row
stale _index after a re-run returning a different same-length list	wrong row
captured .best after the job re-ran and row 0 changed	wrong row
row.raw (a public property), or a string / int / dict	row 0

The range guard at :5218 only fires when the list shrinks past the index; whenever the new list is long enough, a stale or foreign index is accepted with no error and no warning. Picking a different instance type on a p4d-class row is a real cost and capacity event, and the user has no signal it happened.

Suggested direction — either:

1. keep the index but verify it: after rec = rows[recommendation_index] (:5224) assert rec is recommendation.raw and raise otherwise. Two lines, and it closes every row in the table above including F6; or
2. stop using a proxy: pass the row's raw shape straight through and skip the rows lookup when a row object was supplied, which removes the two-reads coupling entirely.

Either way getattr(..., "_index", 0)'s 0 default should go — silently defaulting a mistyped argument to the top-ranked row is the root of the last table row.

On the test: test_duplicate_spec_deploys_the_exact_row_passed mocks _deploy_recommendation and asserts on its kwargs, so it proves the index is forwarded but not that the index selects the intended row. TestDeployRecommendationRowSelection already exercises selection for real against describe_model_package — the duplicate-spec case would be worth adding there so the fix is actually pinned.

[Lokiiiiii]

F17 (nit): the F13 fix overshot — this emits one WARNING per undescribable candidate, up to max_scan.

Raising this from DEBUG to WARNING was the right move, and the summary line at :126 carries the operator-facing information. But this per-candidate line fires on every skip, so a role that cannot DescribeAIBenchmarkJob at all produces up to DEFAULT_MAX_SCAN (1000) warnings for a single list_benchmarks() call. Measured with 250 denied candidates: 250 per-candidate warnings plus the one summary.

Suggested direction: log the first skip (or the first few) at WARNING for diagnosability and leave the rest to the summary, or log per-candidate at DEBUG and keep WARNING for the summary only. The summary already reports the count.

[Lokiiiiii]

F18 (nit): "All three limits are logged when hit" — max_results isn't.

Measured against the three cases: hitting max_scan warns (:120), skipped candidates warn (:126), and hitting max_results (:118) logs nothing. Natural exhaustion correctly stays silent — the while/else is right, I checked that specifically.

Revision 1 did log this one, and its docstring gave the reason: "logs when the scan is truncated so a silent cap never reads as 'all matches'." That rationale still applies to max_results — a caller who gets exactly 100 rows back has no way to tell whether that is all the matches or just the cap. The log went away with the rewrite.

Suggested direction: either restore a log on the max_results break, or narrow the docstring claim to the two limits that are logged.