feat: actionable guidance for removed v2 interfaces#6004
Open
jam-jee wants to merge 7 commits into
Open
Conversation
5bd8224 to
b642f20
Compare
10 tasks
In v3 the v2 top-level modules (sagemaker.estimator, sagemaker.model, ...) were removed, so importing them fails with a bare "ModuleNotFoundError: No module named 'sagemaker.estimator'" that gives the caller no path forward. This adds an actionable migration experience. - raise_removed_in_v3() helper in sagemaker.core.deprecations: emits a DeprecationWarning and raises a ModuleNotFoundError naming the v3 replacement (and the exact import where the migration guide publishes one), plus the migration-guide link. - 12 curated "tombstone" modules under sagemaker/ for the high-traffic removed interfaces (estimator, model, predictor, base_predictor, predictor_async, transformer, tuner, processing, pipeline, multidatamodel, automl, algorithm). These ship with sagemaker-core. - A fallback meta-path finder (_RemovedV2ModuleFinder) registered from sagemaker.core.__init__ as a last resort: any other removed sagemaker.<name> gets a generic "not available in v3, see migration guide" message instead of a bare error. Appended to sys.meta_path so it never shadows real v3 packages or the tombstones, with a _KNOWN_V3_TOPLEVEL guard protecting real packages (core/train/serve/mlops/lineage/ai_registry). Adds tests/unit/test_removed_v2_modules.py (36 tests) covering tombstone messages and the fallback finder (registration, ordering, non-shadowing of real packages, pass-through of non-sagemaker and nested names).
The raised ModuleNotFoundError is the loud, authoritative signal (it stops execution and carries the full message with the v3 replacement + migration link). Logging the same message at WARNING duplicated it noisily on a path that always raises. Downgrade the logger call to debug in both raise_removed_in_v3 (tombstones) and the fallback finder (catch-all), leaving a breadcrumb for log-captured environments without the noise. The DeprecationWarning and the raise are unchanged.
test_finder_does_not_shadow_real_v3_packages imported sagemaker.train / sagemaker.serve to prove the fallback finder does not shadow them. That assumed those packages are installed -- but the sagemaker-core unit-test CI job installs sagemaker-core only, so the import raised ModuleNotFoundError and the test failed. Assert the actual contract instead: the finder's find_spec() returns None (pass-through) for real v3 top-level packages, which needs no import and holds in a core-only environment. Also covers mlops and lineage.
9df0320 to
aa6553a
Compare
Contributor
|
Can we point to samples of ModelTrainer instead of generic message - https://github.com/aws/sagemaker-python-sdk/blob/master/migration.md ? |
Address PR feedback: name the actual v3 replacement (identifiable in the
repo) and link its API docs, instead of vague package references.
- Add a v3_docs argument to raise_removed_in_v3 so each message carries a
direct readthedocs API link alongside the copy-pasteable import.
- Update all 12 tombstones to verified-importable v3 targets + docs URLs:
estimator/algorithm -> sagemaker.train.ModelTrainer
model/multidatamodel -> sagemaker.serve.ModelBuilder
predictor/base_predictor -> sagemaker.core.resources.Endpoint
predictor_async -> sagemaker.serve.ModelBuilder
transformer -> sagemaker.core.resources.TransformJob
tuner -> sagemaker.core.resources.HyperParameterTuningJob
processing -> sagemaker.core.resources.ProcessingJob
pipeline -> sagemaker.mlops.workflow.pipeline.Pipeline
automl -> sagemaker.core.resources.AutoMLJob
Corrects two paths that did not resolve in the shipped v3 code (the
migration guide's sagemaker.mlops.pipeline and sagemaker.mlops.processing
.DataProcessor): Pipeline is at sagemaker.mlops.workflow.pipeline, and
processing maps to the core ProcessingJob resource. All imports and docs
URLs were verified against the installed packages and readthedocs.
Tests assert the import statement and docs URL are present in every
message; updated expected symbols for tuner and processing.
Previously the fallback finder emitted "was removed in v3" guidance for ANY top-level sagemaker.<name> that was not a real v3 package -- including typos and hallucinated imports like `sagemaker.foobar`, which never existed and so should not claim to have been removed. Gate the finder on _REMOVED_V2_MODULES, the set of top-level modules that actually existed in v2 (derived from the master-v2 branch, minus names still present in v3). Unknown names now fall through to a plain ModuleNotFoundError with no deprecation warning and no migration link. Because the name is now known to be a real v2 module, the message states "was removed" (was "may have been removed"). Adds tests: known removed names -> guidance; unknown names -> plain error with no warning/link; finder find_spec pass-through for unknown names.
The fallback finder was registered only from sagemaker/core/__init__.py, so a removed module imported as the very first sagemaker statement (e.g. `import sagemaker.estimator` with nothing else loaded) missed the guidance and got a bare ModuleNotFoundError -- the finder had not been installed yet. Register it from sagemaker-core's namespace sagemaker/__init__.py, which runs on any `import sagemaker.*` (these are pkgutil-style namespace packages whose __init__ executes). Fully guarded so it can never interfere with importing the package. Tombstone modules were unaffected (real files), and normal usage already pulled in core transitively; this closes the first-import edge case. Adds a fresh-interpreter regression test asserting a removed module gets guidance even as the first sagemaker import.
Collaborator
Author
Updated message thrown. Resulting UX detailed in PR description. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available: N/A
Description of changes:
In v3 the v2 top-level modules were removed, so importing one fails with a bare, dead-end error:
This PR turns that into an actionable migration experience. (Usage telemetry for these events is a follow-up, tracked in a separate stacked PR so it can be reviewed independently.)
raise_removed_in_v3()insagemaker.core.deprecations— emits aDeprecationWarning, then raisesModuleNotFoundErrornaming the exact v3 replacement, the copy-pasteable import, and a direct API-docs link + the migration guide.sagemaker/for the high-traffic removed interfaces (estimator,model,predictor,base_predictor,predictor_async,transformer,tuner,processing,pipeline,multidatamodel,automl,algorithm). Each points at a verified-importable v3 target + its docs page. They ship withsagemaker-core(verified in the built wheel)._RemovedV2ModuleFinder) as a last resort for the remaining removed modules. Registered fromsagemaker-core's namespacesagemaker/__init__.pyso it is active on anyimport sagemaker.*(including when a removed module is the very first import). It is appended tosys.meta_pathso it never shadows real v3 packages or the tombstones. Two guards keep messages truthful:_KNOWN_V3_TOPLEVEL— never intercepts real v3 packages (core/train/serve/mlops/lineage/ai_registry), even when a package simply isn't installed._REMOVED_V2_MODULES— only emits guidance for names that actually existed in v2 (derived from themaster-v2top-level surface). Typos / hallucinated imports fall through to a plainModuleNotFoundError.Resulting UX
1. Curated (tombstoned) removed interface — precise replacement + import + docs:
2. Non-tombstoned removed v2 module — generic guidance via the fallback finder:
3. Unknown name (typo / never existed) — plain error, no misleading guidance:
Testing done:
tests/unit/test_removed_v2_modules.py— 44 tests, all pass. Covers: tombstone messages (import + docs URL present), the fallback finder (registration on bareimport sagemaker, append-ordering, non-shadowing of real packages, guidance only for known removed names, pass-through for unknown names), and a fresh-interpreter regression that a removed module gets guidance even as the firstsagemakerimport.sagemaker.train,sagemaker.serve,sagemaker.core) are unaffected.sagemaker-corewheel and confirmed all 12 tombstones are packaged.black(line-length 100) andflake8(repotox.ini) clean.Merge Checklist
General
import sagemakertime via the namespace package init (guarded, best-effort).migration.mdlists two paths that do not resolve in shipped v3 (sagemaker.mlops.pipeline,sagemaker.mlops.processing.DataProcessor); this PR anchors to the verified paths (sagemaker.mlops.workflow.pipeline,sagemaker.core.resources.ProcessingJob). The guide may need a follow-up fix.Tests
unique_name_from_baseto create resource names in integ tests (if appropriate) (N/A — unit tests only)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.