PyTest Migration #144
PyTest Migration #144
Conversation
diaconuccalin
left a comment
diaconuccalin
left a comment
There was a problem hiding this comment.
Great feature, fantastically useful! :)
One more comment other than the ones I've already left, maybe add the instructions you included in the PR description to a dedicated .md file in the DeeployTest directory, with a mention of it in the big README, for better documentation for future users (including myself :) ).
| IMAGE="ghcr.io/pulp-platform/deeploy:main" | ||
| else | ||
| IMAGE="ghcr.io/pulp-platform/deeploy:devel" | ||
| IMAGE="ghcr.io/victor-jung/deeploy:pytest-migration" |
There was a problem hiding this comment.
It's only draft, but don't forget this hard-coded path :)
DeeployTest/test_generic.py
Outdated
| import pytest | ||
| from testUtils.pytestRunner import DeeployTestConfig, get_test_paths, get_worker_id, run_complete_test | ||
|
|
||
| KERNEL_TESTS = [ |
There was a problem hiding this comment.
I would move these lists in a dedicated file since they're only going to get bigger in time. I would keep the current file only as a test setup file.
There was a problem hiding this comment.
I think this makes sense, especially when I'll adapt the PR for several platforms. Then I would have a deeploytest.py (or test_suite.py) file where I declare the test functions with markers (what is currently test_generic.py) for every platform. On the side, I'll make a dedicated file per platform with the lists of test names.
…test, fix generic runner
…one (state contamination)
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis pull request migrates the test infrastructure from legacy Python test runners to a unified pytest-based system with marker-driven test selection. GitHub Actions workflows are refactored to accept pytest-markers as inputs instead of explicit test names. Multiple legacy testRunner scripts are deleted and replaced with pytest configuration, fixtures, and comprehensive platform-specific test suites. Changes
Sequence Diagram: Old vs. New Test Execution FlowsequenceDiagram
participant GHA as GitHub Actions
participant OldRunner as testRunner_*.py
participant NewRunner as pytest
participant Config as Marker Config
participant Result as Test Result
rect rgb(200, 150, 255)
Note over GHA,Result: OLD FLOW
GHA->>OldRunner: CLI args (test-names, simulators)
OldRunner->>OldRunner: Parse args, instantiate TestRunner
OldRunner->>OldRunner: per-test loop execution
OldRunner->>Result: Return result
end
rect rgb(150, 200, 255)
Note over GHA,Result: NEW FLOW
GHA->>Config: pytest-marker input
GHA->>NewRunner: pytest test_platforms.py -m "platform and marker"
NewRunner->>Config: Discover tests by marker
NewRunner->>NewRunner: Parameterized test execution (-n 4)
NewRunner->>Result: Aggregate and return results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
toolchain/banshee.patch (1)
1-61: Verify LTO pass manager removal impact; exact version pinning is a design trade-off, not a critical issue.The LLVM 15 upgrade (llvm-sys "120" → "=150") is properly handled: the removal of the
-opaque-pointers=0flag aligns with LLVM 15's opaque pointer default, and the FFI type change from&'static i8to&'static libc::c_charfollows best practices.However, the removal of
LLVMPassManagerBuilderPopulateLTOPassManagerinsrc/engine.rs(line 284) should be clarified. Verify that this change does not regress link-time optimization behavior, as the reason for removing this pass is not obvious from the diff.Regarding exact version pinning (
=prefix on all dependencies): while this prevents transitive security updates, it may be intentional for reproducible builds in a simulator/research context. If this strategy is intentional, document it; if not, consider the trade-offs between reproducibility and receiving transitive security patches for critical dependencies.
🤖 Fix all issues with AI agents
In @.github/workflows/_runner-siracusa-neureka-tiled.yml:
- Line 6: The workflow filename/definition uses the name string
"_runner-siracusa-neureka-tiled-sequential" which misleadingly implies
sequential test execution while the job runs tests with "-n 4" (parallel
pytest/xdist), so either rename the workflow string to reflect parallel
execution (e.g., remove "-sequential" or change to "-parallel") or update the
test invocation to run sequentially; locate the name field "name:
_runner-siracusa-neureka-tiled-sequential" and the test command that includes
"-n 4" and make them consistent so the workflow name accurately represents the
execution mode.
In @.github/workflows/_runner-siracusa.yml:
- Around line 17-20: The workflow input "test-type" is inconsistent with other
runners; rename the input to "pytest-marker" (replace occurrences of test-type
with pytest-marker), update its description to reflect marker-based selection,
and change the test invocation to use marker selection (e.g., use -m "siracusa
and ${{ inputs.pytest-marker }}" when running pytest) so the interface matches
_runner-softhier.yml and _runner-snitch.yml.
- Around line 45-47: The workflow step currently runs pytest with a direct test
function selector (`pytest test_platforms.py::test_siracusa_${{ inputs.test-type
}} -v -s`) and a misleading comment; change it to use marker-based filtering and
parallel execution like the other runners by invoking pytest with the marker
input (e.g., `-m "${{ inputs.pytest-marker }}"`) and `-n 4`, and update the
comment to accurately state "Run tests using pytest markers and xdist
parallelism"; specifically replace the direct selector
`test_platforms.py::test_siracusa_${{ inputs.test-type }}` with a marker
expression that includes `siracusa` plus the provided marker (`-m "siracusa and
${{ inputs.pytest-marker }}"`) and add `-n 4` to enable parallel runs.
In @.github/workflows/_select-env.yml:
- Line 37: The workflow currently hardcodes
IMAGE="ghcr.io/victor-jung/deeploy:pytest-migration", which uses a
contributor-owned container; change this to avoid trust/supply-chain risk by
removing the personal namespace and either defaulting to an organization-owned
image (e.g., set IMAGE to the official org image or tag) or by leaving IMAGE
unset and requiring explicit opt-in via an environment variable or workflow
input; update the IMAGE assignment in the workflow so it references a
maintainer-controlled image or an opt-in mechanism instead of
ghcr.io/victor-jung/deeploy:pytest-migration.
In @DeeployTest/test_platforms.py:
- Around line 289-321: The test_siracusa_tiled_kernels_l2_singlebuffer function
contains a duplicated create_test_config + run_and_assert_test block; remove the
second occurrence so the function only constructs the config and calls
run_and_assert_test once for the given test_params (delete the repeated
create_test_config(...) and subsequent run_and_assert_test(...) that duplicate
the earlier ones).
🧹 Nitpick comments (15)
pyproject.toml (1)
31-33: LGTM! Addition of pytest-xdist enables parallel test execution.The addition of
pytest-xdistaligns well with the PR's goal of parallel test execution. The reordering ofcoloredlogsis cosmetic and has no functional impact.Consider pinning versions for
pytestandpytest-xdistto ensure reproducible builds and avoid potential breaking changes in future releases, similar to howprotobufandonnx-graphsurgeonare pinned.📌 Suggested version pinning
'coloredlogs', -'pytest', -'pytest-xdist', +'pytest==8.3.4', +'pytest-xdist==3.6.1',.github/workflows/infra-generate-ccache.yml (1)
38-46: All pytest test paths are valid and exist in test_platforms.py.All nine hardcoded pytest node IDs correctly match the parametrized test functions and their data in the config files. The tiled test parameters are properly generated using the
generate_test_params()function, which constructs test IDs in the format{test_name}-{l1}-{config}.Consider using pytest markers (e.g.,
pytest -m ccache_build) to group these tests for more maintainable CI workflows, reducing the need to update hardcoded test node IDs if test names change.DeeployTest/test_mempool_config.py (1)
10-12: Prefer explicit typing/immutability for config constants.
Makes intent clearer and reduces accidental mutation.Proposed tweak
# Default number of threads for MemPool -DEFAULT_NUM_THREADS = 16 +DEFAULT_NUM_THREADS: int = 16.github/workflows/_runner-cortexm.yml (1)
39-45: Use-n autoinstead of hardcoded-n 4.
pytest-xdist is already a project dependency, so the-nflag is available. However, since this workflow accepts dynamic inputs for bothrunneranddocker-image, hardcoding-n 4is suboptimal. Use-n autoto automatically detect the available cores in the execution environment, whether running on GitHub-hosted or self-hosted runners.Suggested change
- pytest test_platforms.py -v -n 4 -m "cortexm and ${{ inputs.pytest-marker }}" + pytest test_platforms.py -v -n auto -m "cortexm and ${{ inputs.pytest-marker }}"DeeployTest/test_deeploy_internal.py (1)
1-31: Consider adding timeouts to subprocess calls.The subprocess calls throughout this file lack timeout parameters, which could cause tests to hang indefinitely if the underlying scripts malfunction or enter infinite loops. Consider adding a reasonable timeout (e.g., 60-300 seconds depending on expected test duration) to all
subprocess.runcalls.Example timeout addition
- result = subprocess.run(cmd, cwd = script_dir, capture_output = True, text = True) + result = subprocess.run(cmd, cwd = script_dir, capture_output = True, text = True, timeout = 300)Note: You may want to wrap subprocess calls in try-except blocks to catch
subprocess.TimeoutExpiredand provide informative error messages..github/workflows/_runner-generic.yml (1)
39-44: LGTM! Unified pytest invocation simplifies test execution.The single pytest command with marker filtering replaces the previous per-test loop approach. The
-n 4parallel execution aligns with the PR's goal of enabling parallel test runs.Consider adding a cache save step.
The ccache is restored on line 35 but there's no corresponding save step. Adding a save step after tests complete would preserve compilation artifacts for future runs.
💾 Suggested cache save step
Add after the Run Test step:
- name: Save ccache uses: actions/cache/save@v4 if: always() with: path: /app/.ccache key: ccache-ci-${{ github.sha }}DeeployTest/README.md (1)
5-5: Improve markdown formatting for better rendering.Several markdown formatting issues were detected:
- Line 5: Heading should be
##(h2) instead of###(h3) to follow proper heading hierarchy- Lines 8, 20: Code blocks showing command output should specify language (e.g.,
textorconsole)- Lines 47, 54: Shell command blocks should specify language (e.g.,
bashorshell)📝 Proposed markdown formatting fixes
Line 5 - Fix heading level:
-### Executing and Collecting Test Groups +## Executing and Collecting Test GroupsLine 8 - Add language specifier:
-``` +```text @pytest.mark.generic: mark test as a Generic platform testLine 20 - Add language specifier:
-``` +```text platform linux -- Python 3.10.0, pytest-9.0.2, pluggy-1.6.0 -- /usr/scratch/normandie/jungvi/micromamba/envs/deeploy/bin/python3.10Lines 47 and 54 - Add language specifier:
-``` +```bash pytest test_platforms.py -m "generic and kernels" -vvv --log-cli-level=DEBUGBased on learnings from static analysis tools.
Also applies to: 8-8, 20-20, 47-47, 54-54
DeeployTest/test_dmas.py (4)
47-80: Fixture cleanup logic could miss some dynamically generated types.The fixture only clears attributes containing
closure_argsormemcpyin the name. If other dynamically generated types are created (e.g., fromPointerClass), they won't be cleaned. Consider whether a more comprehensive cleanup is needed, or document that this is intentionally scoped to DMA-specific types.Additionally, the cleanup in
yieldblock (lines 72-80) catchesAttributeErrorbut this exception would only occur if another thread removed the attribute betweendir()anddelattr(). This is an unlikely race condition, but the try-except is a reasonable safeguard.
131-134: Consider addingstrict=Truetozip()for safety.As flagged by static analysis, using
strict=Trueensures the iterables have the same length, catching mismatches early.♻️ Suggested fix
- assert all(tileDim <= inDim for inDim, tileDim in zip(input_shape, tile_shape)), \ + assert all(tileDim <= inDim for inDim, tileDim in zip(input_shape, tile_shape, strict=True)), \ f'Tile shape {tile_shape} must be <= input shape {input_shape}'
228-272: Significant code duplication across DMA test functions.The three test functions (
test_mchan_dma,test_l3_dma,test_snitch_dma) share nearly identical structure for path setup, build directory logic, config creation, and simulation. Consider extracting common logic into a helper function.♻️ Example refactor - extract common test logic
def _run_dma_test( dma_type: str, test_shape: tuple, doublebuffer: bool, platform: str, num_cores: int, deeploy_test_dir: str, toolchain: str, toolchain_dir: str, cmake_args: tuple, skipgen: bool, skipsim: bool, ) -> None: """Common implementation for DMA tests.""" input_shape, tile_shape, node_count, data_type = test_shape test_name = f"test{dma_type}_{param_id_dma(test_shape)}_{param_id_dma(doublebuffer)}" gen_dir, _, test_name_clean = get_test_paths(f"test_dma_gen/{test_name}", platform, base_dir=deeploy_test_dir) if not skipgen: deployer, test_inputs, test_outputs = setup_dma_deployer( dma_type, input_shape, tile_shape, node_count, data_type, doublebuffer, gen_dir ) generateTestNetwork(deployer, [test_inputs], [test_outputs], gen_dir, _NoVerbosity) worker_id = get_worker_id() build_dir = str( Path(deeploy_test_dir) / f"TEST_{platform.upper()}" / (f"build_{worker_id}" if worker_id != "master" else "build_master") ) from testUtils.pytestRunner import DeeployTestConfig config = DeeployTestConfig( test_name=test_name_clean, test_dir=gen_dir, platform=platform, simulator='gvsoc', tiling=True, gen_dir=gen_dir, build_dir=build_dir, toolchain=toolchain, toolchain_install_dir=toolchain_dir, cmake_args=[*list(cmake_args), f"NUM_CORES={num_cores}"], ) configure_cmake(config) build_binary(config) if not skipsim: from testUtils.pytestRunner import run_simulation result = run_simulation(config) assert result.success, f"{dma_type} test failed with {result.error_count} errors" assert result.error_count == 0, f"Found {result.error_count} errors"
251-252: Imports inside test functions are unconventional.Moving
from testUtils.pytestRunner import DeeployTestConfigandrun_simulationto the top of the file would be more idiomatic and slightly improve test startup time.Also applies to: 301-302, 351-352
Deeploy/TilingExtension/TilerExtension.py (1)
256-311: Minimalloc file paths are now instance-specific.The changes correctly use
self._minimalloc_inputandself._minimalloc_outputinstead of hardcoded filenames, enabling parallel test execution.Consider whether these temporary files should be cleaned up after successful test runs to avoid accumulating test artifacts. This could be added as an optional cleanup step.
DeeployTest/testUtils/pytestRunner.py (2)
17-41: Type hint inconsistency for mutable defaults.The type hints for
cmake_argsandgen_argsareList[str]but they default toNone. Consider usingOptional[List[str]]for accuracy:- cmake_args: List[str] = None - gen_args: List[str] = None + cmake_args: Optional[List[str]] = None + gen_args: Optional[List[str]] = None
291-321: Consider: Sentinel values for parse failure.When the output parsing fails (lines 302-306),
error_countandtotal_countare set to-1. This works as a sentinel value but could be confusing since these are typically non-negative counts. An alternative would be usingNonewithOptional[int]types, but the current approach is functional for the use case.DeeployTest/test_platforms.py (1)
335-335: Prefix unused variable with underscore.The
config_namevalue is only used for test ID generation inparam_idand is not used within the test function body. Consider using_config_nameor just_to indicate it's intentionally unused:- test_name, l1, config_name = test_params + test_name, l1, _config_name = test_paramsThis applies to all tiled test functions that unpack this tuple.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
.github/workflows/_runner-chimera.yml.github/workflows/_runner-cortexm.yml.github/workflows/_runner-generic.yml.github/workflows/_runner-mempool.yml.github/workflows/_runner-siracusa-neureka-tiled-sequential.yml.github/workflows/_runner-siracusa-neureka-tiled.yml.github/workflows/_runner-siracusa-tiled-sequential.yml.github/workflows/_runner-siracusa-tiled.yml.github/workflows/_runner-siracusa.yml.github/workflows/_runner-snitch-tiled-sequential.yml.github/workflows/_runner-snitch.yml.github/workflows/_runner-softhier.yml.github/workflows/_select-env.yml.github/workflows/ci-deeploy-testing.yml.github/workflows/ci-deeploy.yml.github/workflows/ci-platform-chimera.yml.github/workflows/ci-platform-cortexm.yml.github/workflows/ci-platform-generic.yml.github/workflows/ci-platform-mempool.yml.github/workflows/ci-platform-siracusa-neureka-tiled.yml.github/workflows/ci-platform-siracusa-tiled.yml.github/workflows/ci-platform-siracusa.yml.github/workflows/ci-platform-snitch-tiled.yml.github/workflows/ci-platform-snitch.yml.github/workflows/ci-platform-softhier.yml.github/workflows/infra-generate-ccache.ymlContainer/Dockerfile.deeployDeeploy/TilingExtension/TilerExtension.pyDeeployTest/README.mdDeeployTest/conftest.pyDeeployTest/pytest.iniDeeployTest/testMVP.pyDeeployTest/testRunner_chimera.pyDeeployTest/testRunner_cortexm.pyDeeployTest/testRunner_generic.pyDeeployTest/testRunner_mempool.pyDeeployTest/testRunner_siracusa.pyDeeployTest/testRunner_siracusa_l3dma.pyDeeployTest/testRunner_siracusa_mchandma.pyDeeployTest/testRunner_snitch.pyDeeployTest/testRunner_snitch_dma.pyDeeployTest/testRunner_softhier.pyDeeployTest/testRunner_tiled_siracusa.pyDeeployTest/testRunner_tiled_siracusa_w_neureka.pyDeeployTest/testRunner_tiled_snitch.pyDeeployTest/testUtils/pytestRunner.pyDeeployTest/test_chimera_config.pyDeeployTest/test_cortexm_config.pyDeeployTest/test_deeploy_internal.pyDeeployTest/test_dmas.pyDeeployTest/test_generic_config.pyDeeployTest/test_mempool_config.pyDeeployTest/test_platforms.pyDeeployTest/test_siracusa_config.pyDeeployTest/test_siracusa_neureka_tiled_config.pyDeeployTest/test_siracusa_tiled_config.pyDeeployTest/test_snitch_config.pyDeeployTest/test_snitch_tiled_config.pyDeeployTest/test_softhier_config.pypyproject.tomlscripts/generate_test_matrix.pytoolchain/banshee.patch
💤 Files with no reviewable changes (16)
- .github/workflows/_runner-siracusa-neureka-tiled-sequential.yml
- DeeployTest/testRunner_cortexm.py
- DeeployTest/testRunner_tiled_snitch.py
- .github/workflows/ci-deeploy-testing.yml
- DeeployTest/testRunner_softhier.py
- DeeployTest/testRunner_tiled_siracusa_w_neureka.py
- DeeployTest/testRunner_siracusa.py
- DeeployTest/testRunner_snitch_dma.py
- .github/workflows/_runner-siracusa-tiled-sequential.yml
- DeeployTest/testRunner_siracusa_l3dma.py
- DeeployTest/testRunner_tiled_siracusa.py
- DeeployTest/testRunner_generic.py
- DeeployTest/testRunner_mempool.py
- DeeployTest/testRunner_chimera.py
- DeeployTest/testRunner_snitch.py
- DeeployTest/testRunner_siracusa_mchandma.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-24T11:43:47.236Z
Learnt from: diaconuccalin
Repo: pulp-platform/Deeploy PR: 117
File: .github/workflows/ci-platform-siracusa.yml:57-60
Timestamp: 2025-09-24T11:43:47.236Z
Learning: In the Deeploy test system, test names in CI workflows correspond to directory names under DeeployTest/Tests/, not Python function names. The TestRunner class executes tests by passing directory paths via the `-t` argument, where each directory contains test configurations and definitions.
Applied to files:
.github/workflows/ci-deeploy.yml.github/workflows/_runner-generic.yml.github/workflows/_runner-chimera.yml.github/workflows/_runner-siracusa-tiled.yml.github/workflows/_runner-siracusa.yml.github/workflows/_runner-snitch.yml.github/workflows/_runner-cortexm.ymlDeeployTest/pytest.ini.github/workflows/ci-platform-cortexm.yml.github/workflows/_runner-snitch-tiled-sequential.ymlDeeployTest/README.md.github/workflows/_runner-softhier.ymlDeeployTest/testMVP.pyDeeployTest/conftest.py.github/workflows/_runner-mempool.yml.github/workflows/ci-platform-chimera.yml.github/workflows/ci-platform-snitch.yml.github/workflows/ci-platform-mempool.yml.github/workflows/ci-platform-softhier.ymlDeeployTest/testUtils/pytestRunner.py.github/workflows/_runner-siracusa-neureka-tiled.ymlDeeployTest/test_deeploy_internal.py.github/workflows/infra-generate-ccache.yml
📚 Learning: 2025-09-09T15:58:06.454Z
Learnt from: Xeratec
Repo: pulp-platform/Deeploy PR: 105
File: Deeploy/Targets/PULPOpen/DMA/MchanDma.py:61-64
Timestamp: 2025-09-09T15:58:06.454Z
Learning: The _legalizeTransfers function in TilingCodeGeneration.py handles conversion from elements to bytes for DMA operations when isFinalMemoryLevel is true, eliminating the need for individual DMA implementations like MchanDma to perform this conversion manually.
Applied to files:
DeeployTest/test_dmas.py
📚 Learning: 2025-09-09T15:58:06.454Z
Learnt from: Xeratec
Repo: pulp-platform/Deeploy PR: 105
File: Deeploy/Targets/PULPOpen/DMA/MchanDma.py:61-64
Timestamp: 2025-09-09T15:58:06.454Z
Learning: The _legalizeTransfers function in TilingCodeGeneration.py handles conversion from elements to bytes for DMA operations when isFinalMemoryLevel is true, eliminating the need for individual DMA implementations like MchanDma to perform this conversion.
Applied to files:
DeeployTest/test_dmas.py
🧬 Code graph analysis (4)
DeeployTest/testMVP.py (2)
Deeploy/TilingExtension/TilerExtension.py (1)
TilerDeployerWrapper(962-1045)DeeployTest/testUtils/tilingUtils.py (3)
DBOnlyL3Tiler(14-26)DBTiler(29-38)SBTiler(41-45)
DeeployTest/test_dmas.py (10)
DeeployTest/testUtils/codeGenerate.py (1)
generateTestNetwork(259-289)DeeployTest/testUtils/dmaUtils.py (6)
MemcpyLayer(103-104)MemcpyParser(86-100)MemcpyTileConstraint(60-83)MemcpyTypeChecker(40-57)generate_graph(107-118)prepare_deployer_with_custom_tiling(358-377)DeeployTest/testUtils/pytestRunner.py (2)
get_test_paths(54-89)get_worker_id(353-360)Deeploy/AbstractDataTypes.py (1)
PointerClass(536-559)Deeploy/Targets/PULPOpen/DMA/L3Dma.py (1)
L3Dma(27-60)Deeploy/Targets/PULPOpen/DMA/MchanDma.py (1)
MchanDma(28-79)Deeploy/Targets/Snitch/CodeTransformationPasses/SnitchClusterSynch.py (1)
SnitchSynchCoresPass(15-24)Deeploy/Targets/Snitch/CodeTransformationPasses/SnitchCoreFilter.py (1)
SnitchCoreFilterPass(11-24)Deeploy/Targets/Snitch/CodeTransformationPasses/SnitchProfileExecutionBlock.py (1)
SnitchProfileExecutionBlockPass(22-31)Deeploy/TilingExtension/TilerExtension.py (1)
TilingReadyNodeBindings(1048-1056)
DeeployTest/testUtils/pytestRunner.py (3)
DeeployTest/conftest.py (6)
toolchain(135-137)cmake_args(141-143)skipgen(123-125)skipsim(129-131)deeploy_test_dir(86-88)toolchain_dir(98-103)DeeployTest/testUtils/testRunner.py (1)
run(325-336)Deeploy/DeeployTypes.py (1)
copy(1016-1020)
DeeployTest/test_deeploy_internal.py (1)
DeeployTest/testUtils/testRunner.py (1)
run(325-336)
🪛 markdownlint-cli2 (0.18.1)
DeeployTest/README.md
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
DeeployTest/testMVP.py
121-121: Probable use of insecure hash functions in hashlib: md5
(S324)
DeeployTest/test_dmas.py
11-11: Docstring contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF002)
11-11: Docstring contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF002)
133-133: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
150-150: Avoid specifying long messages outside the exception class
(TRY003)
262-262: Consider [*list(cmake_args), "NUM_CORES=8"] instead of concatenation
Replace with [*list(cmake_args), "NUM_CORES=8"]
(RUF005)
312-312: Consider [*list(cmake_args), "NUM_CORES=8"] instead of concatenation
Replace with [*list(cmake_args), "NUM_CORES=8"]
(RUF005)
362-362: Consider [*list(cmake_args), "NUM_CORES=9"] instead of concatenation
Replace with [*list(cmake_args), "NUM_CORES=9"]
(RUF005)
DeeployTest/testUtils/pytestRunner.py
136-136: subprocess call: check for execution of untrusted input
(S603)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
196-196: subprocess call: check for execution of untrusted input
(S603)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
227-227: subprocess call: check for execution of untrusted input
(S603)
231-231: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Avoid specifying long messages outside the exception class
(TRY003)
283-283: subprocess call: check for execution of untrusted input
(S603)
DeeployTest/test_deeploy_internal.py
26-26: subprocess call: check for execution of untrusted input
(S603)
45-45: subprocess call: check for execution of untrusted input
(S603)
70-70: subprocess call: check for execution of untrusted input
(S603)
92-92: subprocess call: check for execution of untrusted input
(S603)
114-114: subprocess call: check for execution of untrusted input
(S603)
136-136: subprocess call: check for execution of untrusted input
(S603)
164-164: subprocess call: check for execution of untrusted input
(S603)
190-190: subprocess call: check for execution of untrusted input
(S603)
216-216: subprocess call: check for execution of untrusted input
(S603)
230-230: subprocess call: check for execution of untrusted input
(S603)
252-252: subprocess call: check for execution of untrusted input
(S603)
269-269: subprocess call: check for execution of untrusted input
(S603)
283-283: subprocess call: check for execution of untrusted input
(S603)
314-314: subprocess call: check for execution of untrusted input
(S603)
341-341: subprocess call: check for execution of untrusted input
(S603)
368-368: subprocess call: check for execution of untrusted input
(S603)
DeeployTest/test_platforms.py
207-207: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
229-229: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
291-291: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
335-335: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
364-364: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
393-393: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
422-422: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
451-451: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
494-494: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
516-516: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
542-542: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
545-545: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
575-575: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
604-604: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
633-633: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
662-662: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
692-692: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
723-723: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: siracusa-kernels-tiled-l2-doublebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l2-doublebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-kernels-tiled-l2-singlebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l3-doublebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l3-singlebuffer / test-runner-siracusa-tiled
- GitHub Check: siracusa-models-tiled-l2-singlebuffer / test-runner-siracusa-tiled
🔇 Additional comments (69)
toolchain/banshee.patch (3)
88-103: LGTM: Proper FFI type usage.The change from
&'static i8to&'static libc::c_char(line 100) is a best practice for FFI code. Thec_chartype is portable across platforms where C'scharmay be signed or unsigned, making this more robust than assumingi8.This correctly utilizes the
libccrate added in the Cargo.toml dependencies.
76-87: No action required—this is a necessary API migration for LLVM 15.The removal of
LLVMPassManagerBuilderPopulateLTOPassManageris mandatory, not discretionary. The legacy PassManagerBuilder API (including the LTO pass manager function) is being removed in LLVM 15 across the ecosystem. The patch updatesllvm-sysfrom version 120 to 150, reflecting this required upgrade. The remaining pass managers (LLVMPassManagerBuilderPopulateFunctionPassManagerandLLVMPassManagerBuilderPopulateModulePassManager) handle optimization appropriately for LLVM 15.
62-75: Removal of-Cllvm-args=-opaque-pointers=0is correct for LLVM 15.This patch upgrades Banshee's
llvm-sysdependency from version 120 (LLVM 12) to 150 (LLVM 15). In LLVM 15+, opaque pointers are the default and mandatory in later versions, making the flag that explicitly disables them unnecessary and deprecated. The removal aligns with the LLVM upgrade and removes technical debt.Container/Dockerfile.deeploy (1)
89-112: Consider scoping the NVIDIA pip index to only the installs that need it (reliability + supply-chain surface).The URL
https://pypi.ngc.nvidia.comis correct per NVIDIA documentation and does not require a/simplesuffix. However, writing a global/etc/pip.confmeans every laterpip installwill query the NVIDIA index—including unrelated installs liketoml-to-requirements(line 109). If that endpoint is slow or unavailable, builds fail unnecessarily, and you widen the dependency-resolution surface for packages that don't need NVIDIA packages.Consider scoping the index via environment variable or command-line flag on only the relevant install lines:
RUN pip install --extra-index-url https://pypi.ngc.nvidia.com -r requirements.txtLikely an incorrect or invalid review comment.
.github/workflows/ci-platform-softhier.yml (1)
38-38: No action needed — the "kernels" marker is properly defined.The marker is registered in
DeeployTest/conftest.pyvia thepytest_configurehook with the description "mark test as a kernel test (individual operators)". The workflow change is correct and will work as intended..github/workflows/_runner-mempool.yml (2)
39-45: Marker filter approach is solid; consider-n auto(optional) and verify xdist availability.#!/bin/bash set -euo pipefail # Confirm marker expression appears as intended and isn't accidentally quoted/escaped. rg -n --hidden -S 'pytest\s+test_platforms\.py.*-m\s+"mempool and' .github/workflows/_runner-mempool.yml
17-19: All callers have been properly updated to usepytest-marker.Verification confirms both invocations of
_runner-mempool.ymlinci-platform-mempool.yml(lines 38 and 46) correctly pass thepytest-markerparameter, and notest-namesreferences remain in the workflows..github/workflows/_runner-cortexm.yml (1)
17-19: All callers have been properly updated to usepytest-markerinstead oftest-names. Both workflow jobs (cortexm-kernelsat line 34 andcortexm-modelsat line 42 inci-platform-cortexm.yml) pass the correct input parameter with appropriate values..github/workflows/ci-platform-mempool.yml (1)
38-38: Marker-based selection is properly implemented.
Thekernelsandmodelsmarkers are explicitly registered inDeeployTest/conftest.pyand actively used throughoutDeeployTest/test_platforms.py, correctly selecting individual operator tests and full network tests respectively.DeeployTest/test_mempool_config.py (1)
13-34: No issues found. All 22 test paths inKERNEL_TESTSandMODEL_TESTScorrectly map to existing directories underDeeployTest/Tests/, and the config module is actively used bytest_platforms.py. The test path contract is sound.Likely an incorrect or invalid review comment.
.github/workflows/ci-platform-siracusa.yml (1)
38-38: LGTM! Clean simplification of test selection.The migration from explicit test-names lists to categorical test-type values ("kernels", "models") simplifies the workflow and aligns well with the marker-based test selection strategy.
Also applies to: 46-46
.github/workflows/_runner-softhier.yml (1)
17-19: LGTM! Well-structured pytest migration.The migration to pytest with marker-based selection is clean:
- Parallel execution with 4 workers leverages available cores
- CCACHE setup is appropriate
- Marker expression allows flexible test filtering
Also applies to: 34-40
.github/workflows/_runner-snitch.yml (1)
17-19: LGTM! Consistent pytest migration.The migration follows the same clean pattern as other platform runners, with marker-based filtering and parallel execution.
Also applies to: 39-44
DeeployTest/test_snitch_config.py (1)
24-24: No action needed. The empty MODEL_TESTS is intentional and consistent with other platforms.Three platforms have empty MODEL_TESTS configurations (softhier, snitch, chimera), while others have populated model test lists. This is a valid configuration state reflecting that model tests for these platforms are not yet implemented.
.github/workflows/ci-platform-snitch-tiled.yml (1)
32-38: LGTM! Workflow successfully migrated to marker-based test selection.The replacement of explicit test configurations with a single pytest-marker simplifies the workflow and aligns with the PR's goal of centralizing test logic in pytest.
DeeployTest/pytest.ini (2)
20-24: Good use of strict pytest options.The
--strict-markersand--strict-configflags will help catch typos in marker names and configuration issues early.
34-38: This configuration is appropriate for the project's dependency landscape.The filterwarnings configuration treats all warnings as errors but excludes
DeprecationWarningandPendingDeprecationWarning. This is a pragmatic choice for a project with external dependencies like protobuf, numpy, onnx, and onnxruntime—packages that frequently emit deprecation warnings as they evolve. Code inspection found no deprecated patterns in Deeploy's own codebase, so the filter is protecting against upstream package deprecations rather than masking internal issues..github/workflows/_runner-siracusa-tiled.yml (2)
17-19: LGTM! Input simplified to marker-based selection.The workflow now accepts a single pytest-marker input instead of per-test configurations, aligning with the centralized pytest approach.
34-37: LGTM! Test execution properly combines markers.The pytest invocation correctly combines the platform marker (
siracusa_tiled) with the input marker, enabling flexible test selection while maintaining platform-specific filtering.DeeployTest/testMVP.py (2)
119-122: LGTM! Appropriate use of MD5 for test isolation.The MD5 hash generates a unique identifier for minimalloc IO files to prevent conflicts during parallel test execution. While static analysis flags MD5 as insecure, this is a non-cryptographic use case (generating unique file identifiers) where MD5 is perfectly acceptable.
123-131: LGTM! Proper test isolation for parallel execution.Passing
testNameandworkDirtoTilerDeployerWrapperenables per-test workspace isolation, which is essential for parallel pytest execution with pytest-xdist. This ensures that concurrent tests don't interfere with each other's minimalloc files.DeeployTest/test_siracusa_config.py (2)
5-7: LGTM! Clear platform metadata.The platform-specific constants (
PLATFORM_NAME,SIMULATOR,DEFAULT_CORES) provide clear configuration for Siracusa tests and align with the centralized test configuration approach.
9-80: LGTM! Comprehensive test coverage.The
KERNEL_TESTSandMODEL_TESTSlists provide comprehensive coverage across FP32, Integer, and Mixed kernels, as well as various models. This centralization makes test management more maintainable than the previous directory-based approach..github/workflows/_runner-snitch-tiled-sequential.yml (2)
17-19: LGTM! Simplified input scheme.The migration to a single
pytest-markerinput is cleaner than the previous multi-parameter approach, enabling flexible test selection through marker composition.
34-39: LGTM! Efficient parallel test execution.The parallel execution strategy with ccache is well-configured. The marker composition
"snitch_tiled and ${{ inputs.pytest-marker }}"provides flexible test filtering..github/workflows/_runner-siracusa-neureka-tiled.yml (1)
34-40: LGTM! Consistent with the unified pytest migration.The test execution follows the same clean pattern as other migrated workflows, with proper ccache setup and marker-based filtering.
.github/workflows/ci-platform-siracusa-neureka-tiled.yml (1)
32-78: LGTM! Well-structured marker-based test matrix.The pytest markers effectively compose multiple test dimensions (kernels/models, buffer strategy, memory level, wmem variant) into a clear test matrix. This approach is more maintainable than the previous explicit test configurations.
.github/workflows/ci-platform-siracusa-tiled.yml (1)
32-84: LGTM! Improved documentation and consistent structure.The addition of descriptive comments for each job and the systematic naming convention (
siracusa-{type}-tiled-{memory}-{buffer}) significantly improve workflow readability. The marker composition clearly maps to each job's purpose.DeeployTest/test_deeploy_internal.py (4)
52-142: LGTM! Comprehensive memory allocation testing.The test class effectively covers both MiniMalloc and TetrisRandom strategies with positive and negative test cases. The use of
--shouldFailflag to verify correct failure behavior is a good testing practice.
144-221: LGTM! Thorough tiler extension testing.The parameterized tests provide comprehensive coverage across multiple test paths and scenarios (basic, constrained, double-buffer). The test structure is clean and maintainable.
223-274: LGTM! Well-organized debug and type testing.The tests appropriately cover type system validation and debug transformations across relevant platforms. The separation between standalone and class-based tests is logical.
290-372: LGTM! Complete type inference test coverage.The test class effectively validates type inference behavior with both failure and success scenarios, using clear, descriptive test names and appropriate input configurations.
Note on static analysis warnings: The Ruff S603 warnings about subprocess execution are false positives in this context. The commands are constructed from known, trusted paths and literals within the test suite, not from external user input.
DeeployTest/test_cortexm_config.py (1)
1-23: LGTM! Clean configuration structure.The static test path lists are well-organized and align with the marker-based test selection strategy introduced in this PR. The configuration will be consumed by the unified platform test harness in
test_platforms.py..github/workflows/ci-platform-chimera.yml (1)
38-38: LGTM! Marker-based test selection simplifies CI.The migration from explicit test-names to pytest-marker input streamlines test selection and aligns with the unified pytest approach across all platform workflows.
.github/workflows/ci-platform-snitch.yml (1)
38-38: LGTM! Consistent marker-based approach.The pytest-marker input aligns with the unified test selection strategy applied across all platform workflows.
.github/workflows/_runner-generic.yml (1)
17-19: LGTM! Input renamed for marker-based selection.The pytest-marker input clearly reflects the new marker-based test selection approach.
.github/workflows/ci-deeploy.yml (2)
46-46: LGTM! Job name accurately reflects internal test scope.The rename from
deeploy-memory-allocationtodeeploy-internal-testsbetter represents the broader internal test coverage enabled by the pytest migration.
59-63: LGTM! Marker-based internal test execution.The pytest invocation with the
deeploy_internalmarker aligns with the unified marker-based test selection strategy across the PR.DeeployTest/test_chimera_config.py (1)
1-13: LGTM! Clean configuration structure.The file follows a clear pattern for platform test configuration, and the comment acknowledges the current minimal test coverage.
.github/workflows/_runner-chimera.yml (1)
39-45: Excellent simplification with marker-based test execution.The migration to a single pytest invocation with marker-based filtering is much cleaner than the previous multi-parameter approach. The comment explaining the parallelization choice (4 threads matching VM cores) is helpful.
DeeployTest/test_generic_config.py (1)
1-90: Excellent organization and comprehensive test coverage.The configuration is well-structured with clear categorization (FP32, Integer, Mixed) and inline comments. The comprehensive test lists for both kernels and models make the Generic platform test coverage explicit and maintainable.
.github/workflows/ci-platform-generic.yml (1)
32-46: Clean migration to marker-based test selection.The transition from explicit test-name lists to pytest markers (
kernels,models) simplifies CI configuration and aligns with the centralized test infrastructure. This approach makes it easier to add or remove tests without modifying workflows..github/workflows/ci-platform-cortexm.yml (1)
32-46: Consistent marker-based test selection applied to Cortex-M.The workflow correctly mirrors the generic platform pattern, using pytest markers for test selection.
scripts/generate_test_matrix.py (2)
23-52: Script implementation looks correct.The CLI argument handling, config mapping, and JSON output are well-structured. Minor suggestion: consider using
argparsefor more robust argument parsing if the script grows in complexity.
16-20: Confirm the intended scope for test matrix generation.The script currently only imports from
test_siracusa_tiled_config, while other platform-specific tiled configurations exist (test_snitch_tiled_config.py,test_siracusa_neureka_tiled_config.py) and are actively used elsewhere in the codebase (e.g.,DeeployTest/test_platforms.py). Clarify whether this script should support multiple platforms or if platform-specific matrix generators are preferred.DeeployTest/test_snitch_tiled_config.py (1)
1-28: Configuration file structure looks good.The format is consistent with other config files in the PR. Empty dictionaries serve as clear placeholders for future test configurations.
Deeploy/TilingExtension/TilerExtension.py (2)
55-84: Good addition of test isolation for minimalloc files.The
testNameandworkDirparameters enable parallel test execution without file conflicts. The path sanitization (replacing/and\) is appropriate for creating safe filenames.One minor consideration: the sanitization doesn't handle other potentially problematic characters (e.g.,
:,*,?,",<,>,|on Windows). If cross-platform compatibility is a concern, consider using a more comprehensive sanitization.
964-972: Wrapper correctly propagates new parameters.The
TilerDeployerWrapperproperly passestestNameandworkDirto the underlyingTilerinstance.DeeployTest/test_siracusa_neureka_tiled_config.py (2)
1-57: Configuration file structure is well-organized.The file provides comprehensive test configurations for the Siracusa Neureka platform with clear separation between buffer modes and memory levels.
28-35: No action needed. The presence of"Kernels/Integer/Attention"inL3_SINGLEBUFFER_MODELSis intentional and consistent across all config files (test_siracusa_tiled_config.py,test_siracusa_neureka_tiled_config.py, andtest_siracusa_config.py). This naming pattern reflects that certain kernel operators are tested with L3 memory configurations, and the test data includes a full neural network (network.onnx), justifying its classification as a model-like test despite its Kernels/ path.DeeployTest/test_dmas.py (1)
355-357: The code is correct.gvsocis the appropriate default simulator for Snitch DMA tests. Bothtest_snitch_config.pyandtest_snitch_tiled_config.pylistgvsocas the primary supported simulator, and all DMA test functions intest_dmas.py(MchanDma, L3Dma, and SnitchDma) consistently usesimulator='gvsoc', confirming this is the standard choice across platforms.DeeployTest/test_siracusa_tiled_config.py (3)
1-10: LGTM! Platform constants are well-defined.The configuration constants for the Siracusa tiled platform are clear and appropriately scoped.
12-102: LGTM! Kernel dictionaries are well-structured.The kernel test configurations with varying L1 memory sizes provide good coverage for testing different memory pressure scenarios.
104-117: Verify: Kernel path in models dictionary.
Kernels/Integer/Attentionis placed inL2_SINGLEBUFFER_MODELS(and similarly in other model dictionaries). This path starts withKernels/which suggests it might be a kernel test rather than a model test. Is this intentional, or should it be in the kernel dictionaries instead?DeeployTest/conftest.py (4)
15-46: LGTM! CLI options are well-designed.The pytest CLI options provide good flexibility for test configuration with sensible defaults.
49-82: LGTM! Marker registration and logging configuration are well-implemented.The comprehensive marker set enables flexible test selection, and the verbosity-based logging is appropriately configured.
106-119: Consider: Consistent return type forccache_dir.The fixture returns
Pathwhen reading fromCCACHE_DIRenvironment variable (line 111) but also returnsPathfor the fallback (line 117), which is consistent. However, returningNonewhen ccache is unavailable means callers must handle this case.Since this is
autouse=True, the primary purpose is settingCCACHE_DIRin the environment rather than returning a value—the return value is rarely used. This is acceptable.
122-143: LGTM! Simple fixtures for CLI option access.These fixtures provide clean access to CLI options for test functions.
DeeployTest/testUtils/pytestRunner.py (7)
54-89: LGTM! Path resolution logic is correct.The function properly handles both relative and absolute paths, with appropriate fallback and warning for paths outside the base directory.
92-141: LGTM! Network generation function is well-implemented.The subprocess call is appropriate for a test runner in a controlled CI environment. The command construction logic is correct.
143-200: LGTM! CMake configuration function is robust.The function properly validates prerequisites, handles environment variables, and constructs the CMake command with appropriate platform-specific flags.
203-231: LGTM! Build function is straightforward.
324-350: LGTM! Test orchestration function is well-structured.The function cleanly orchestrates the complete test flow with appropriate logging and skip handling.
363-466: LGTM! Comprehensive test configuration factory.The function handles the complexity of different platform configurations well. The worker-based build directories properly support parallel test execution.
One minor suggestion: the magic number
1024000on line 435 could referenceDEFAULT_L2from the config for consistency.
469-488: LGTM! Test assertion helper is correct.The guard
error_count >= 0properly handles the sentinel value case, and the assertions provide clear failure messages.DeeployTest/test_platforms.py (4)
38-59: LGTM! Helper functions are clean and well-designed.
generate_test_paramsandparam_idprovide good abstractions for generating parametrized test cases.
62-103: LGTM! PLATFORM_CONFIGS provides a clean configuration registry.Centralizing platform configurations reduces duplication and makes it easier to maintain consistency across tests.
128-161: LGTM! Generic platform tests are well-structured.The test functions properly use
PLATFORM_CONFIGSand apply appropriate markers for test selection.
680-708: LGTM! Neureka wmem tests properly pass generator arguments.The
gen_args=["--neureka-wmem"]parameter correctly enables Neureka weight memory for these test variants.
|
|
||
| --- | ||
| name: _runner-siracusa-neureka-tiled | ||
| name: _runner-siracusa-neureka-tiled-sequential |
There was a problem hiding this comment.
Potentially misleading workflow name.
The workflow is named _runner-siracusa-neureka-tiled-sequential, but the test execution uses -n 4 for parallel execution (line 39). The "-sequential" suffix may confuse users about the execution mode. Consider clarifying whether this refers to job-level sequencing or renaming to reflect the parallel test execution.
🤖 Prompt for AI Agents
In @.github/workflows/_runner-siracusa-neureka-tiled.yml at line 6, The workflow
filename/definition uses the name string
"_runner-siracusa-neureka-tiled-sequential" which misleadingly implies
sequential test execution while the job runs tests with "-n 4" (parallel
pytest/xdist), so either rename the workflow string to reflect parallel
execution (e.g., remove "-sequential" or change to "-parallel") or update the
test invocation to run sequentially; locate the name field "name:
_runner-siracusa-neureka-tiled-sequential" and the test command that includes
"-n 4" and make them consistent so the workflow name accurately represents the
execution mode.
| test-type: | ||
| required: true | ||
| type: string | ||
| num-cores: | ||
| required: true | ||
| type: number | ||
| description: "Type of tests to run: kernels or models" |
There was a problem hiding this comment.
Inconsistent input naming across platform runners.
This workflow uses test-type as the input parameter, while other platform runners like _runner-softhier.yml and _runner-snitch.yml use pytest-marker. This naming inconsistency makes the test invocation interface less uniform across platforms.
Consider renaming to pytest-marker and using marker-based selection (e.g., -m "siracusa and ${{ inputs.pytest-marker }}") to align with other runners.
🤖 Prompt for AI Agents
In @.github/workflows/_runner-siracusa.yml around lines 17 - 20, The workflow
input "test-type" is inconsistent with other runners; rename the input to
"pytest-marker" (replace occurrences of test-type with pytest-marker), update
its description to reflect marker-based selection, and change the test
invocation to use marker selection (e.g., use -m "siracusa and ${{
inputs.pytest-marker }}" when running pytest) so the interface matches
_runner-softhier.yml and _runner-snitch.yml.
| # Run tests using pytest markers | ||
| pytest test_platforms.py::test_siracusa_${{ inputs.test-type }} -v -s |
There was a problem hiding this comment.
Test execution approach inconsistent with other runners.
The Siracusa runner has two issues compared to other platform runners:
-
Different test selection method: Uses direct test function selection (
::test_siracusa_${{ inputs.test-type }}) instead of marker-based filtering (e.g.,-m "siracusa and marker") used in_runner-softhier.ymland_runner-snitch.yml. -
Missing parallel execution: Unlike other runners, this doesn't use
-n 4for parallel test execution, which would improve performance on the 4-core GitHub Actions VMs. -
Misleading comment: The comment says "Run tests using pytest markers" but the command doesn't actually use markers.
🔧 Suggested alignment with other runners
-
- # Run tests using pytest markers
- pytest test_platforms.py::test_siracusa_${{ inputs.test-type }} -v -s
+ # Run tests with 4 parallel threads as GitHub action VM has 4 cores
+ pytest test_platforms.py -v -n 4 -m "siracusa and ${{ inputs.pytest-marker }}"Note: This assumes the input parameter is renamed from test-type to pytest-marker as suggested in the previous comment.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @.github/workflows/_runner-siracusa.yml around lines 45 - 47, The workflow
step currently runs pytest with a direct test function selector (`pytest
test_platforms.py::test_siracusa_${{ inputs.test-type }} -v -s`) and a
misleading comment; change it to use marker-based filtering and parallel
execution like the other runners by invoking pytest with the marker input (e.g.,
`-m "${{ inputs.pytest-marker }}"`) and `-n 4`, and update the comment to
accurately state "Run tests using pytest markers and xdist parallelism";
specifically replace the direct selector `test_platforms.py::test_siracusa_${{
inputs.test-type }}` with a marker expression that includes `siracusa` plus the
provided marker (`-m "siracusa and ${{ inputs.pytest-marker }}"`) and add `-n 4`
to enable parallel runs.
| IMAGE="ghcr.io/pulp-platform/deeploy:main" | ||
| else | ||
| IMAGE="ghcr.io/pulp-platform/deeploy:devel" | ||
| IMAGE="ghcr.io/victor-jung/deeploy:pytest-migration" |
There was a problem hiding this comment.
Avoid defaulting CI to a contributor-owned container image (reliability + supply-chain risk).
For forks/PRs and long-term maintenance, ghcr.io/victor-jung/deeploy:pytest-migration is likely to be inaccessible/ephemeral and shifts trust to a personal namespace.
Proposed change (keep official default; require explicit opt-in for migration image)
- else
- IMAGE="ghcr.io/victor-jung/deeploy:pytest-migration"
+ else
+ IMAGE="ghcr.io/pulp-platform/deeploy:devel"
fi🤖 Prompt for AI Agents
In @.github/workflows/_select-env.yml at line 37, The workflow currently
hardcodes IMAGE="ghcr.io/victor-jung/deeploy:pytest-migration", which uses a
contributor-owned container; change this to avoid trust/supply-chain risk by
removing the personal namespace and either defaulting to an organization-owned
image (e.g., set IMAGE to the official org image or tag) or by leaving IMAGE
unset and requiring explicit opt-in via an environment variable or workflow
input; update the IMAGE assignment in the workflow so it references a
maintainer-controlled image or an opt-in mechanism instead of
ghcr.io/victor-jung/deeploy:pytest-migration.
| def test_siracusa_tiled_kernels_l2_singlebuffer(test_params, deeploy_test_dir, toolchain, toolchain_dir, cmake_args, | ||
| skipgen, skipsim) -> None: | ||
| test_name, l1, config_name = test_params | ||
| config = create_test_config( | ||
| test_name = test_name, | ||
| platform = "Siracusa", | ||
| simulator = "gvsoc", | ||
| deeploy_test_dir = deeploy_test_dir, | ||
| toolchain = toolchain, | ||
| toolchain_dir = toolchain_dir, | ||
| cmake_args = cmake_args, | ||
| tiling = True, | ||
| cores = SIRACUSA_DEFAULT_CORES, | ||
| l1 = l1, | ||
| default_mem_level = "L2", | ||
| double_buffer = False, | ||
| ) | ||
| run_and_assert_test(test_name, config, skipgen, skipsim) | ||
| config = create_test_config( | ||
| test_name = test_name, | ||
| platform = "Siracusa", | ||
| simulator = "gvsoc", | ||
| deeploy_test_dir = deeploy_test_dir, | ||
| toolchain = toolchain, | ||
| toolchain_dir = toolchain_dir, | ||
| cmake_args = cmake_args, | ||
| tiling = True, | ||
| cores = SIRACUSA_DEFAULT_CORES, | ||
| l1 = l1, | ||
| default_mem_level = "L2", | ||
| double_buffer = False, | ||
| ) | ||
| run_and_assert_test(test_name, config, skipgen, skipsim) |
There was a problem hiding this comment.
Critical: Duplicate test execution.
The test function runs the same test configuration twice. Lines 307-321 are an exact duplicate of lines 292-306, causing each parameterized test to execute twice unnecessarily.
🐛 Proposed fix: Remove duplicate code
def test_siracusa_tiled_kernels_l2_singlebuffer(test_params, deeploy_test_dir, toolchain, toolchain_dir, cmake_args,
skipgen, skipsim) -> None:
test_name, l1, config_name = test_params
config = create_test_config(
test_name = test_name,
platform = "Siracusa",
simulator = "gvsoc",
deeploy_test_dir = deeploy_test_dir,
toolchain = toolchain,
toolchain_dir = toolchain_dir,
cmake_args = cmake_args,
tiling = True,
cores = SIRACUSA_DEFAULT_CORES,
l1 = l1,
default_mem_level = "L2",
double_buffer = False,
)
run_and_assert_test(test_name, config, skipgen, skipsim)
- config = create_test_config(
- test_name = test_name,
- platform = "Siracusa",
- simulator = "gvsoc",
- deeploy_test_dir = deeploy_test_dir,
- toolchain = toolchain,
- toolchain_dir = toolchain_dir,
- cmake_args = cmake_args,
- tiling = True,
- cores = SIRACUSA_DEFAULT_CORES,
- l1 = l1,
- default_mem_level = "L2",
- double_buffer = False,
- )
- run_and_assert_test(test_name, config, skipgen, skipsim)🧰 Tools
🪛 Ruff (0.14.10)
291-291: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In @DeeployTest/test_platforms.py around lines 289 - 321, The
test_siracusa_tiled_kernels_l2_singlebuffer function contains a duplicated
create_test_config + run_and_assert_test block; remove the second occurrence so
the function only constructs the config and calls run_and_assert_test once for
the given test_params (delete the repeated create_test_config(...) and
subsequent run_and_assert_test(...) that duplicate the earlier ones).
2 participants
We used to write the tests directly in the GitHub Action YAML files; this is not a good practice, as it is hard to run several tests locally. PyTest is a well-known test framework that allows decoupling the testing from the GitHub CI, so users can easily run all tests for any platform locally if needed.
Currently, I use PyTest only for the Generic platform. I'd like a first round of review from @Xeratec or @diaconuccalin before moving forward. Moving the CI for other platforms will be done in a similar way.All platforms and internal deeploy tests are now moved to the PyTest suite and the CI is only using the PyTest suite.
Additionally, the PyTest suite can run tests in parallel and generate one build folder per worker to prevent conflicts.
Tagging @haugoug as he's interested in this feature.
To run all the tests of the generic platform with 8 parallel threads, you can do
You can select a specific marker with
-m, like only the kernels, you can do:To list all the captured tests for a given expression, you can do:
To execute a specific test, you can use:
You can also use an expression to filter tests, for instance the following command execute all test whose name contain "Adder":
I summarized the important points to use the PyTest suite in a README in the
DeeployTestfolder.Added
pytestandpytest-xdistas dependencies of Deeploy.pytest.inifor the global configuration of PyTest for the project.conftest.pyto define CLI args for PyTest for the whole project, it also defines a set of global fixtures and markers.pytestRunner.pycontains helper functions and fixtures for the whole project.test_platforms.pylists the E2E tests and sorts them into marked categories (per platform and per kernel/model).Changed
Fixed
nvidia-pyindexwas broken as it now tries to build the wheel to respect the new policy on packages usingpyproject. Instead of installing this package, we just add thehttps://pypi.ngc.nvidia.comchannel to the pip config file.PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.