Skip to content

fix: make sagemaker-core the sole owner of sagemaker/__init__.py (#5795) - #6333

Open
mohamedzeidan2021 wants to merge 1 commit into
aws:masterfrom
mohamedzeidan2021:fix/issue-5795-namespace-package-conflict
Open

mohamedzeidan2021 wants to merge 1 commit into
aws:masterfrom
mohamedzeidan2021:fix/issue-5795-namespace-package-conflict

Conversation

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

Issue

Fixes #5795.

sagemaker-core, sagemaker-train, sagemaker-serve, and sagemaker-mlops each shipped an identical top-level sagemaker/__init__.py. Installing two of them writes the same file path into site-packages, which strict installers and OS package managers (e.g. FreeBSD pkg, 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 — so import sagemaker.estimator guidance could be lost depending on install order.

Fix

Make sagemaker-core the sole owner of sagemaker/__init__.py by deleting the redundant 3-line pkgutil.extend_path stub from sagemaker-train, sagemaker-serve, and sagemaker-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 the sagemaker namespace as PEP 420 namespace portions (all four pyproject.toml already set namespaces = true), aggregated at runtime by the extend_path in core's init.

Per-distribution regression guards added: sagemaker-{train,serve,mlops}/tests/unit/test_namespace_packaging.py.

Validation

  • Built each sub-package wheel: they ship sagemaker/<subpkg>/__init__.py but no top-level sagemaker/__init__.py. Negative control: on master the train wheel ships sagemaker/__init__.py (the conflicting file).
  • Import matrix with all four src dirs on PYTHONPATH, in multiple orderings: import sagemaker.core/.train/.serve/.mlops all succeed; sagemaker.__file__ always resolves to core's init; the migration finder stays on sys.meta_path; import sagemaker.estimator still yields the actionable v3 guidance.
  • New guards pass on branch and fail on master. black/flake8 clean.

Backwards compatibility

No public API or importable surface changes. The deleted stubs contained only a docstring + extend_path; core's init already provides both extend_path and the migration finder.

@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 1m 33s —— View job


Code Review — PR #6333

  • Read the PR diff (/tmp/pr.diff)
  • Verify packaging config (pyproject.toml namespaces = true, find)
  • Confirm sagemaker-core retains sagemaker/__init__.py (extend_path + migration finder)
  • Review the added regression tests
  • Post review

Summary

This is a correct, well-scoped fix. I recommend it for merge. The change deletes the redundant 3-line pkgutil.extend_path stub sagemaker/__init__.py from sagemaker-train, sagemaker-serve, and sagemaker-mlops, leaving sagemaker-core as the sole owner of that path, and adds per-distribution regression guards.

I verified the key claims against the checked-out tree:

  • sagemaker-core remains the sole sagemaker/__init__.py (sagemaker-core/src/sagemaker/__init__.py) and it carries both pkgutil.extend_path and the v2→v3 removed-module migration finder. So no functional behavior is lost by deleting the stubs — the deleted files only contained extend_path, which core already provides.
  • All four pyproject.toml set namespaces = true with include = ["sagemaker*"] and where = ["src"]. With the top-level __init__.py gone, find_namespace_packages correctly discovers each distribution's own subpackages as PEP 420 portions (e.g. sagemaker.train + sagemaker.ai_registry for train; sagemaker.serve; sagemaker.mlops), while core's regular sagemaker/__init__.py roots the aggregate package and extend_path merges the rest.
  • sagemaker-core>=2.23.0 is a hard dependency of all three, so the single owning __init__.py is always present exactly once — import sagemaker can never end up with no init.
  • MANIFEST.in (train) only does recursive-include src *.json, so it does not re-introduce the __init__.py into the wheel.
  • The latent correctness bug described in the PR body is real: since the stubs differed from core's fuller init, install order could let a stub clobber core's init and silently drop the migration finder. Consolidating ownership eliminates that.

Tests

The three added test_namespace_packaging.py guards are reasonable and correct (parents[2]/"src" resolves to each distribution root's src). Two minor observations, neither blocking:

  • The guards assert on the source layout (the file is absent from src/, the subpackage init is present). They don't exercise the built wheel or the runtime import matrix that the PR description validated manually. That richer validation is exactly what would catch a future pyproject.toml/setuptools regression (e.g. someone flipping namespaces), but it lives outside CI. Consider a follow-up build-artifact check if this class of bug recurs — not required for this PR.
  • test_still_ships_the_subpackage_init for sagemaker-train only checks sagemaker/train/__init__.py, not the second package sagemaker/ai_registry/__init__.py. Harmless; one representative is enough for a layout guard.

Security / performance

No 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.
· branch fix/issue-5795-namespace-package-conflict

@jam-jee jam-jee left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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. A python -m build --wheel + zip-listing assertion in the twine env would catch a future pyproject regression.
  • sagemaker-train/tests/unit/test_namespace_packaging.py:38 asserts only sagemaker/train/__init__.py; train also ships sagemaker/ai_registry/__init__.py.
  • No positive guard in sagemaker-core asserting that it does ship sagemaker/__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.

This branch is being deployed

1 queued deployment
auto-approve — 9796730d Deployed Sep 28, 2026 by mohamedzeidan2021 via wait-for-approval #1556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants