Copilot commented on code in PR #813:
URL: https://github.com/apache/incubator-graphar/pull/813#discussion_r2646687492
##########
python/CMakeLists.txt:
##########
@@ -27,7 +27,19 @@ project(
LANGUAGES CXX)
set(CMAKE_CXX_STANDARD 17)
-add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/../cpp
${CMAKE_BINARY_DIR}/graphar)
+
+# The Python wheel build (e.g. via cibuildwheel) may run from an isolated copy
of
+# the `python/` directory, where `../cpp` doesn't exist. In CI we bundle `cpp/`
+# into the project directory as `_bundled_cpp/`.
+if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/_bundled_cpp/CMakeLists.txt")
+ set(GRAPHAR_CPP_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/_bundled_cpp")
+elseif(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/../cpp/CMakeLists.txt")
+ set(GRAPHAR_CPP_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../cpp")
+else()
+ message(FATAL_ERROR "GraphAr C++ sources not found. Expected either
'_bundled_cpp/' or '../cpp/'.")
+endif()
+
Review Comment:
The GRAPHAR_CPP_SOURCE_DIR variable is set but later used to set
target_include_directories with paths like "${GRAPHAR_CPP_SOURCE_DIR}/src".
However, there's no verification that these subdirectories exist. Consider
adding checks to ensure that "${GRAPHAR_CPP_SOURCE_DIR}/src" and
"${GRAPHAR_CPP_SOURCE_DIR}/thirdparty" exist before using them, or the build
will fail with unclear error messages.
```suggestion
# Verify that the expected subdirectories exist before adding the C++
sources.
if(NOT EXISTS "${GRAPHAR_CPP_SOURCE_DIR}/src")
message(FATAL_ERROR "GraphAr C++ sources are missing the 'src' directory
under '${GRAPHAR_CPP_SOURCE_DIR}'.")
endif()
if(NOT EXISTS "${GRAPHAR_CPP_SOURCE_DIR}/thirdparty")
message(FATAL_ERROR "GraphAr C++ sources are missing the 'thirdparty'
directory under '${GRAPHAR_CPP_SOURCE_DIR}'.")
endif()
```
##########
.github/workflows/python-wheel-workflow.yml:
##########
@@ -0,0 +1,282 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+on:
+ # Trigger the workflow on push or pull request,
+ # but only for the main branch
+ push:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/python-wheel-workflow.yml'
+ - '.github/scripts/update_version.py'
+ pull_request:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/ci.yml'
+ - '.github/workflows/python.yml'
+ workflow_dispatch:
+ inputs:
+ publish_pypi:
+ description: "Publish to PyPI (manual runs only)"
+ required: true
+ default: false
+ type: boolean
+
+jobs:
+ build_sdist:
+ name: Build source distribution
+ runs-on: ubuntu-22.04
+ steps:
+ - uses: actions/checkout@v3
+
+ - name: Set up Python
+ uses: actions/setup-python@v4
+ with:
+ python-version: "3.9"
+
+ - name: Install dependencies
+ run: |
+ python -m pip install --upgrade pip
+ pip install build twine
+ - name: update pyporject version
+ if: github.event_name != 'workflow_dispatch' || !inputs.publish_pypi
+ run: |
+ python .github/scripts/update_version.py
+ - name: Build sdist
+ run: |
+ # Bundle C++ sources into python/ so the sdist contains them.
+ rm -rf python/_bundled_cpp
+ cp -a cpp python/_bundled_cpp
+ cd python
+ python -m build --sdist
+ - name: Store artifacts
+ uses: actions/upload-artifact@v4
+ with:
+ name: sdist
+ path: python/dist/*
+
+ build_wheels:
+ name: Build wheels on ${{ matrix.runner }}
+ runs-on: ${{ matrix.runner }}
+ needs: build_sdist
+ strategy:
+ matrix:
+ include:
+ # Job 1: Native x86_64 build
+ - platform: x86_64
+ runner: ubuntu-latest # This is the standard x86_64 runner
+ os: linux
+ manylinux: _2_28
+ deployment-target: ''
+
+ # Job 2: Native aarch64 build
+ - platform: aarch64
+ runner: ubuntu-22.04-arm # This is a native ARM64 runner
+ os: linux
+ manylinux: _2_28
+ deployment-target: ''
+
+ # Job 3: macOS arm64 build
+ - platform: arm64
+ runner: macos-latest
+ os: macos
+ deployment-target: '11.0'
+
+
+ env:
+ CIBW_PLATFORM: ${{ matrix.os }}
+ CIBW_BUILD: "cp310-* cp311-* cp312-* cp313-*"
+ CIBW_SKIP: "*-musllinux_*"
+ # Pin arch to the matrix platform
+ CIBW_ARCHS: ${{ (matrix.os == 'linux' && matrix.platform) || (matrix.os
== 'macos' && matrix.platform) || (matrix.os == 'windows' && 'AMD64') || 'auto'
}}
Review Comment:
The CIBW_ARCHS expression has redundant logic. The condition `(matrix.os ==
'windows' && 'AMD64')` will never be true because there's no Windows job in the
matrix. Consider simplifying this to: `${{ matrix.platform }}` since all matrix
entries now define the platform field explicitly.
```suggestion
CIBW_ARCHS: ${{ matrix.platform }}
```
##########
.github/scripts/update_version.py:
##########
@@ -0,0 +1,82 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import json
+import re
+import sys
+import urllib.request
+from packaging.version import Version
+
+PACKAGE_NAME = "graphar"
+FILE_PATH = "python/pyproject.toml"
+
+def get_next_version():
+ versions = []
+ urls = [
+ f"https://pypi.org/pypi/{PACKAGE_NAME}/json";,
+ f"https://test.pypi.org/pypi/{PACKAGE_NAME}/json";
+ ]
+
+ print(f"Fetching versions for {PACKAGE_NAME}...")
+ for url in urls:
+ try:
+ with urllib.request.urlopen(url, timeout=5) as r:
Review Comment:
The urllib.request.urlopen call has a hardcoded timeout of 5 seconds. In
environments with slow network connections, this may cause the version check to
fail unnecessarily. Consider increasing the timeout to at least 10 seconds for
more robust operation, or making it configurable.
##########
python/pyproject.toml:
##########
@@ -16,30 +16,43 @@
# under the License.
[build-system]
-requires = ["scikit-build-core>=0.3.3", "pybind11", "ninja ~= 1.11"]
+requires = ["scikit-build-core>=0.8.3", "pybind11"]
Review Comment:
The 'ninja ~= 1.11' dependency was removed from the build requirements.
While scikit-build-core may handle this internally, verify that Ninja is still
available during builds. If Ninja is required by your build process but not
automatically provided by scikit-build-core or the build environment, this
removal could cause build failures.
```suggestion
requires = ["scikit-build-core>=0.8.3", "pybind11", "ninja ~= 1.11"]
```
##########
.github/workflows/python-wheel-workflow.yml:
##########
@@ -0,0 +1,282 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+on:
+ # Trigger the workflow on push or pull request,
+ # but only for the main branch
+ push:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/python-wheel-workflow.yml'
+ - '.github/scripts/update_version.py'
+ pull_request:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/ci.yml'
+ - '.github/workflows/python.yml'
Review Comment:
The workflow triggers on paths that include '.github/workflows/ci.yml' and
'.github/workflows/python.yml', but the workflow file itself is named
'python-wheel-workflow.yml'. This means changes to this workflow file won't
trigger it on pull requests, which is inconsistent with push events (line 30
correctly includes this file). Update line 38-39 to include
'.github/workflows/python-wheel-workflow.yml' or the wildcard pattern
'.github/workflows/python*.yml' instead of the specific files that don't exist
or aren't relevant.
```suggestion
- '.github/workflows/python-wheel-workflow.yml'
- '.github/scripts/update_version.py'
```
##########
.github/workflows/python-wheel-workflow.yml:
##########
@@ -0,0 +1,282 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+on:
+ # Trigger the workflow on push or pull request,
+ # but only for the main branch
+ push:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/python-wheel-workflow.yml'
+ - '.github/scripts/update_version.py'
+ pull_request:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/ci.yml'
+ - '.github/workflows/python.yml'
+ workflow_dispatch:
+ inputs:
+ publish_pypi:
+ description: "Publish to PyPI (manual runs only)"
+ required: true
+ default: false
+ type: boolean
+
+jobs:
+ build_sdist:
+ name: Build source distribution
+ runs-on: ubuntu-22.04
+ steps:
+ - uses: actions/checkout@v3
Review Comment:
The workflow uses 'actions/checkout@v3', 'actions/setup-python@v4', and
'actions/upload-artifact@v4' which are older versions. Consider updating to the
latest major versions (v4 for checkout, v5 for setup-python if available, and
ensuring all actions are up to date) to benefit from security patches and
improvements. Additionally, it's a best practice to pin actions to specific
commit SHAs rather than tags for supply chain security.
##########
.github/workflows/python-wheel-workflow.yml:
##########
@@ -0,0 +1,282 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+on:
+ # Trigger the workflow on push or pull request,
+ # but only for the main branch
+ push:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/python-wheel-workflow.yml'
+ - '.github/scripts/update_version.py'
+ pull_request:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/ci.yml'
+ - '.github/workflows/python.yml'
+ workflow_dispatch:
+ inputs:
+ publish_pypi:
+ description: "Publish to PyPI (manual runs only)"
+ required: true
+ default: false
+ type: boolean
+
+jobs:
+ build_sdist:
+ name: Build source distribution
+ runs-on: ubuntu-22.04
+ steps:
+ - uses: actions/checkout@v3
+
+ - name: Set up Python
+ uses: actions/setup-python@v4
+ with:
+ python-version: "3.9"
+
+ - name: Install dependencies
+ run: |
+ python -m pip install --upgrade pip
+ pip install build twine
+ - name: update pyporject version
+ if: github.event_name != 'workflow_dispatch' || !inputs.publish_pypi
+ run: |
+ python .github/scripts/update_version.py
+ - name: Build sdist
+ run: |
+ # Bundle C++ sources into python/ so the sdist contains them.
+ rm -rf python/_bundled_cpp
+ cp -a cpp python/_bundled_cpp
+ cd python
+ python -m build --sdist
+ - name: Store artifacts
+ uses: actions/upload-artifact@v4
+ with:
+ name: sdist
+ path: python/dist/*
+
+ build_wheels:
+ name: Build wheels on ${{ matrix.runner }}
+ runs-on: ${{ matrix.runner }}
+ needs: build_sdist
+ strategy:
+ matrix:
+ include:
+ # Job 1: Native x86_64 build
+ - platform: x86_64
+ runner: ubuntu-latest # This is the standard x86_64 runner
+ os: linux
+ manylinux: _2_28
+ deployment-target: ''
+
+ # Job 2: Native aarch64 build
+ - platform: aarch64
+ runner: ubuntu-22.04-arm # This is a native ARM64 runner
+ os: linux
+ manylinux: _2_28
+ deployment-target: ''
+
+ # Job 3: macOS arm64 build
+ - platform: arm64
+ runner: macos-latest
+ os: macos
+ deployment-target: '11.0'
+
+
+ env:
+ CIBW_PLATFORM: ${{ matrix.os }}
+ CIBW_BUILD: "cp310-* cp311-* cp312-* cp313-*"
Review Comment:
The CIBW_BUILD environment variable skips Python 3.9, but the project's
pyproject.toml specifies Python 3.9 as the minimum supported version
(requires-python = ">=3.9") and includes it in classifiers. Either update
CIBW_BUILD to include "cp39-*" or update pyproject.toml to reflect that only
Python 3.10+ will have wheels available.
```suggestion
CIBW_BUILD: "cp39-* cp310-* cp311-* cp312-* cp313-*"
```
##########
.github/scripts/update_version.py:
##########
@@ -0,0 +1,82 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import json
+import re
+import sys
+import urllib.request
+from packaging.version import Version
+
+PACKAGE_NAME = "graphar"
+FILE_PATH = "python/pyproject.toml"
+
+def get_next_version():
+ versions = []
+ urls = [
+ f"https://pypi.org/pypi/{PACKAGE_NAME}/json";,
+ f"https://test.pypi.org/pypi/{PACKAGE_NAME}/json";
+ ]
+
+ print(f"Fetching versions for {PACKAGE_NAME}...")
+ for url in urls:
+ try:
+ with urllib.request.urlopen(url, timeout=5) as r:
+ data = json.load(r)
+ versions.extend(data.get("releases", {}).keys())
+ except Exception:
+ pass
+
+ if not versions:
+ return "0.0.1.dev1"
+
+ latest = max([Version(v) for v in versions])
+ print(f"Latest version found: {latest}")
+
+ if latest.is_devrelease:
+ return f"{latest.major}.{latest.minor}.{latest.micro}.dev{latest.dev +
1}"
+ else:
+ return f"{latest.major}.{latest.minor}.{latest.micro + 1}.dev1"
Review Comment:
The version increment logic has an issue. When the latest version is not a
dev release (line 51-52), this creates a new dev release by incrementing the
micro version (e.g., "0.13.0" → "0.13.1.dev1"). However, if the current
pyproject.toml version is "0.13.0-dev" (which is invalid per PEP 440) and the
latest published version is "0.13.0", this would try to create "0.13.1.dev1",
which doesn't match the base version in pyproject.toml. Consider checking the
current version in pyproject.toml and using that as the base for dev versioning.
##########
.github/workflows/python-wheel-workflow.yml:
##########
@@ -0,0 +1,282 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+on:
+ # Trigger the workflow on push or pull request,
+ # but only for the main branch
+ push:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/python-wheel-workflow.yml'
+ - '.github/scripts/update_version.py'
+ pull_request:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/ci.yml'
+ - '.github/workflows/python.yml'
+ workflow_dispatch:
+ inputs:
+ publish_pypi:
+ description: "Publish to PyPI (manual runs only)"
+ required: true
+ default: false
+ type: boolean
+
+jobs:
+ build_sdist:
+ name: Build source distribution
+ runs-on: ubuntu-22.04
+ steps:
+ - uses: actions/checkout@v3
+
+ - name: Set up Python
+ uses: actions/setup-python@v4
+ with:
+ python-version: "3.9"
+
+ - name: Install dependencies
+ run: |
+ python -m pip install --upgrade pip
+ pip install build twine
+ - name: update pyporject version
+ if: github.event_name != 'workflow_dispatch' || !inputs.publish_pypi
+ run: |
+ python .github/scripts/update_version.py
+ - name: Build sdist
+ run: |
+ # Bundle C++ sources into python/ so the sdist contains them.
+ rm -rf python/_bundled_cpp
+ cp -a cpp python/_bundled_cpp
+ cd python
+ python -m build --sdist
+ - name: Store artifacts
+ uses: actions/upload-artifact@v4
+ with:
+ name: sdist
+ path: python/dist/*
+
+ build_wheels:
+ name: Build wheels on ${{ matrix.runner }}
+ runs-on: ${{ matrix.runner }}
+ needs: build_sdist
+ strategy:
+ matrix:
+ include:
+ # Job 1: Native x86_64 build
+ - platform: x86_64
+ runner: ubuntu-latest # This is the standard x86_64 runner
+ os: linux
+ manylinux: _2_28
+ deployment-target: ''
+
+ # Job 2: Native aarch64 build
+ - platform: aarch64
+ runner: ubuntu-22.04-arm # This is a native ARM64 runner
+ os: linux
+ manylinux: _2_28
+ deployment-target: ''
+
+ # Job 3: macOS arm64 build
+ - platform: arm64
+ runner: macos-latest
+ os: macos
+ deployment-target: '11.0'
+
+
+ env:
+ CIBW_PLATFORM: ${{ matrix.os }}
+ CIBW_BUILD: "cp310-* cp311-* cp312-* cp313-*"
+ CIBW_SKIP: "*-musllinux_*"
+ # Pin arch to the matrix platform
+ CIBW_ARCHS: ${{ (matrix.os == 'linux' && matrix.platform) || (matrix.os
== 'macos' && matrix.platform) || (matrix.os == 'windows' && 'AMD64') || 'auto'
}}
+ CIBW_MANYLINUX_X86_64_IMAGE: ${{ matrix.os == 'linux' &&
format('manylinux{0}', matrix.manylinux) || '' }}
+ CIBW_MANYLINUX_AARCH64_IMAGE: ${{ matrix.os == 'linux' &&
format('manylinux{0}', matrix.manylinux) || '' }}
+ CIBW_ENVIRONMENT_WINDOWS: DISTUTILS_USE_SDK=1 MSSdk=1
+ CIBW_ENVIRONMENT_MACOS: ${{ matrix.os == 'macos' &&
format('MACOSX_DEPLOYMENT_TARGET={0} CMAKE_OSX_DEPLOYMENT_TARGET={0}
CFLAGS=-mmacosx-version-min={0} CXXFLAGS=-mmacosx-version-min={0}
LDFLAGS=-mmacosx-version-min={0}', matrix.deployment-target) || '' }}
+ CIBW_BEFORE_BUILD_LINUX: |
+ set -eux
+ if [ -f /etc/system-release-cpe ]; then
+ ALMA_MAJOR="$(cut -d: -f5 /etc/system-release-cpe | cut -d. -f1)"
+ else
+ . /etc/os-release
+ ALMA_MAJOR="${VERSION_ID%%.*}"
+ fi
+ dnf install -y 'dnf-command(config-manager)' || dnf install -y
dnf-plugins-core || true
+ # Follow official Apache Arrow install instructions for
AlmaLinux/RHEL-family
+ dnf install -y epel-release || \
+ dnf install -y oracle-epel-release-el${ALMA_MAJOR} || \
+ dnf install -y
https://dl.fedoraproject.org/pub/epel/epel-release-latest-${ALMA_MAJOR}.noarch.rpm
+ dnf install -y
https://packages.apache.org/artifactory/arrow/almalinux/${ALMA_MAJOR}/apache-arrow-release-latest.rpm
+ dnf config-manager --set-enabled epel || :
+ dnf config-manager --set-enabled powertools || :
+ dnf config-manager --set-enabled crb || :
+ dnf config-manager --set-enabled ol${ALMA_MAJOR}_codeready_builder || :
+ dnf config-manager --set-enabled
codeready-builder-for-rhel-${ALMA_MAJOR}-rhui-rpms || :
+ subscription-manager repos --enable
codeready-builder-for-rhel-${ALMA_MAJOR}-$(arch)-rpms || :
+ # manylinux images may carry older Arrow packages (e.g. arrow1700-*)
which
+ # conflict with the newer packages from the Apache Arrow repo (e.g.
arrow2200-*).
+ # Remove any preinstalled Arrow/Parquet RPMs so we install a
consistent set.
+ dnf remove -y 'arrow*' 'parquet*' || true
+ # Required for GraphAr C++ build via Arrow CMake packages
+ dnf install -y --allowerasing \
+ arrow-devel \
+ arrow-dataset-devel \
+ arrow-acero-devel \
+ parquet-devel \
+ libcurl-devel re2-devel ccache
+ steps:
+ - name: Checkout (needed for some tooling)
+ uses: actions/checkout@v3
+
+ - name: Set up Python
+ uses: actions/setup-python@v4
+ with:
+ python-version: "3.9"
+
+ - name: Set up Miniconda (macOS/Windows)
+ if: matrix.os == 'windows' || matrix.os == 'macos'
+ uses: conda-incubator/setup-miniconda@v3
+ with:
+ auto-activate-base: true
+ miniforge-version: latest
+ use-mamba: true
+
+ - name: Install Arrow (macOS)
+ if: matrix.os == 'macos'
+ shell: bash
+ run: |
+ set -euxo pipefail
+ mamba install -y -c conda-forge arrow-cpp
+ # Note: CONDA_PREFIX may be unset unless conda is activated in this
shell.
+ # setup-miniconda exports CONDA (base install prefix), which is
sufficient here.
+ echo "CMAKE_PREFIX_PATH=$CONDA" >> "$GITHUB_ENV"
+ # Optional sanity check: ensure Arrow dylib isn't built for a newer
macOS than deployment target.
+ if command -v otool >/dev/null 2>&1; then
+ ls -lah "$CONDA/lib" || true
+ if [ -f "$CONDA/lib/libarrow.dylib" ]; then
+ otool -l "$CONDA/lib/libarrow.dylib" | (grep -A3 -E
'LC_BUILD_VERSION|LC_VERSION_MIN_MACOSX' || true)
+ fi
+ fi
+ - name: Install Arrow (Windows)
+ if: matrix.os == 'windows'
+ shell: pwsh
+ run: |
+ mamba install -y -c conda-forge arrow-cpp
+ Add-Content $env:GITHUB_ENV
"CMAKE_PREFIX_PATH=$env:CONDA_PREFIX\\Library"
+ Add-Content $env:GITHUB_ENV
"PATH=$env:CONDA_PREFIX\\Library\\bin;$env:PATH"
+ - name: Download sdist artifact
+ uses: actions/download-artifact@v4
+ with:
+ name: sdist
+ path: sdist
+
+ - name: Extract sdist
+ shell: bash
+ run: |
+ set -euxo pipefail
+ ls -lah sdist
+ SDIST_FILE=""
+ for f in sdist/*.tar.gz sdist/*.zip; do
+ if [ -f "$f" ]; then
+ SDIST_FILE="$f"
+ break
+ fi
+ done
+ if [ -z "$SDIST_FILE" ]; then
+ echo "No sdist file found in sdist/" >&2
+ exit 1
+ fi
+ mkdir -p sdist_pkg
+ case "$SDIST_FILE" in
+ *.tar.gz) tar -xzf "$SDIST_FILE" -C sdist_pkg ;;
+ *.zip) unzip -q "$SDIST_FILE" -d sdist_pkg ;;
+ esac
+ PKGDIR="$(find sdist_pkg -mindepth 1 -maxdepth 1 -type d | head -n
1)"
+ if [ -z "$PKGDIR" ]; then
+ echo "Failed to locate extracted sdist directory" >&2
+ exit 1
+ fi
+ echo "PKGDIR=$PKGDIR" >> "$GITHUB_ENV"
+ - name: Build wheels
+ shell: bash
+ run: |
+ set -euxo pipefail
+ python -m pip install --upgrade pip
+ python -m pip install packaging cibuildwheel
+ mkdir -p python/dist
+ python -m cibuildwheel --output-dir python/dist "$PKGDIR"
+ - name: Store artifacts
+ uses: actions/upload-artifact@v4
+ with:
+ name: wheels-${{ matrix.os }}-${{ matrix.platform }}
+ path: python/dist/*
+
+ upload_test_pypi:
+ name: Publish to TestPyPI (auto)
+ needs: [build_wheels, build_sdist]
+ runs-on: ubuntu-22.04
+ if: github.event_name == 'push'
Review Comment:
The upload_test_pypi job will attempt to publish on every push to main, but
the version is automatically incremented by the build_sdist job. If multiple
commits are pushed to main in quick succession, or if the workflow is triggered
multiple times before completion, there could be race conditions or version
conflicts. Consider adding a check to prevent concurrent executions or
implementing a locking mechanism to ensure only one upload happens at a time.
##########
.github/workflows/python-wheel-workflow.yml:
##########
@@ -0,0 +1,282 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+on:
+ # Trigger the workflow on push or pull request,
+ # but only for the main branch
+ push:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/python-wheel-workflow.yml'
+ - '.github/scripts/update_version.py'
+ pull_request:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/ci.yml'
+ - '.github/workflows/python.yml'
+ workflow_dispatch:
+ inputs:
+ publish_pypi:
+ description: "Publish to PyPI (manual runs only)"
+ required: true
+ default: false
+ type: boolean
+
+jobs:
+ build_sdist:
+ name: Build source distribution
+ runs-on: ubuntu-22.04
+ steps:
+ - uses: actions/checkout@v3
+
+ - name: Set up Python
+ uses: actions/setup-python@v4
+ with:
+ python-version: "3.9"
+
+ - name: Install dependencies
+ run: |
+ python -m pip install --upgrade pip
+ pip install build twine
+ - name: update pyporject version
+ if: github.event_name != 'workflow_dispatch' || !inputs.publish_pypi
+ run: |
+ python .github/scripts/update_version.py
+ - name: Build sdist
+ run: |
+ # Bundle C++ sources into python/ so the sdist contains them.
+ rm -rf python/_bundled_cpp
+ cp -a cpp python/_bundled_cpp
+ cd python
+ python -m build --sdist
+ - name: Store artifacts
+ uses: actions/upload-artifact@v4
+ with:
+ name: sdist
+ path: python/dist/*
+
+ build_wheels:
+ name: Build wheels on ${{ matrix.runner }}
+ runs-on: ${{ matrix.runner }}
+ needs: build_sdist
+ strategy:
+ matrix:
+ include:
+ # Job 1: Native x86_64 build
+ - platform: x86_64
+ runner: ubuntu-latest # This is the standard x86_64 runner
+ os: linux
+ manylinux: _2_28
+ deployment-target: ''
+
+ # Job 2: Native aarch64 build
+ - platform: aarch64
+ runner: ubuntu-22.04-arm # This is a native ARM64 runner
+ os: linux
+ manylinux: _2_28
+ deployment-target: ''
+
+ # Job 3: macOS arm64 build
+ - platform: arm64
+ runner: macos-latest
+ os: macos
+ deployment-target: '11.0'
+
+
+ env:
+ CIBW_PLATFORM: ${{ matrix.os }}
+ CIBW_BUILD: "cp310-* cp311-* cp312-* cp313-*"
+ CIBW_SKIP: "*-musllinux_*"
+ # Pin arch to the matrix platform
+ CIBW_ARCHS: ${{ (matrix.os == 'linux' && matrix.platform) || (matrix.os
== 'macos' && matrix.platform) || (matrix.os == 'windows' && 'AMD64') || 'auto'
}}
+ CIBW_MANYLINUX_X86_64_IMAGE: ${{ matrix.os == 'linux' &&
format('manylinux{0}', matrix.manylinux) || '' }}
+ CIBW_MANYLINUX_AARCH64_IMAGE: ${{ matrix.os == 'linux' &&
format('manylinux{0}', matrix.manylinux) || '' }}
+ CIBW_ENVIRONMENT_WINDOWS: DISTUTILS_USE_SDK=1 MSSdk=1
Review Comment:
The CIBW_ENVIRONMENT_WINDOWS variable is set but there's no Windows job
defined in the build matrix. This environment variable will have no effect and
should be removed, or a Windows build job should be added to the matrix.
```suggestion
```
##########
python/pyproject.toml:
##########
@@ -16,30 +16,43 @@
# under the License.
[build-system]
-requires = ["scikit-build-core>=0.3.3", "pybind11", "ninja ~= 1.11"]
+requires = ["scikit-build-core>=0.8.3", "pybind11"]
build-backend = "scikit_build_core.build"
[project]
name = "graphar"
-version = "0.13.0"
-description = "GraphAr command line tool"
+version = "0.13.0-dev"
Review Comment:
The version string "0.13.0-dev" doesn't follow Python's version scheme
conventions. According to PEP 440, development releases should use ".dev" as a
suffix without a hyphen. Change this to "0.13.0.dev0" or similar to comply with
Python packaging standards.
```suggestion
version = "0.13.0.dev0"
```
##########
python/pyproject.toml:
##########
@@ -16,30 +16,43 @@
# under the License.
[build-system]
-requires = ["scikit-build-core>=0.3.3", "pybind11", "ninja ~= 1.11"]
+requires = ["scikit-build-core>=0.8.3", "pybind11"]
build-backend = "scikit_build_core.build"
[project]
name = "graphar"
-version = "0.13.0"
-description = "GraphAr command line tool"
+version = "0.13.0-dev"
+description = "An open source, standard data file format for graph data
storage and retrieval."
readme = "README.md"
authors = [{ name = "GraphAr community", email = "[email protected]" }]
requires-python = ">=3.9"
dependencies = ["typer ~= 0.1", "pydantic ~= 2.0, < 2.12", "pyyaml ~= 6.0",
"pytest ~= 7.2"]
+license = {text = "Apache-2.0"}
+classifiers = [
+ "License :: OSI Approved :: Apache Software License",
+ "Programming Language :: Python :: 3",
+ "Programming Language :: Python :: 3.9",
+ "Programming Language :: Python :: 3.10",
+ "Programming Language :: Python :: 3.11",
+ "Programming Language :: Python :: 3.12",
+ "Programming Language :: Python :: 3.13",
+]
-[tool.poetry.group.docs]
-optional = true
+[tool.scikit-build]
+build-dir = "build"
-[tool.poetry.group.docs.dependencies]
-pdoc = "*"
+[tool.scikit-build.sdist]
+include = [
+ "../cpp/**",
+ "../CMakeLists.txt",
Review Comment:
The sdist includes both "../cpp/**" and "../CMakeLists.txt", but the root
CMakeLists.txt file may not be necessary or appropriate for the Python package
build. Verify that the Python build actually uses the root CMakeLists.txt, or
if it only needs the cpp subdirectory contents. Including unnecessary files
increases the source distribution size.
```suggestion
```
##########
.github/workflows/python-wheel-workflow.yml:
##########
@@ -0,0 +1,282 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+on:
+ # Trigger the workflow on push or pull request,
+ # but only for the main branch
+ push:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/python-wheel-workflow.yml'
+ - '.github/scripts/update_version.py'
+ pull_request:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/ci.yml'
+ - '.github/workflows/python.yml'
+ workflow_dispatch:
+ inputs:
+ publish_pypi:
+ description: "Publish to PyPI (manual runs only)"
+ required: true
+ default: false
+ type: boolean
+
+jobs:
+ build_sdist:
+ name: Build source distribution
+ runs-on: ubuntu-22.04
+ steps:
+ - uses: actions/checkout@v3
+
+ - name: Set up Python
+ uses: actions/setup-python@v4
+ with:
+ python-version: "3.9"
+
+ - name: Install dependencies
+ run: |
+ python -m pip install --upgrade pip
+ pip install build twine
+ - name: update pyporject version
+ if: github.event_name != 'workflow_dispatch' || !inputs.publish_pypi
Review Comment:
The condition on line 65 uses 'inputs.publish_pypi' with a negation, but
this will fail when the workflow is not triggered via workflow_dispatch because
'inputs' will be null/undefined. The condition should be rewritten as: `if:
github.event_name != 'workflow_dispatch' || github.event.inputs.publish_pypi !=
'true'` to properly handle all trigger scenarios.
```suggestion
if: github.event_name != 'workflow_dispatch' ||
github.event.inputs.publish_pypi != 'true'
```
##########
.github/scripts/update_version.py:
##########
@@ -0,0 +1,82 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import json
+import re
+import sys
+import urllib.request
+from packaging.version import Version
+
+PACKAGE_NAME = "graphar"
+FILE_PATH = "python/pyproject.toml"
+
+def get_next_version():
+ versions = []
+ urls = [
+ f"https://pypi.org/pypi/{PACKAGE_NAME}/json";,
+ f"https://test.pypi.org/pypi/{PACKAGE_NAME}/json";
+ ]
+
+ print(f"Fetching versions for {PACKAGE_NAME}...")
+ for url in urls:
+ try:
+ with urllib.request.urlopen(url, timeout=5) as r:
+ data = json.load(r)
+ versions.extend(data.get("releases", {}).keys())
+ except Exception:
+ pass
Review Comment:
'except' clause does nothing but pass and there is no explanatory comment.
```suggestion
except Exception as e:
print(f"Warning: Failed to fetch versions from {url}: {e}",
file=sys.stderr)
```
##########
.github/workflows/python-wheel-workflow.yml:
##########
@@ -0,0 +1,282 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+on:
+ # Trigger the workflow on push or pull request,
+ # but only for the main branch
+ push:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/python-wheel-workflow.yml'
+ - '.github/scripts/update_version.py'
+ pull_request:
+ branches:
+ - "main"
+ paths:
+ - 'cpp/**'
+ - 'python/**'
+ - '.github/workflows/ci.yml'
+ - '.github/workflows/python.yml'
+ workflow_dispatch:
+ inputs:
+ publish_pypi:
+ description: "Publish to PyPI (manual runs only)"
+ required: true
+ default: false
+ type: boolean
+
+jobs:
+ build_sdist:
+ name: Build source distribution
+ runs-on: ubuntu-22.04
+ steps:
+ - uses: actions/checkout@v3
+
+ - name: Set up Python
+ uses: actions/setup-python@v4
+ with:
+ python-version: "3.9"
+
+ - name: Install dependencies
+ run: |
+ python -m pip install --upgrade pip
+ pip install build twine
+ - name: update pyporject version
Review Comment:
Spelling error: "pyporject" should be "pyproject".
```suggestion
- name: update pyproject version
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
