fix: make sagemaker-core the sole owner of sagemaker/__init__.py (#5795) - #6333
mohamedzeidan2021 wants to merge 1 commit into
Conversation
|
Claude finished @mohamedzeidan2021's task in 1m 33s —— View job Code Review — PR #6333
SummaryThis is a correct, well-scoped fix. I recommend it for merge. The change deletes the redundant 3-line I verified the key claims against the checked-out tree:
TestsThe three added
Security / performanceNo security or performance implications. No public API or importable-surface changes. Note: the inline-comment tool was not available in this environment, so I've consolidated all feedback here. Nothing found rose above minor observations, so inline anchoring wasn't essential. |
jam-jee
left a comment
There was a problem hiding this comment.
Summary: The packaging change is correct and I'd like to see it land. Verified: all four pyproject.toml use find(where=["src"], include=["sagemaker*"], namespaces=true), so train/, ai_registry/, serve/, mlops/ are still discovered as PEP 420 portions; MANIFEST.in doesn't re-add an init; sagemaker-core's regular __init__.py (extend_path + register_removed_module_finder) coexists with PEP 420 portions on py>=3.3 because PathFinder returns the regular spec regardless of sys.path order and extend_path uses find_spec(...).submodule_search_locations. Bonus: this also stops pip uninstall sagemaker-train from deleting the shared init via its RECORD. But the PR breaks the pylint gate in all three sub-packages.
Blocking
sagemaker-train/tox.ini, sagemaker-serve/tox.ini, sagemaker-mlops/tox.ini [testenv:pylint] run python -m pylint --rcfile=../.pylintrc -j 0 src/sagemaker --fail-under=9.9. pylint 3.0.3 (pinned) treats a directory argument as a package and requires __init__.py, so codestyle-doc-tests now fails in train, serve and mlops with:
src/sagemaker/__init__.py:1:0: F0010 error while code parsing: Unable to load file src/sagemaker/__init__.py: [Errno 2] No such file or directory
flake8/docstyle/black/twine in the same jobs are green, so this is the only failure. Fix in this PR by pointing pylint at the real packages — src/sagemaker/train src/sagemaker/ai_registry, src/sagemaker/serve, src/sagemaker/mlops — or by adding --recursive=y.
Nits
tests/unit/test_namespace_packaging.py(all three) checks the source tree, not the built wheel, which is the artifact #5795 is actually about. Apython -m build --wheel+ zip-listing assertion in the twine env would catch a futurepyprojectregression.sagemaker-train/tests/unit/test_namespace_packaging.py:38asserts onlysagemaker/train/__init__.py; train also shipssagemaker/ai_registry/__init__.py.- No positive guard in
sagemaker-coreasserting that it does shipsagemaker/__init__.py.
CI attribution: integ-tests (sagemaker-train) test_multi_container_local_mode fails inside the training container (FileExistsError: '/opt/ml/model/code/' in algo-2), unrelated to host packaging, 1/129. integ-tests (serve/mlops) and integ-tests-us-east-1 are the usual capacity/quota failures seen on every PR. codecov/project is the partial-matrix artefact (3 of 4 unit jobs ran; patch green).
…_.py (aws#5795) sagemaker-core, -train, -serve, and -mlops each shipped an identical top-level sagemaker/__init__.py. Installing two of them writes the same file path, which strict installers and OS package managers (e.g. FreeBSD pkg) reject as a conflict. It also meant the last-installed wheel silently clobbered the others' init -- and sagemaker-core's is the only one that registers the v2->v3 removed- module migration finder, so that guidance could be lost depending on install order. Make sagemaker-core the sole owner of sagemaker/__init__.py (it is a required dependency of the other three, so the file is always present exactly once). The other three now contribute to the sagemaker namespace via PEP 420 / the pkgutil extend_path in core's init. Add per-distribution regression guards.
f758bef to
9796730
Compare
Issue
Fixes #5795.
sagemaker-core,sagemaker-train,sagemaker-serve, andsagemaker-mlopseach shipped an identical top-levelsagemaker/__init__.py. Installing two of them writes the same file path into site-packages, which strict installers and OS package managers (e.g. FreeBSDpkg, the reporter's case) reject as a file conflict.It also had a latent correctness bug: whichever wheel installed last silently clobbered the others'
sagemaker/__init__.py, and only sagemaker-core's init registers the v2→v3 removed-module migration finder — soimport sagemaker.estimatorguidance could be lost depending on install order.Fix
Make sagemaker-core the sole owner of
sagemaker/__init__.pyby deleting the redundant 3-linepkgutil.extend_pathstub fromsagemaker-train,sagemaker-serve, andsagemaker-mlops. sagemaker-core is a required dependency (sagemaker-core>=2.23.0) of all three, so the file is always present exactly once. The other three contribute to thesagemakernamespace as PEP 420 namespace portions (all fourpyproject.tomlalready setnamespaces = true), aggregated at runtime by theextend_pathin core's init.Per-distribution regression guards added:
sagemaker-{train,serve,mlops}/tests/unit/test_namespace_packaging.py.Validation
sagemaker/<subpkg>/__init__.pybut no top-levelsagemaker/__init__.py. Negative control: onmasterthe train wheel shipssagemaker/__init__.py(the conflicting file).srcdirs on PYTHONPATH, in multiple orderings:import sagemaker.core/.train/.serve/.mlopsall succeed;sagemaker.__file__always resolves to core's init; the migration finder stays onsys.meta_path;import sagemaker.estimatorstill yields the actionable v3 guidance.black/flake8clean.Backwards compatibility
No public API or importable surface changes. The deleted stubs contained only a docstring +
extend_path; core's init already provides bothextend_pathand the migration finder.