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 :)
…test, fix generic runner
…one (state contamination)
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughMigrates legacy per-test Python runners to a pytest-based test harness. Replaces workflow inputs/lists with a single Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant Config as Workflow Input (pytest-marker)
participant Pytest as pytest (test_platforms)
participant Harness as pytestRunner (DeeployTest/testUtils)
participant Toolchain as Toolchain/Build/Simulator
participant Result as Test Result
GHA->>Config: provide `pytest-marker`
GHA->>Pytest: run `pytest test_platforms.py -m "platform and <marker>" -n 4`
Pytest->>Harness: create DeeployTestConfig per test param
Harness->>Toolchain: configure CMake and build (per-worker dirs)
Harness->>Toolchain: run simulation or host execution
Toolchain-->>Harness: return stdout/stderr and exit code
Harness->>Pytest: produce TestResult (success/fail, logs)
Pytest->>GHA: report aggregated results
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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.
| 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.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the commit exists and show the changes to _select-env.yml
git show 0cd8ce2ec46a8d2e2d3b4e6195f4fd846d700cec -- .github/workflows/_select-env.yml
Length of output: 0
🏁 Script executed:
#!/bin/bash
# First check if the commit exists
echo "=== Checking if commit exists ==="
git cat-file -t 0cd8ce2ec46a8d2e2d3b4e6195f4fd846d700cec 2>&1
echo ""
echo "=== Current content of the file at line 37 ==="
sed -n '35,40p' .github/workflows/_select-env.yml
echo ""
echo "=== Recent commits affecting this file ==="
git log --oneline -10 -- .github/workflows/_select-env.yml
echo ""
echo "=== Check if the personal image reference still exists ==="
grep -n "victor-jung" .github/workflows/_select-env.yml || echo "No victor-jung reference found"
Length of output: 648
🏁 Script executed:
#!/bin/bash
# Show what the commit actually changed
echo "=== Full diff of commit 0cd8ce2 ==="
git show --stat 0cd8ce2ec46a8d2e2d3b4e6195f4fd846d700cec
echo ""
echo "=== Is this commit in current branch history? ==="
git branch --contains 0cd8ce2ec46a8d2e2d3b4e6195f4fd846d700cec
echo ""
echo "=== What did it change (abbreviated)? ==="
git show --oneline --name-only 0cd8ce2ec46a8d2e2d3b4e6195f4fd846d700cec | head -20
Length of output: 689
@Victor-Jung, I checked commit 0cd8ce2, and while it does address other CodeRabbit suggestions (changes to _runner-siracusa.yml, ci-platform-siracusa.yml, and test_platforms.py), the issue with the container image in .github/workflows/_select-env.yml is still present.
The file currently still contains at line 37:
IMAGE="ghcr.io/victor-jung/deeploy:pytest-migration"This line creates a supply-chain risk by defaulting to a contributor-owned container image. For production CI, the workflow should either:
- Default to the official organization image:
ghcr.io/pulp-platform/deeploy:devel - Require explicit opt-in via a workflow input for development images
Would you like to address this in a follow-up commit?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/_runner-siracusa.yml:
- Around line 45-47: The pytest command places -v -n 4 between -m and its marker
argument, so pytest will treat -v as the marker; move the marker expression
immediately after -m so the command becomes something like: pytest -m "siracusa
and ${{ inputs.pytest-marker }}" -v -n 4 test_platforms.py (i.e., ensure -m is
immediately followed by "siracusa and ${{ inputs.pytest-marker }}" and then
place -v -n 4 afterwards).
🧹 Nitpick comments (2)
DeeployTest/README.md (1)
3-12: Consider improving markdown structure for consistency.Minor markdown linting issues flagged by static analysis:
- Heading jump from
#(h1) to###(h3) at line 5 - consider using##for major sections- Code blocks lack language specifiers (e.g., use
```bashfor shell commands,```textfor output)These don't affect functionality but improve rendering and accessibility.
📝 Example fix for heading structure
# How to use the DeeployTest PyTest Suite? -### Executing and Collecting Test Groups +## Executing and Collecting Test GroupsDeeployTest/test_platforms.py (1)
289-306: Consider prefixing unusedconfig_namewith underscore.The
config_namevariable is part of the parameter tuple for generating test IDs viaparam_id, but isn't used within the test body. This pattern repeats across all tiled test functions. Prefixing with underscore (_config_name) signals intentional non-use and silences the linter warning.♻️ Example fix (apply to all tiled tests)
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 + test_name, l1, _config_name = test_params
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/_runner-siracusa.yml.github/workflows/ci-platform-siracusa.ymlDeeployTest/README.mdDeeployTest/test_platforms.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/_runner-siracusa.ymlDeeployTest/README.md
🧬 Code graph analysis (1)
DeeployTest/test_platforms.py (2)
DeeployTest/testUtils/pytestRunner.py (2)
create_test_config(363-466)run_and_assert_test(469-488)DeeployTest/conftest.py (6)
deeploy_test_dir(86-88)toolchain(135-137)toolchain_dir(98-103)cmake_args(141-143)skipgen(123-125)skipsim(129-131)
🪛 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)
15-15: 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)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
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)
320-320: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
349-349: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
378-378: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
407-407: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
436-436: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
479-479: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
501-501: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
527-527: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
530-530: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
560-560: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
589-589: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
618-618: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
647-647: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
677-677: Unpacked variable config_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
708-708: 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). (17)
- GitHub Check: siracusa-models-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-neureka-models-tiled-singlebuffer-L3 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-kernels-tiled-singlebuffer-L2 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-models-tiled-doublebuffer-L3 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-kernels-tiled-singlebuffer-L2-wmem / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-kernels-tiled-doublebuffer-L2 / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-models-tiled-doublebuffer-L3-wmem / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-models / test-runner-siracusa
- GitHub Check: siracusa-kernels / test-runner-siracusa
- GitHub Check: mempool-kernels / test-runner-mempool
- GitHub Check: mempool-models / test-runner-mempool
- GitHub Check: generic-models / test-runner-generic
- GitHub Check: generic-kernels / test-runner-generic
- GitHub Check: softhier-kernels / test-runner-softhier
- GitHub Check: docs
🔇 Additional comments (4)
.github/workflows/ci-platform-siracusa.yml (1)
32-46: LGTM!Clean migration to marker-based test selection. The
kernelsandmodelsmarkers will be combined withsiracusain the runner workflow to properly filter tests.DeeployTest/test_platforms.py (3)
38-59: LGTM - well-designed helper functions.The
generate_test_paramsandparam_idfunctions provide clean test parameterization with readable test IDs.
62-103: Clean platform configuration structure.The
PLATFORM_CONFIGSdictionary provides a well-organized central configuration for platforms. Note that Siracusa and Siracusa+Neureka configurations are handled inline in their respective test functions (withcoresparameter), which is appropriate given their additional complexity.
128-143: Well-structured test with proper marker and fixture usage.The test function pattern is clean: markers for selection, parameterization for test variations, fixture injection for configuration, and delegation to
create_test_config/run_and_assert_testfor execution. This pattern is consistently applied across all platform tests.
| # Run tests using pytest markers | ||
| pytest test_platforms.py -m -v -n 4 "siracusa and ${{ inputs.pytest-marker }}" |
There was a problem hiding this comment.
Incorrect pytest argument order will cause test selection to fail.
The -m flag requires the marker expression immediately after it. The current command places -v -n 4 between -m and its argument, which will cause pytest to interpret -v as the marker expression instead.
🐛 Proposed fix
- # Run tests using pytest markers
- pytest test_platforms.py -m -v -n 4 "siracusa and ${{ inputs.pytest-marker }}"
+ # Run tests using pytest markers
+ pytest test_platforms.py -m "siracusa and ${{ inputs.pytest-marker }}" -v -n 4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Run tests using pytest markers | |
| pytest test_platforms.py -m -v -n 4 "siracusa and ${{ inputs.pytest-marker }}" | |
| # Run tests using pytest markers | |
| pytest test_platforms.py -m "siracusa and ${{ inputs.pytest-marker }}" -v -n 4 |
🤖 Prompt for AI Agents
In @.github/workflows/_runner-siracusa.yml around lines 45 - 47, The pytest
command places -v -n 4 between -m and its marker argument, so pytest will treat
-v as the marker; move the marker expression immediately after -m so the command
becomes something like: pytest -m "siracusa and ${{ inputs.pytest-marker }}" -v
-n 4 test_platforms.py (i.e., ensure -m is immediately followed by "siracusa and
${{ inputs.pytest-marker }}" and then place -v -n 4 afterwards).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/_runner-siracusa.yml:
- Around line 14-20: The workflow uses hyphenated input names in expressions
(inputs.docker-image and inputs.pytest-marker) which GitHub Actions treats as
subtraction; update all occurrences of `${{ inputs.docker-image }}` and `${{
inputs.pytest-marker }}` to bracket notation `${{ inputs['docker-image'] }}` and
`${{ inputs['pytest-marker'] }}` respectively (search for these exact expression
strings in the template and replace them), ensuring every call site and
reference uses the bracket form so the workflow evaluates correctly.
🧹 Nitpick comments (1)
.github/workflows/_runner-siracusa.yml (1)
40-48: Preferenv+working-directoryfor marker passing; consider-n autoInlining
${{ inputs[...] }}into the bash command makes quoting/shell-parsing brittle; passing viaenvis safer and also reads cleaner. Also, hard-coding-n 4may under/over-utilize runners;-n autois usually a better default.Proposed refactor
- name: Run Test - run: | - cd DeeployTest - mkdir -p /app/.ccache - export CCACHE_DIR=/app/.ccache - - # Run tests using pytest markers - pytest test_platforms.py -v -n 4 -m "siracusa and ${{ inputs.pytest-marker }}" + working-directory: DeeployTest shell: bash + env: + CCACHE_DIR: /app/.ccache + PYTEST_MARKER: ${{ inputs['pytest-marker'] }} + run: | + set -euo pipefail + mkdir -p "${CCACHE_DIR}" + # Run tests using pytest markers + pytest test_platforms.py -v -n auto -m "siracusa and ${PYTEST_MARKER}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/_runner-siracusa.yml
🧰 Additional context used
🧠 Learnings (1)
📚 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/_runner-siracusa.yml
⏰ 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). (17)
- GitHub Check: siracusa-neureka-models-tiled-doublebuffer-L3-wmem / test-runner-siracusa-neureka-tiled
- GitHub Check: siracusa-neureka-models-tiled-singlebuffer-L3 / test-runner-siracusa-neureka-tiled
- GitHub Check: deeploy-internal-tests
- GitHub Check: siracusa-models / test-runner-siracusa
- GitHub Check: siracusa-kernels / test-runner-siracusa
- GitHub Check: linting
- GitHub Check: snitch-kernels-tiled-singlebuffer-L2 / test-runner-snitch-tiled
- GitHub Check: softhier-kernels / test-runner-softhier
- GitHub Check: chimera-kernels / test-runner-chimera
- GitHub Check: cortexm-kernels / test-runner-cortexm
- GitHub Check: cortexm-models / test-runner-cortexm
- GitHub Check: mempool-models / test-runner-mempool
- GitHub Check: mempool-kernels / test-runner-mempool
- GitHub Check: snitch-kernels / test-runner-snitch
- GitHub Check: generic-models / test-runner-generic
- GitHub Check: generic-kernels / test-runner-generic
- GitHub Check: docs
| docker-image: | ||
| required: true | ||
| type: string | ||
| test-names: | ||
| pytest-marker: | ||
| required: true | ||
| type: string | ||
| num-cores: | ||
| required: true | ||
| type: number | ||
| description: "Pytest marker for test selection (e.g., 'kernels', 'models')" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/_runner-siracusa.ymlRepository: pulp-platform/Deeploy
Length of output: 1583
🏁 Script executed:
# Search for all callers of this workflow
rg -n "uses.*_runner-siracusa\.yml" --hiddenRepository: pulp-platform/Deeploy
Length of output: 258
🏁 Script executed:
# Search for old input references that might still be in use
rg -n "pytest-marker|test-names|num-cores" .github/workflows/ --hiddenRepository: pulp-platform/Deeploy
Length of output: 4497
🌐 Web query:
GitHub Actions hyphenated input names property access vs bracket notation
💡 Result:
Short answer: use bracket notation.
Details:
- In workflow/JS expressions a hyphen is not a valid identifier (it's parsed as subtraction), so dot notation fails. Use bracket notation, e.g. github.event.inputs['my-input']. [1][2]
- GitHub also exposes inputs as environment variables named INPUT_<NAME_IN_UPPERCASE>; hyphens are preserved in that env var name (e.g. INPUT_NUM-OCTOCATS), so in scripts use bracket access like process.env['INPUT_NUM-OCTOCATS'] (or process.env['INPUT_MY-INPUT']). [1][2]
Examples:
- Expression in workflow: ${{ github.event.inputs['my-input'] }}. [1]
- In a JavaScript action step: const val = process.env['INPUT_MY-INPUT'];. [2][1]
Sources:
[1] GitHub Actions metadata / inputs (env variable naming & examples).
[2] JavaScript property accessors (dot vs bracket notation).
Use bracket notation for hyphenated input names — workflow evaluation will fail
In GitHub Actions expressions, hyphens are parsed as subtraction operators, not valid identifier characters. The expressions ${{ inputs.docker-image }} and ${{ inputs.pytest-marker }} will fail at runtime. Use bracket notation instead:
Required fix
container:
- image: ${{ inputs.docker-image }}
+ image: ${{ inputs['docker-image'] }}
steps:
- name: Run Test
run: |
cd DeeployTest
mkdir -p /app/.ccache
export CCACHE_DIR=/app/.ccache
- pytest test_platforms.py -v -n 4 -m "siracusa and ${{ inputs.pytest-marker }}"
+ pytest test_platforms.py -v -n 4 -m "siracusa and ${{ inputs['pytest-marker'] }}"Affects lines 26 and 47. All call sites have correctly passed these inputs; only the template's expression syntax needs correction.
🤖 Prompt for AI Agents
In @.github/workflows/_runner-siracusa.yml around lines 14 - 20, The workflow
uses hyphenated input names in expressions (inputs.docker-image and
inputs.pytest-marker) which GitHub Actions treats as subtraction; update all
occurrences of `${{ inputs.docker-image }}` and `${{ inputs.pytest-marker }}` to
bracket notation `${{ inputs['docker-image'] }}` and `${{
inputs['pytest-marker'] }}` respectively (search for these exact expression
strings in the template and replace them), ensuring every call site and
reference uses the bracket form so the workflow evaluates correctly.
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.