Package PTODSL into PTOAS wheel and unify install flows#900
Conversation
Codex Review该评论由 review 机器人自动更新。
SummaryReview failed at stage Findings未生成结构化 findings，因为 review 过程提前失败。 Log Tail |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
zhangstevenunity
left a comment
zhangstevenunity
left a comment
There was a problem hiding this comment.
Reviewed the PTODSL packaging / install-flow consolidation. No blockers - the design is coherent and well-guarded: fail-fast _assert_installed_ptodsl_payload in the build backend, has_ptodsl_init + in-zip re-check in create_wheel.sh, and the compiler-only-tarball guards added to both collect_ptoas_dist*.sh.
Coverage of the bootstrap removal looks correct. On the ci_sim path, test/dsl-st no longer self-locates ptodsl, but scripts/sim_dsl.sh sources scripts/ptoas_env.sh, which puts ${PTO_INSTALL_DIR} on PYTHONPATH, and the new ptodsl/CMakeLists.txt install rule stages ${PTO_INSTALL_DIR}/ptodsl/__init__.py - so import ptodsl resolves. The executable editable .pth also gets the ordering right: mlir_core first (avoiding the MLIR-shadowing pitfall ptoas_env.sh:111-113 explicitly warns about) and the repo ptodsl sources ahead of the install-tree copy. No stale references to the deleted ptodsl/pyproject.toml remain in CI/scripts.
Two inline notes below (both non-blocking), plus one trivial nit: ptodsl/tests/test_jit_compile.py now has an unused import sys (its only use was the removed sys.path.insert) - harmless at runtime but ruff/flake8 F401 would flag it.
| pto.tile.store(tile, dst) | ||
|
|
||
|
|
||
| mlir_text = wheel_compile_probe.compile().mlir_text() |
There was a problem hiding this comment.
This smoke test used to be two trivial imports; it now runs a full @pto.jit(target="a5").compile(). CompiledKernelHandle.build() -> build_module() calls module.operation.verify() (ptodsl/ptodsl/_tracing/runtime.py:91), and pto.tload has a verifier whose documented constraint is "product of partition-view sizes must match product of tile valid dims" (include/PTO/IR/PTOOps.td:648). This probe loads a dynamic [rows, cols] partition into a static [1, 128] tile, so it relies on the verifier skipping the dynamic case (it most likely does - verifiers only reject provable mismatches).
The concern is coverage: this probe now gates three release jobs - the docker builder stage, the docker runtime stage, and the mac wheel test - that previously only ran import mlir.ir / from mlir.dialects import pto. Given the PR notes "Testing: Not run in this PR flow," please run it once before merge (e.g. the docker builder-stage test_wheel_imports.sh, and one scripts/sim_dsl.sh test/dsl-st). A verifier rejection or a target/arch dependency here would fail all three image/wheel builds. (Risk is bounded: I confirmed the probe is fully in-process - no ptoas binary, numpy, or torch needed - and the op names / surface APIs it asserts on are correct.)
| set(PTODSL_BUILD_PACKAGE_DIR | ||
| "${PTODSL_BUILD_ROOT}/ptodsl") | ||
|
|
||
| add_custom_target(PTODSLPackage ALL |
There was a problem hiding this comment.
PTODSLPackage is an ALL target with no OUTPUT/DEPENDS/BYPRODUCTS, so the remove_directory + copy_directory pair re-copies the entire package on every ninja invocation, including no-op incremental builds. Consider an add_custom_command(OUTPUT <stamp> DEPENDS <sources>) + wrapper target, or file(COPY ...) at configure time, so staging only reruns when the sources actually change. Non-blocking.
A3 板测完成（有跳过）
|
4 participants
Summary
This PR packages PTODSL together with the PTOAS wheel and unifies the install and editable development flows so users no longer need to manually locate a separate PTODSL checkout.
Changes included:
Details
The wheel packaging flow now treats PTODSL as part of the PTOAS distribution instead of an external package that users must discover and wire up themselves.
To support local development, the editable flow is also consolidated at the repository root, so PTODSL developers can continue iterating from the source tree without maintaining a separate package install path.
The runtime and test/example codepaths are updated accordingly to stop injecting manual PTODSL path bootstrap logic.
Testing