feat: support ptodsl#725
Conversation
* feat: update workflow * feat: rename pto.aicore to pto.kernel --------- Co-authored-by: mouliangyu <mouliangyu@huawei.com>
* pip install ptoas * use pip install in CI * wheels pipelines use pip install * add missing license header * fix pip setup
Codex Review该评论由 review 机器人自动更新。
Summary发现 2 个问题：版本号被回退会影响发布和产物版本，TileLang DSL 的旧方言导入在常规安装包中会失败。 Findings
该 PR 相对 origin/main 把
这里仍从 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new developer skills under .codex/skills, adds the 3rdparty/PTO-Gym submodule, updates .gitignore, and implements a PEP 517 build backend in _ptoas_build_backend.py along with updated Docker and distribution scripts to package TileLang resources. It also updates CMakeLists.txt to register new tests and adds extensive documentation. The reviewer feedback highlights critical configuration issues, such as missing directories (tilelang-dsl and ptodsl/tests) that will break CMake, and configure-time evaluation of $ENV{PYTHONPATH}. Additionally, several improvements are suggested for the Python build backend, including failing fast on missing package directories, handling non-directory paths safely when removing directories, explicitly passing PY_PACKAGE_DIR to the wheel creation script, and adhering to PEP 427 by omitting the non-integer Build tag from the wheel metadata.
| set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/python/pto) | ||
|
|
||
| add_subdirectory(python) | ||
| add_subdirectory(tilelang-dsl) |
There was a problem hiding this comment.
The directory tilelang-dsl is added via add_subdirectory, but it is missing from the repository and not defined as a submodule in .gitmodules. This will cause CMake configuration to fail immediately. Please ensure the tilelang-dsl directory is included in the repository or added as a submodule.
| if(BUILD_TESTING) | ||
| enable_testing() | ||
| if(PTO_ENABLE_PYTHON_BINDING) | ||
| add_subdirectory(ptodsl/tests) |
There was a problem hiding this comment.
The directory ptodsl/tests is added via add_subdirectory, but the ptodsl directory is missing from the repository. This will cause CMake configuration to fail. Please ensure the ptodsl directory is included in the repository.
| set_tests_properties(tilelang_dsl_import PROPERTIES | ||
| ENVIRONMENT "PYTHONPATH=${CMAKE_BINARY_DIR}/python:$ENV{PYTHONPATH}" | ||
| ) |
There was a problem hiding this comment.
Evaluating $ENV{PYTHONPATH} at configure time is an anti-pattern because it captures the environment of the configure-time shell rather than the test-run shell. Furthermore, if PYTHONPATH is empty at configure time, this results in a trailing colon (e.g., PYTHONPATH=/path/to/bin:), which tells Python to prepend the current working directory (.) to sys.path, posing a security and predictability risk.
If CMake 3.22+ is supported, consider using ENVIRONMENT_MODIFICATION to prepend to PYTHONPATH dynamically:
set_tests_properties(tilelang_dsl_import PROPERTIES
ENVIRONMENT_MODIFICATION "PYTHONPATH=path_list_prepend:${CMAKE_BINARY_DIR}/python"
)Otherwise, avoid appending $ENV{PYTHONPATH} directly if it might be empty.
set_tests_properties(tilelang_dsl_import PROPERTIES
ENVIRONMENT "PYTHONPATH=${CMAKE_BINARY_DIR}/python"
)
| set_tests_properties(tilelang_dsl_unittest PROPERTIES | ||
| ENVIRONMENT "PYTHONPATH=${CMAKE_BINARY_DIR}/python:$ENV{PYTHONPATH}" | ||
| ) |
There was a problem hiding this comment.
Evaluating $ENV{PYTHONPATH} at configure time is an anti-pattern because it captures the environment of the configure-time shell rather than the test-run shell. Furthermore, if PYTHONPATH is empty at configure time, this results in a trailing colon (e.g., PYTHONPATH=/path/to/bin:), which tells Python to prepend the current working directory (.) to sys.path, posing a security and predictability risk.
If CMake 3.22+ is supported, consider using ENVIRONMENT_MODIFICATION to prepend to PYTHONPATH dynamically:
set_tests_properties(tilelang_dsl_unittest PROPERTIES
ENVIRONMENT_MODIFICATION "PYTHONPATH=path_list_prepend:${CMAKE_BINARY_DIR}/python"
)Otherwise, avoid appending $ENV{PYTHONPATH} directly if it might be empty.
set_tests_properties(tilelang_dsl_unittest PROPERTIES
ENVIRONMENT "PYTHONPATH=${CMAKE_BINARY_DIR}/python"
)
|
|
||
| def _cmake_configure_and_build(): | ||
| """CMake configure + Ninja build + install.""" | ||
| _BUILD_DIR.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
If _MLIR_PY_PKG (which is derived from LLVM_BUILD_DIR) does not exist, the build will fail much later during the wheel creation or dialect installation phase. Checking its existence early in _cmake_configure_and_build provides a much clearer error message and avoids wasting time on a compilation that is guaranteed to fail.
| _BUILD_DIR.mkdir(parents=True, exist_ok=True) | |
| _BUILD_DIR.mkdir(parents=True, exist_ok=True) | |
| if not _MLIR_PY_PKG.exists(): | |
| raise RuntimeError( | |
| f"MLIR python package directory not found at {_MLIR_PY_PKG}. " | |
| "Please ensure LLVM_BUILD_DIR is set correctly and LLVM was built with python bindings enabled." | |
| ) |
| if dst.exists(): | ||
| shutil.rmtree(dst) | ||
| shutil.copytree(tilelang_src, dst) |
There was a problem hiding this comment.
Using shutil.rmtree(dst) directly when dst.exists() is true can fail with NotADirectoryError if dst is a file or a symlink rather than a directory. A safer approach is to check if dst is a directory and not a symlink before calling shutil.rmtree, and use dst.unlink() otherwise.
if dst.exists() or dst.is_symlink():
if dst.is_dir() and not dst.is_symlink():
shutil.rmtree(dst)
else:
dst.unlink()
shutil.copytree(tilelang_src, dst)| env = os.environ.copy() | ||
| env.update({ | ||
| "PTO_SOURCE_DIR": str(_REPO), | ||
| "PTO_INSTALL_DIR": str(_PTO_INSTALL_DIR), | ||
| "LLVM_BUILD_DIR": str(_LLVM_BUILD_DIR), | ||
| }) |
There was a problem hiding this comment.
The create_wheel.sh script relies on PY_PACKAGE_DIR to copy the dialect files and TileLang resources. When running locally outside of Docker, PY_PACKAGE_DIR might not be set in the user's environment, causing the script to fail or copy files to incorrect locations. Explicitly passing PY_PACKAGE_DIR in the environment dictionary ensures reliable local builds.
env = os.environ.copy()
env.update({
"PTO_SOURCE_DIR": str(_REPO),
"PTO_INSTALL_DIR": str(_PTO_INSTALL_DIR),
"LLVM_BUILD_DIR": str(_LLVM_BUILD_DIR),
"PY_PACKAGE_DIR": str(_MLIR_PY_PKG),
})| "Generator: _ptoas_build_backend\n" | ||
| "Root-Is-Purelib: True\n" | ||
| f"Tag: {tag}\n" | ||
| "Build: editable\n" | ||
| ).encode() | ||
| metadata_content = ( | ||
| "Metadata-Version: 2.1\n" |
There was a problem hiding this comment.
According to PEP 427, the Build tag in the WHEEL metadata is optional, but if present, it must be an integer (e.g., Build: 1). Using a non-integer value like Build: editable violates the specification and can cause packaging or installation tools to fail. It is safer to omit the Build tag entirely.
wheel_meta = (
"Wheel-Version: 1.0\n"
"Generator: _ptoas_build_backend\n"
"Root-Is-Purelib: True\n"
f"Tag: {tag}\n"
).encode()20 participants
No description provided.