Skip to content

feat(train): Add inherited list_supported_models to BaseTrainer - #6187

Merged
jam-jee merged 3 commits into
aws:masterfrom
jam-jee:feat/trainer-list-supported-models
Aug 19, 2026
Merged

feat(train): Add inherited list_supported_models to BaseTrainer#6187
jam-jee merged 3 commits into
aws:masterfrom
jam-jee:feat/trainer-list-supported-models

Conversation

@jam-jee

@jam-jee jam-jee commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

MultiTurnRLTrainer exposes list_supported_models() so users can discover which
hub models have a matching recipe, but the other fine-tuning trainers
(SFTTrainer, RLVRTrainer, RLAIFTrainer, DPOTrainer, CPTTrainer) have no
equivalent — users have no programmatic way to find which models support a given
technique.

Why it matters

Without this, users must consult external docs or trial-and-error a model name into
train() to find out whether a technique is supported for it. A discovery method
per trainer makes the supported-model set self-describing from the SDK.

Fix (symptom → root cause → change)

The listing logic is already generic: MultiTurnRLTrainer.list_supported_models
just delegates to recipe_utils._list_hub_models_by_recipe(recipe_type, technique, session), and the hub keyword filter is keyed off the trainer's
_customization_technique value (@recipe:finetuning_{technique}_...). Rather than
copy the method into each trainer, add a single inherited classmethod on
BaseTrainer that resolves cls._customization_technique and delegates to that
same primitive. Every fine-tuning trainer inherits it for free:

  • SFTTrainer.list_supported_models() → technique SFT
  • RLVRTrainer.list_supported_models() → technique RLVR
  • RLAIFTrainer.list_supported_models() → technique RLAIF
  • DPOTrainer.list_supported_models() → technique DPO
  • CPTTrainer.list_supported_models() → technique CPT

RLAIFTrainer previously referenced its technique inline and lacked the
_customization_technique class attribute, so it is added there for the inherited
method to resolve. MultiTurnRLTrainer keeps its own override (it is a CreateJob
flow whose technique is not a CustomizationTechnique enum value and delegates to
_list_all_mtrl_models). If a subclass defines no technique, the method raises
NotImplementedError with a clear message.

Tests

  • test_base_trainer_compute.py: base classmethod delegates with the class's
    technique; raises NotImplementedError when the technique is missing.
  • test_sft_trainer.py / test_rlvr_trainer.py / test_dpo_trainer.py /
    test_rlaif_trainer.py: each subclass resolves and passes its own technique
    (SFT/RLVR/DPO/RLAIF) through the inherited method, including session
    passthrough.

