Conversation
- Enable pulling from private GitLab repo - Improve caching for pip, apt and cargo - Fix cMake version - Remove problematic pip installation in favor of apt package - Add ZSH an Oh My ZSH - Add package dependencies for GAP9 SDK - Remove unused files from the container - Fix banshee package problems
… issueFix duplicate template generation due to PULP inheritance issue
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/infra-generate-ccache-gap9.yml:
- Around line 16-17: The comment stating the cron schedule is CET is incorrect
because GitHub Actions uses UTC; update the comment above the cron entry that
currently reads the schedule as "0 2 * * *" so it accurately states the timezone
(UTC) or converts the time to CET if you want a 02:00 CET run (e.g., note that
"0 2 * * *" runs at 02:00 UTC / 03:00 CET). Keep the cron line "- cron: \"0 2 *
* *\"" as-is if you intend UTC and change the explanatory comment to mention
UTC, or adjust the cron expression and comment together if you want actual CET
scheduling.
- Around line 50-54: The cache step using actions/cache@v4 currently uses a
static key "ccache-gap9" which prevents updates; update the step that defines
path: /app/.ccache and key: ccache-gap9 to generate a unique, versioned key
(e.g., include github.run_id or a hashFiles() call) and add a restore-keys entry
so older caches can be used as fallbacks; specifically modify the "Clean and
Upload CCache" step to compute a dynamic key (for example by combining a prefix
like "ccache-gap9-" with hashFiles('**/ccache-config') or github.run_id) and add
restore-keys: ccache-gap9- to allow partial restores.
In `@Deeploy/Targets/GAP9/Platform.py`:
- Around line 202-218: The _bufferRepresentation output in GAP9TransientBuffer
uses self._type while GAP9VariableBuffer uses self._instance, causing
inconsistent type/property serialization; inspect the classes
GAP9TransientBuffer and GAP9VariableBuffer and decide on the canonical attribute
name (either _type or _instance), then update
GAP9TransientBuffer._bufferRepresentation (or GAP9VariableBuffer) to use the
chosen attribute consistently and add a fallback (e.g., check hasattr and
default to None) to avoid AttributeError when the attribute is absent; ensure
the updated attribute name matches how instances are initialized elsewhere in
the class constructors.
- Around line 294-306: MemoryGAP9PlatformWrapper defines untiledOps = [] which
is inconsistent with MemoryGAP9Platform (which sets untiledOps = ["add"]) and
renders the conditional in getTargetMemoryLevel dead; update
MemoryGAP9PlatformWrapper.untiledOps to match the platform (set untiledOps to
["add"]) following the pattern used in MemoryPULPPlatformWrapper so the check in
getTargetMemoryLevel(node, tensorName, ctxt) behaves as intended.
🧹 Nitpick comments (4)
cmake/simulation.cmake (1)
72-78: Minor stylistic inconsistency withgvsoc_flags_add_files_to_hyperflash.The hyperflash version (lines 63-70) uses
list(TRANSFORM)while this uses aforeachloop. Both are correct, but aligning the implementation style would improve maintainability.♻️ Optional: align with hyperflash version using list(TRANSFORM)
function(gvsoc_flags_add_files_to_flash out_var files_var) - set(flags) - foreach(file ${${files_var}}) - list(APPEND flags "--flash-property=${file}@flash:readfs_flash:files") - endforeach() + set(flags ${${files_var}}) + list(TRANSFORM flags PREPEND "--flash-property=") + list(TRANSFORM flags APPEND "@flash:readfs_flash:files") set(${out_var} ${flags} PARENT_SCOPE) endfunction()Deeploy/Targets/GAP9/Platform.py (2)
221-237: LGTM!Properly extends parent class buffer representation. The same
getattrsimplification could be applied here for consistency:memoryLevel = getattr(self, "_memoryLevel", None)
262-270: Avoid mutable default argument with function call.The default
engines = [GAP9ClusterEngine("GAP9Cluster")]is evaluated once at function definition time. While unlikely to cause issues in practice, this pattern can lead to subtle bugs if the list is modified.♻️ Suggested fix
class GAP9Platform(DeploymentPlatform): def __init__(self, - engines = [GAP9ClusterEngine("GAP9Cluster")], + engines = None, variableBuffer = GAP9VariableBuffer, constantBuffer = GAP9ConstantBuffer, structBuffer = GAP9StructBuffer, transientBuffer = GAP9TransientBuffer) -> None: + if engines is None: + engines = [GAP9ClusterEngine("GAP9Cluster")] super().__init__(engines, variableBuffer, constantBuffer, structBuffer, transientBuffer).github/workflows/infra-generate-ccache-gap9.yml (1)
32-41: Avoid swallowing setup/build failures.
|| truehides failures in environment setup and installation, which can lead to generating or uploading an incomplete cache with no signal. If the script is optional, log and skip explicitly; otherwise fail fast.🛠️ Proposed fix (fail fast or explicit guard)
- name: Build Deeploy shell: bash run: | source /app/install/gap9-sdk/.gap9-venv/bin/activate - source /app/install/gap9-sdk/configs/gap9_evk_audio.sh || true - pip install -e . || true + if [ ! -f /app/install/gap9-sdk/configs/gap9_evk_audio.sh ]; then + echo "Missing gap9_evk_audio.sh" >&2 + exit 1 + fi + source /app/install/gap9-sdk/configs/gap9_evk_audio.sh + pip install -e . deactivate - name: Generate CCache for GAP9 run: | source /app/install/gap9-sdk/.gap9-venv/bin/activate - source /app/install/gap9-sdk/configs/gap9_evk_audio.sh || true + source /app/install/gap9-sdk/configs/gap9_evk_audio.sh
runwangdl
force-pushed
the
gap9-operators-github
branch
from
f4face7 to
67e7e5b
Compare
- Remove TargetLibraries/GAP9/third_party from git tracking (this is a CMake-generated symlink that should not be in the repo) - Add GAP9/third_party to .gitignore to prevent future tracking - Revert pulp-nn-mixed submodule to original commit (a9b4aaf5) (submodule was unintentionally updated to b69ec23e)
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Xeratec
left a comment
Xeratec
left a comment
There was a problem hiding this comment.
Thanks for the adjustments. I think we will converge in the next round. I only have a few minor comments and would still prefer to move the pulp-nn libraries from TargetLibraries/PULPOpen/third_party to TargetLibraries/third_party.
As discussed offline, I will create another PR once this is merged to properly update the Dockerflow, including support for running on external hardware.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TargetLibraries/PULPOpen/CMakeLists.txt (1)
42-47:⚠️ Potential issue | 🟠 MajorGuard
pulp-nn-mixedinclusion to prevent duplicate target errors.Both
TargetLibraries/PULPOpen/CMakeLists.txtandTargetLibraries/GAP9/CMakeLists.txtcalladd_subdirectory(../third_party/pulp-nn-mixed). Configuring both platforms in the same build causes CMake to fail with "source directory already added" error. Wrap the subdirectory call with aTARGETguard:-add_subdirectory(../third_party/pulp-nn-mixed) +if(NOT TARGET pulp-nn-mixed) + add_subdirectory(../third_party/pulp-nn-mixed) +endif() target_include_directories(pulp-nn-mixed PUBLIC ${PULP_SDK_INCLUDES}) target_compile_options(pulp-nn-mixed PUBLIC ${PULP_SDK_COMPILE_FLAGS})
🤖 Fix all issues with AI agents
In @.github/workflows/ci-platform-gap9-tiled.yml:
- Around line 27-30: The conditional passed to the select-env step (the value of
input docker_image_deeploy in the select-env block) lacks explicit grouping for
the || and && operators; update the expression to add parentheses so the
repo-equals check is grouped with the && fallback (e.g. wrap (github.repository
== 'pulp-platform/Deeploy' && 'ghcr.io/pulp-platform/deeploy-gap9:latest')
before the ||) so forks don't yield the literal "false" string and the fallback
behaves correctly; modify the docker_image_deeploy expression in the select-env
step accordingly.
8c1f6ed to
a0e65a6
Compare
Specify binary directory when adding third_party submodules since they are not subdirectories of PULPOpen/GAP9.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@TargetLibraries/GAP9/CMakeLists.txt`:
- Around line 39-47: NUM_CORES is used without being defined which leads to
surprising behavior; add an explicit guard that checks DEFINED NUM_CORES before
the USE_PREBUILT_PULPNN check and error out if it is unset. Specifically, before
the block that uses NUM_CORES (the if(USE_PREBUILT_PULPNN AND NOT NUM_CORES
EQUAL 8) check), add a check like if(NOT DEFINED NUM_CORES) and call
message(FATAL_ERROR "...") to fail fast and tell the user to supply NUM_CORES;
keep the rest of the logic for option(USE_PREBUILT_PULPNN ...) and the 8-core
validation unchanged.
🧹 Nitpick comments (2)
TargetLibraries/GAP9/CMakeLists.txt (2)
5-12: AddCONFIGURE_DEPENDSto keep globbed sources in sync.
Without it, adding/removing files won’t trigger a CMake reconfigure.♻️ Proposed change
-file(GLOB_RECURSE SOURCES - "src/**" -) +file(GLOB_RECURSE SOURCES CONFIGURE_DEPENDS + "${CMAKE_CURRENT_LIST_DIR}/src/**" +) @@ -file(GLOB_RECURSE PULPOPEN_SOURCES "../PULPOpen/src/**") +file(GLOB_RECURSE PULPOPEN_SOURCES CONFIGURE_DEPENDS + "${CMAKE_CURRENT_LIST_DIR}/../PULPOpen/src/**" +)
21-23: Prefertarget_compile_definitionsforNUM_CORES.
This keeps usage requirements in the right property instead ofCOMPILE_OPTIONS.♻️ Proposed change
-target_compile_options(deeploygap9 PUBLIC - -DNUM_CORES=${NUM_CORES} - ) +target_compile_definitions(deeploygap9 PUBLIC NUM_CORES=${NUM_CORES})
Signed-off-by: Run Wang <52746141+runwangdl@users.noreply.github.com>
|
@Xeratec I’ve updated the changelog and resolved the requested changes. |
Xeratec
left a comment
Xeratec
left a comment
There was a problem hiding this comment.
The changelog is modified in the wrong place. No worries, I will fix it, rebase the branch, and merge it once the PR passes. Thanks a lot for your contribution.
2 participants
Summary
This PR adds complete GAP9 platform support to Deeploy, including platform integration, DMA support, tiling capabilities, CI/CD workflows, and comprehensive testing infrastructure. This represents 20 commits specifically focused on GAP9 development.
Added
GAP9 Platform Support
Changed
- Transpose operator: Fixed GCC segmentation fault caused by template syntax (commit 9ca4595)
- LayerNorm operator: Resolved epsilon ABI compatibility issue (commit 6b5c2e5)
Known Limitations
Platform Capabilities
✅ Multi-core (1-8) | ✅ L1/L2/L3 memory | ✅ Multi-channel DMA
✅ GVSoC simulation | ✅ Tiling | ✅ PULP-NN integration
PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.