Skip to content

feat: add nightly test for neug package#76

Open
lnfjpt wants to merge 2 commits intoalibaba:mainfrom
lnfjpt:main
Open

feat: add nightly test for neug package#76
lnfjpt wants to merge 2 commits intoalibaba:mainfrom
lnfjpt:main

Conversation

@lnfjpt
Copy link
Collaborator

@lnfjpt lnfjpt commented Mar 18, 2026

Fixes #50

Greptile Summary

This PR adds a new GitHub Actions nightly workflow (.github/workflows/neug-nightly-test.yml) that installs the published neug PyPI package and runs a subset of Python tests across 3 OS platforms and 7 Python versions (3.8–3.14). It also fixes a bash array syntax bug in scripts/install_deps.sh by removing an erroneous comma from the INTERACTIVE_MACOS array.

Key findings:

  • Invalid branches filter on schedule: The branches: [main] key is nested under the - cron schedule item, which is not supported by GitHub Actions — schedule events do not accept branch filters and always run on the default branch. This key will be silently ignored and should be removed.
  • Version mismatch risk: The workflow installs neug from PyPI but runs test files taken directly from the checked-out source tree. If the local tests reference features not yet published to PyPI (or vice versa), test results will be misleading. The analogous neug-test.yml also installs requirements.txt before running tests — this step is missing here.
  • Unused matrix variables: platform-name and arch are declared in include blocks but never referenced in any step.
  • Shell array fix (correct): Removing the comma from INTERACTIVE_MACOS=("xsimd", "cmake") is a valid fix — bash array elements are space-separated, and the comma caused "xsimd," to be treated as a single element literal.

Confidence Score: 2/5

  • The workflow has two functional issues that undermine its reliability before it is merged.
  • The branches filter under schedule is invalid GitHub Actions syntax and will be silently dropped, meaning the intent to restrict execution is not achieved. More importantly, the mismatch between installing neug from PyPI and running tests from the local source tree means results may not accurately reflect whether the published package works correctly. These issues need to be addressed for the workflow to be trustworthy. The install_deps.sh fix is correct and safe on its own.
  • .github/workflows/neug-nightly-test.yml requires attention for the invalid schedule.branches syntax and the PyPI-vs-source test mismatch.

Important Files Changed

Filename Overview
.github/workflows/neug-nightly-test.yml New nightly CI workflow with two significant issues: branches filter under schedule is invalid GitHub Actions syntax and will be silently ignored; tests run local source files against the PyPI-installed package, creating a potential version mismatch. Unused matrix variables (platform-name, arch) and a missing trailing newline are also present.
scripts/install_deps.sh Correct bug fix: removes erroneous comma from the INTERACTIVE_MACOS bash array literal (("xsimd", "cmake")("xsimd" "cmake")), which previously caused "xsimd," (with trailing comma) to be treated as a single array element.

Sequence Diagram

sequenceDiagram
    participant S as Scheduler (cron 18:00 UTC)
    participant GH as GitHub Actions
    participant R as Runner (ubuntu/macos × 7 Python versions)
    participant PyPI as PyPI Registry
    participant WS as Workspace (git checkout)

    S->>GH: Trigger nightly workflow
    GH->>R: Spawn matrix job (3 OS × 7 Python = 21 jobs)
    R->>WS: actions/checkout@v4 (with submodules)
    R->>R: actions/setup-python@v5 (3.8–3.14)
    R->>PyPI: pip install neug
    PyPI-->>R: neug package (published version)
    R->>WS: cd tools/python_bind/
    R->>R: pip install -r requirements_dev.txt
    R->>WS: Read test files (test_batch_loading.py, test_alter_property.py)
    Note over R,WS: ⚠️ Tests are from local source but neug is from PyPI — potential version mismatch
    R->>R: python3 -m pytest tests/test_batch_loading.py
    R->>R: python3 -m pytest tests/test_alter_property.py
Loading

Last reviewed commit: "update"

Greptile also left 4 inline comments on this PR.

Comment on lines +6 to +7
branches:
- main
Copy link
Contributor

Choose a reason for hiding this comment

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

P1 Invalid branches key under schedule event