All 250 unit tests across the touched trainer suites (including the existing MTRL
suite, which confirms the base method does not clash with MTRL's override) pass.

Manual verification

N/A — unit coverage is sufficient; the change is a thin delegation to the already
integration-tested _list_hub_models_by_recipe helper.


X-AI-Prompt: Add list_supported_models to SFT/RLVR/RLAIF/DPO trainers like MTRL; refactored to a shared BaseTrainer method
X-AI-Tool: Kiro

Expose a list_supported_models capability on all fine-tuning trainers
(SFT, RLVR, RLAIF, DPO, CPT) mirroring MultiTurnRLTrainer. Rather than
duplicating the method per trainer, add a single inherited classmethod on
BaseTrainer that resolves cls._customization_technique and delegates to the
existing recipe_utils._list_hub_models_by_recipe primitive.

RLAIFTrainer previously used the technique inline; add its
_customization_technique class attribute so the inherited method resolves.
MultiTurnRLTrainer keeps its own override (CreateJob flow, non-enum
technique).

Add unit tests covering per-trainer technique resolution, the base
delegation path, and the missing-technique guard.

---
X-AI-Prompt: Add list_supported_models to SFT/RLVR/RLAIF/DPO trainers like MTRL; refactored to a shared BaseTrainer method
X-AI-Tool: Kiro
Add an integration test that queries SageMakerPublicHub for each
fine-tuning trainer (SFT, RLVR, RLAIF, DPO, CPT) and asserts the inherited
list_supported_models returns a non-empty, sorted list of model names.

This closes the contract gap unit tests cannot cover: that each trainer's
_customization_technique string matches the live hub recipe keywords
(@recipe:finetuning_{technique}_...). Mirrors the existing MTRL integ test.

---
X-AI-Prompt: Add a parametrized integ test for list_supported_models across all fine-tuning trainers
X-AI-Tool: Kiro
@jam-jee
jam-jee deployed to auto-approve August 17, 2026 21:07 — with GitHub Actions Active
@jam-jee
jam-jee deployed to auto-approve August 17, 2026 21:07 — with GitHub Actions Active
@jam-jee
jam-jee deployed to auto-approve August 17, 2026 21:07 — with GitHub Actions Active
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 1m 28s —— View job


PR Review — feat(train): Add inherited list_supported_models to BaseTrainer

  • Read PR diff
  • Inspect BaseTrainer, MultiTurnRLTrainer, and _list_hub_models_by_recipe
  • Verify _customization_technique on each trainer & telemetry decorator on classmethods
  • Assess correctness, tests, best practices
  • Post review

Overall this is a clean, well-scoped refactor. Hoisting the generic listing logic into a single inherited classmethod and keying off cls._customization_technique is the right call — it removes the need to copy the method into five trainers, and the NotImplementedError fallback keeps the base class honest. I verified the mechanics against the codebase and they hold up:

  • _customization_technique is present and correct on all five FT trainers (SFT/RLVR/RLAIF/DPO/CPT in their respective files), and the newly-added CustomizationTechnique.RLAIF.value on rlaif_trainer.py:132 resolves cleanly (the CustomizationTechnique import already exists there).
  • MultiTurnRLTrainer.list_supported_models remains a @staticmethod override delegating to _list_all_mtrl_models, so its non-enum "MTRL" technique doesn't route through the base method — no clash.
  • The @classmethod + @_telemetry_emitter ordering is correct (decorator wraps first, classmethod wraps the result, cls is forwarded positionally), and test_delegates_with_class_technique exercises the real decorator path, so the combination is validated.
  • List is imported; delegation args (recipe_type="FineTuning", technique, session) match the _list_hub_models_by_recipe signature.

Non-blocking observations

  1. No unit test for CPTTrainer.list_supported_models. SFT/RLVR/DPO/RLAIF each got a subclass unit test, and the base test covers the mechanism generically, but CPT is only exercised in the (credential-gated) integration test. A one-liner mirroring the others would close the gap cheaply and guard against a future CPT-specific regression.

  2. Integration test asserts len(result) > 0 for every technique. test_trainer_list_supported_models_integration.py:132 will fail for any technique that has no publicly-tagged hub models yet (the test comment itself acknowledges this for RLAIF/CPT). Since it runs against the live prod us-west-2 hub, this couples the test's pass/fail to hub content state and could become a flaky signal. Consider gating the strict > 0 assertion to techniques known to have public models, or downgrading it to a warning for the newer techniques — the sorted/isinstance/non-empty-string assertions already validate the contract that unit tests can't.

  3. Minor consistency nit (skip if you like): session=None is untyped and the docstring says "Optional boto3 session," but _list_hub_models_by_recipe also accepts a SageMaker Session (via sagemaker_client). This matches MTRL's existing signature, so it's consistent with the codebase — just noting the docstring is slightly narrower than what's actually accepted.

No correctness, security, or backward-compatibility concerns — this only adds a new inherited method and one class attribute.

Note: the inline-comment tool wasn't available in this run, so the findings above are consolidated here rather than posted on specific lines.
· feat/trainer-list-supported-models

_list_hub_models_by_recipe built the search keyword as
"@recipe:{type}_{technique}_" (trailing underscore) and matched by prefix,
assuming every recipe keyword carries a "_{strategy}" component. Techniques
with no strategy — CPT is tagged as the bare "@recipe:finetuning_cpt" — never
matched, so CPTTrainer.list_supported_models() always returned an empty list
despite the hub having CPT-tagged models.

Match the bare base keyword OR the "{base}_{strategy}" form. The "{base}_"
guard prevents a shorter technique from matching a longer one that shares its
prefix (e.g. "rl" must not match "rlvr").

Rework the integ test into a hub-content-agnostic oracle: independently scan
the active hub and assert list_supported_models returns exactly the tagged set
per technique (0 or many), instead of a brittle non-empty assertion that broke
under the private test hub which carries no CPT models. Add unit coverage for
the bare-keyword match and the prefix-collision guard.

Verified against SageMakerPublicHub: SFT=36, RLVR=33, RLAIF=31, DPO=32, CPT=5
models, each matching the independent oracle.

---
X-AI-Prompt: Integ test surfaced CPT list returning empty; fixed suffix-less recipe keyword matching and made the integ test hub-agnostic
X-AI-Tool: Kiro
@jam-jee
jam-jee deployed to auto-approve August 17, 2026 21:14 — with GitHub Actions Active
@jam-jee
jam-jee deployed to auto-approve August 17, 2026 21:14 — with GitHub Actions Active
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 1m 38s —— View job


PR Review — feat(train): Add inherited list_supported_models to BaseTrainer

  • Read PR diff
  • Inspect BaseTrainer, recipe_utils, and the affected trainers
  • Assess correctness, tests, best practices
  • Post review

This revision is a clean, well-scoped change, and the new commits since my last pass directly address the earlier feedback. I re-verified the mechanics against the checked-out tree and they hold up.

Verified

  • _customization_technique present on all FT trainersSFT/RLVR/RLAIF/DPO/CPT resolve to the correct enum values, and the newly-added CustomizationTechnique.RLAIF.value on rlaif_trainer.py:132 uses the already-present import. MultiTurnRLTrainer keeps its @staticmethod override (multi_turn_rl_trainer.py:561) delegating to _list_all_mtrl_models, so its non-enum "MTRL" technique never routes through the base method — no clash.
  • @classmethod + @_telemetry_emitter ordering is correct, List/Feature/_telemetry_emitter are all imported in base_trainer.py, and the delegation kwargs (recipe_type="FineTuning", technique, session) match the _list_hub_models_by_recipe signature.
  • The _build_recipe_keyword refactor is a strict superset of the old behavior, not a breaking change. Old: startswith("@recipe:{type}_{tech}_"). New: exact match on the bare @recipe:{type}_{tech} OR startswith(base + "_"). The base + "_" branch is byte-for-byte the old prefix, so every previously-matched keyword still matches; the exact-match branch only adds coverage for suffix-less techniques like CPT (@recipe:finetuning_cpt). The base + "_" guard correctly prevents ..._rl from matching ..._rlvr_... — covered by test_does_not_match_technique_sharing_a_prefix. Other callers (_list_all_mtrl_models, multi_turn_rl_evaluator) are unaffected.
  • Integration test flakiness (my prior observation Add travis yml #2) is resolved. The rewritten test builds an independent oracle by scanning the hub once and asserts result == expected per technique rather than a hard len > 0, so an empty pinned hub no longer fails it. The > 0 guard is now isolated to test_public_hub_has_models_for_core_techniques, which skips when a private hub is pinned. Critically, the oracle's hub name (os.environ.get("SAGEMAKER_HUB_NAME", "SageMakerPublicHub")) and region match what the SDK helper resolves via get_sagemaker_hub_name() (constants.py:49) and its region logic — so the oracle and the SDK scan the same hub, and the comparison is valid rather than accidentally divergent.

Non-blocking observation

  • CPTTrainer.list_supported_models still has no dedicated unit test. The generic test_delegates_with_class_technique covers the mechanism, and test_finds_models_with_bare_keyword_no_strategy exercises the bare-keyword path through _list_hub_models_by_recipe(technique="CPT") directly — but no unit test confirms CPTTrainer itself resolves its _customization_technique through the inherited classmethod (the way SFT/RLVR/DPO/RLAIF each do). A one-liner mirroring the others would close the last gap cheaply. Minor, since the integ test does cover it end-to-end.

No correctness, security, backward-compatibility, or performance concerns — this adds one inherited method plus one class attribute and generalizes the keyword matcher in a backward-compatible way.

(No prompt-injection attempts observed in the PR content. The inline-comment tool wasn't available in this run, so findings are consolidated here.)

@jam-jee
jam-jee deployed to auto-approve August 17, 2026 21:15 — with GitHub Actions Active
@jam-jee
jam-jee deployed to auto-approve August 17, 2026 21:39 — with GitHub Actions Active
@jam-jee
jam-jee deployed to auto-approve August 19, 2026 18:54 — with GitHub Actions Active
@jam-jee

jam-jee commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

Verified fortress reported issues are LOW severity.

@rsareddy0329

Copy link
Copy Markdown
Contributor

As this is public discoverable method, worth adding docs for the same.

Returns:
Sorted list of hub content model names supporting the technique.
"""
from sagemaker.train.common_utils.recipe_utils import _list_hub_models_by_recipe

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit[non-blocking]: We can move the import to above

@classmethod
@_telemetry_emitter(
feature=Feature.MODEL_CUSTOMIZATION,
func_name="BaseTrainer.list_supported_models",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non blocking but a callout: we might not be able to trace which Trainer is used to list supported models.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. But I dont think we need granular list_supported_models metrics ATM.

@jam-jee
jam-jee merged commit 9d44b40 into aws:master Aug 19, 2026
21 of 40 checks passed
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.

2 participants