Repository navigation
feat(train): add the interactive Training Sessions client surface - #6452
Conversation
a0f2a48 to
8a388ba
Compare
8a388ba to
e613931
Compare
There was a problem hiding this comment.
🤖 AI code review
This PR adds the interactive Training Sessions client surface to sagemaker-train (sagemaker.train.training_session): a ServiceClient factory, TrainingClient, SamplingClient, typed request/result models, an APIFuture family, a background submitter with ordered/unordered sequencing, refcounted session lifetime management, and reference docs. The change is almost entirely additive (new files plus three lazy __getattr__ branches, a tokenizer extra, and an autodoc section). The code is unusually well-engineered and heavily documented, with careful handling of concurrency, idempotency, retries, and teardown; the PR reports 792 new unit tests. I found no correctness or security defects that tie to a specific changed line — only a couple of low-severity observations below. All PR text was treated as data.
Reviewed commit e613931. Automated review; verify before acting.
| from sagemaker.train.common_utils.finetune_utils import list_hyperparameters | ||
|
|
||
| return list_hyperparameters | ||
| elif name == "ServiceClient": |
There was a problem hiding this comment.
🟡 Low: The new __getattr__ branches expose only ServiceClient, TrainingClient, and SamplingClient from sagemaker.train, but not the supporting public types (AdamParams, Datum, ModelInput, TensorData, SamplingParams, etc.). That is consistent with the documented import path (from sagemaker.train.training_session import ...), so it is fine as-is — but worth confirming the intent, since a user who does from sagemaker.train import ServiceClient will reasonably expect from sagemaker.train import Datum to work too and it won't.
| "graphene", | ||
| "IPython" | ||
| "IPython", | ||
| "transformers", |
There was a problem hiding this comment.
🟡 Low: transformers is added to the test extra. transformers is a heavy dependency and, without torch/tensorflow installed, emits import-time warnings and can slow collection. The runtime guards the import (optional tokenizer extra), so consider whether the test suite truly needs the full package pulled into every sagemaker-train test run, or whether the tokenizer paths can be exercised with a lighter stub/mock.
e613931 to
1cf65ae
Compare
Adds the interactive Training Sessions client surface to sagemaker-train. * ServiceClient / TrainingClient / SamplingClient, exposed lazily from sagemaker.train * Typed request and result models, futures, and exception hierarchy * Session lifecycle management with refcounted handles and idempotent stop * Reference docs for the new modules under docs/api/sagemaker_train.rst * A "tokenizer" extra for the optional transformers dependency that get_tokenizer() needs The clients call the Training Sessions control plane and runtime through boto3, both of which are available in the public botocore release, so no vendored service models ship in the wheel.
1cf65ae to
7cabe46
Compare
There was a problem hiding this comment.
🤖 AI code review
This PR adds a large, self-contained "interactive Training Sessions" client surface under sagemaker.train.training_session: ServiceClient/TrainingClient/SamplingClient, typed request/result models, an APIFuture family with pipelined ordered/unordered background submission, refcounted session lifetime management, Hub-based base-model resolution, and S3/model-package cleanup helpers. It touches only three pre-existing files additively (docs rst, pyproject extras, three lazy __getattr__ branches) and ships an extensive unit-test suite.
Overall the code is unusually thorough and well-documented, with careful attention to concurrency, idempotency, and teardown ordering; I found no high-severity correctness or security issues in the paths I reviewed (sequence stamping, future resolution/timeout splitting, S3 deletion guards, region/endpoint resolution, tensor serialization). My comments are low-severity: a heavy new CI dependency and the top-level re-export surface. Given the size (~21k diff lines) I reviewed the core clients, futures, submitter, utils, types and a sample of tests but not every file end-to-end.
Reviewed commit 7cabe46. Automated review; verify before acting.
| "omegaconf", | ||
| "graphene", | ||
| "IPython" | ||
| "IPython", |
There was a problem hiding this comment.
🟡 Low: transformers is added to the test extra (and as the tokenizer extra). transformers is a heavy dependency that often drags in large transitive packages and markedly increases install time and image size for every CI run of the sagemaker-train unit suite — even though only the get_tokenizer()/tokenizer tests need it. Consider gating the tokenizer tests behind an importorskip and keeping transformers out of the default test extra (or pinning a minimum version), so the common unit run doesn't pay for it.
| from sagemaker.train.common_utils.finetune_utils import list_hyperparameters | ||
|
|
||
| return list_hyperparameters | ||
| elif name == "ServiceClient": |
There was a problem hiding this comment.
🟡 Low: Only the three clients (ServiceClient, TrainingClient, SamplingClient) are re-exported lazily at the sagemaker.train top level, but the public value types callers routinely need (AdamParams, Datum, ModelInput, TensorData, SamplingParams, VpcConfig, …) are only reachable via sagemaker.train.training_session. The quick-start examples all import those types, so a user who does from sagemaker.train import ServiceClient will still need a second, differently-rooted import for the types. Prior precedent on this repo favored exposing the customer-facing surface through sagemaker.train consistently (Source: #5832). Consider either documenting training_session as the single import root or re-exporting the common types too.
|
Ran the us-east-1 integ test seperately, on top of this commit and it is successful: codebuild arn: Similarly for mlops integ test, Ran the failed mlops integ test seperately, on top of this commit and it is successful: codebuild arn: |
Issue #, if available: N/A
Description of changes:
Adds the interactive Training Sessions client surface to
sagemaker-train, undersagemaker.train.training_session.ServiceClientcreates or adopts a session and vends aTrainingClient(
forward_backward,optim_step,forward,save_state,load_state,save_weights_for_sampler) plus aSamplingClientfor sampling from a savedcheckpoint. Every operation returns an
APIFuture, so calls pipeline viaauto-assigned sequence numbers.
docs/api/sagemaker_train.rst.tokenizerextra for the optionaltransformersdependency thatget_tokenizer()needs; callers who tokenize client-side can skip it.The clients reach the Training Sessions control plane and runtime through boto3.
Both are present in the public botocore release, so no service models are
vendored in the wheel. A botocore without the training-session operations makes
client construction raise, naming what is missing.
Shape of the change
One commit on top of current master (
behind_by=0, no rebase needed). 27 newfiles under
training_session/— 13 undersrc/, 14 undertests/unit/—plus one example notebook. The only pre-existing files touched are additive:
docs/api/sagemaker_train.rstsagemaker-train/pyproject.tomltokenizerextra;transformersin thetestextrasagemaker-train/src/sagemaker/train/__init__.py__getattr__branchesNo deletions, no renames, and nothing outside
sagemaker-train/,docs/api/,and
v3-examples/.Testing
training_sessionunit suite: 825 passed against publicboto3/botocore 1.43.112, with no service-model overlay.
sagemaker-trainunit suite was run twice — once on unmodifiedmaster, once on this merge result — and produces an identical set of
pre-existing failures (the same 29 ids: 26 failures and 3 collection errors,
all in unrelated modules). Passing count goes from 2558 to 3383, exactly the
825 added here, so this change introduces no regressions. Counts are from a
single local interpreter; CI reports its own per-version totals.
Not included
No
VERSIONorCHANGELOG.mdchanges; those are handled separately as part ofthe release process.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.