Repository navigation
[Wheel] Migrate Python CLI modules to unified package - #3626
Conversation
8aae0e0 to
b792be8
Compare
|
Thanks for the detailed migration work. I have one question about the non-SKBUILD CMake installation path. In The project metadata still maps the entry point to: mooncake_client = "mooncake.cli_client:main" Would it make sense to add python/mooncake/cli_client.py to the direct CMake install list and include a small non-SKBUILD installation check? I may be overlooking another installation path, so I would appreciate your thoughts. |
- Add python/mooncake/_launcher.py that resolves bundled native binaries via importlib.resources, makes them executable on the fly, and raises a clear error instead of falling back to PATH (avoids running a mismatched build). - Slim cli/cli_client/cli_bench to thin pass-throughs of _launcher.locate, removing the triplicated _locate_binary helper. - Restore pkgutil.extend_path on python/mooncake/__init__.py so the unified package keeps the legacy namespace behaviour for independently installed mooncake subpackages. - Install python/mooncake/cli_client.py in the non-SKBUILD CMake install list. - Rework the unit/packaging tests for the launcher and thin wrappers, and extend the project tests to guard the scikit-build wiring and the build_wheel.sh staging.
|
Thanks for the follow-up changes. The cli_client.py installation path now looks covered. I noticed one related packaging question around transfer_engine_bench. The root pyproject.toml sets BUILD_BENCHMARK = false, while the project metadata still exposes: The CMake installation of the native transfer_engine_bench binary is also conditional on BUILD_BENCHMARK/BUILD_EXAMPLES. As a result, a default scikit-build wheel may contain the console-script wrapper but not the native binary that _launcher.locate() expects. Running transfer_engine_bench from such a wheel would then fail with FileNotFoundError. I understand this may predate this PR and may be outside the intended migration scope. Would you prefer to address it here by either enabling and testing the benchmark binary in the default wheel, or by excluding/limiting this entry point for wheel variants that do not build benchmarks? Or should we track it separately as a follow-up issue? |
|
Thanks for checking this. I rechecked the CMake options: Therefore, The earlier “known gap” comment I added to the packaging test was incorrect and likely contributed to the confusion—sorry about that. I have removed it locally. A custom build explicitly disabling |
|
Thanks for the follow-up changes. I rechecked the non-SKBUILD CMake installation path and noticed one small point that may be worth discussing. The CLI wrappers now import: The wrappers themselves are included in the direct CMake install rules, but I could not find a corresponding install entry for python/mooncake/_launcher.py. Since the legacy directory installation no longer contains this file, would a direct CMake installation still include _launcher.py? If not, commands such as mooncake_master, mooncake_client, or transfer_engine_bench might fail with ModuleNotFoundError: No module named 'mooncake._launcher' in that installation mode. This does not affect the scikit-build wheel path, which stages the launcher separately. I may be missing another installation path, so I wanted to raise this for discussion and get your view on whether it should be handled as part of this PR or tracked separately. |
Include _launcher.py alongside the non-SKBUILD CLI wrappers and remove the incorrect comment about the default benchmark binary configuration.
Description
Implements the general Python CLI subsystem slice of Phase 2 in RFC #3534.
mooncake.cli,mooncake.cli_client,mooncake.cli_bench, andmooncake.transfer_engine_topology_dumpintopython/mooncakeas their sole tracked source;python/testsand add focused source-tree wrapper/import coverage;python -mbehavior;Part of #3534.
Scope and overlap
No open PR was found that migrates this CLI slice. PR #3585 migrates Reshard, and PR #3586 migrates lightweight Store utilities/configuration; this branch is based directly on
mainand contains neither subsystem. Those PRs touch some shared migration surfaces (scripts/build_wheel.sh, packaging tests, CODEOWNERS, and labeler configuration), but this PR changes those surfaces only for the four CLI modules.Service entry points, SSD administration, native executables/bindings, Phase 3 native-extension renames, and the Phase 4/5 release cutover remain out of scope.
Module
mooncake-transfer-engine)mooncake-store)mooncake-reshard)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test commands:
Test results:
python -msmoke tests)twine checkpassed)No environment-only validation gaps remain. The build emitted one pre-existing GCC warning from RDMA control-frame code outside this Python-only diff.
Checklist
./scripts/code_format.sh(no C/C++ source changes; PR-scoped pre-commit passed)AI Assistance Disclosure
OpenAI Codex assisted with repository analysis, the scoped migration, packaging and command-smoke coverage, validation, and PR preparation. The human submitter remains responsible for reviewing and defending every changed line.