GitHub Actions' schedule trigger does not support a branches filter. The branches key is only valid under push, pull_request, pull_request_target, and workflow_run events. As written, the branches: [main] block is nested inside the - cron list item, which is structurally invalid for a schedule event and will be silently ignored by GitHub Actions. The scheduled workflow will always run on the repository's default branch — this key has no effect and should be removed to avoid confusion.

Suggested change
branches:
- main
- cron: '0 18 * * *'
workflow_dispatch:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. Removed the invalid branches filter under schedule trigger.

Comment on lines +52 to +62
- name: Install NeuG
run: |
pip install neug

- name: Run Python Test
run: |
cd ${GITHUB_WORKSPACE}/tools/python_bind/
pip install -r requirements_dev.txt
export FLEX_DATA_DIR=${GITHUB_WORKSPACE}/example_dataset/modern_graph
python3 -m pytest --exitfirst -sv tests/test_batch_loading.py
python3 -m pytest --exitfirst -sv tests/test_alter_property.py No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

P1 Version mismatch between installed package and local test files

The "Install NeuG" step installs neug from PyPI (pip install neug), but the "Run Python Test" step then runs test files directly from the checked-out repository source (${GITHUB_WORKSPACE}/tools/python_bind/tests/). This means the local test files may exercise APIs or behaviours that are not yet published to PyPI (or have already changed), making nightly test results potentially unreliable.

Additionally, comparing with neug-test.yml, that workflow installs both requirements.txt and requirements_dev.txt before running tests, while this workflow only installs requirements_dev.txt. Core runtime dependencies (protobuf==5.29.6, click, Flask, etc.) from requirements.txt should be pulled in transitively by pip install neug, but any version conflicts between the PyPI package's pinned deps and those in requirements.txt may go undetected.

Consider either:

  1. Installing neug from the local source (pip install -e .) and then running tests against it, or
  2. Also installing requirements.txt explicitly to mirror the standard test workflow setup:
    - name: Install NeuG
      run: |
        pip install neug

    - name: Run Python Test
      run: |
        cd ${GITHUB_WORKSPACE}/tools/python_bind/
        pip install -r requirements.txt
        pip install -r requirements_dev.txt
        export FLEX_DATA_DIR=${GITHUB_WORKSPACE}/example_dataset/modern_graph
        python3 -m pytest --exitfirst -sv tests/test_batch_loading.py
        python3 -m pytest --exitfirst -sv tests/test_alter_property.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is by design. The nightly test is specifically intended to verify that the published PyPI package works correctly with the latest test cases. The version mismatch between PyPI package and local test files is expected — if tests fail, it signals that the published package needs an update. The test files used here (test_batch_loading.py, test_alter_property.py) use only unittest (stdlib) and neug itself, so requirements_dev.txt is no longer needed.

Comment on lines +25 to +36
include:
- os: ubuntu-latest
platform-name: "linux-x86_64"
arch: "x86_64"

- os: ubuntu-24.04-arm
platform-name: "linux-arm64"
arch: "arm64"

- os: macos-14
platform-name: "macos-arm64"
arch: "arm64"
Copy link
Contributor

Choose a reason for hiding this comment

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

P2 Unused matrix variables platform-name and arch

The include entries define platform-name and arch for each OS, but neither variable is referenced anywhere in the workflow steps. If these are intended for use in a future step (e.g., as part of artifact naming or a download URL), they should be used; otherwise, they add noise and should be removed to keep the matrix clean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. Removed unused platform-name and arch variables from the matrix include blocks.

pip install -r requirements_dev.txt
export FLEX_DATA_DIR=${GITHUB_WORKSPACE}/example_dataset/modern_graph
python3 -m pytest --exitfirst -sv tests/test_batch_loading.py
python3 -m pytest --exitfirst -sv tests/test_alter_property.py No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

P2 Missing newline at end of file

The file is missing a trailing newline. POSIX requires text files to end with a newline, and many tools (e.g., git diff, linters) will flag this.

Suggested change
python3 -m pytest --exitfirst -sv tests/test_alter_property.py
python3 -m pytest --exitfirst -sv tests/test_alter_property.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. Added trailing newline at end of file.

@lnfjpt
Copy link
Collaborator Author

lnfjpt commented Mar 18, 2026

@lnfjpt lnfjpt changed the title feat: add nightly test for neug package (wip) feat: add nightly test for neug package Mar 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] We need a testing matrix to cover tests across all platforms

1 participant