Update CLI interface Across Project, Fix Tutorial, and Remove Legacy Test#157
Conversation
📝 WalkthroughWalkthroughDocs, comments, CI workflows, Dockerfile, and tests updated: Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions"
participant Repo as "Repository (git)"
participant Prepare as "prepare job"
participant Detect as "detect_release_tag step"
participant Manifest as "manifest_tags step"
participant Merge as "Merge Deeploy Images"
participant Registry as "Docker Registry"
GH->>Repo: trigger workflow (push/tag/dispatch)
GH->>Prepare: start prepare job
Prepare->>Repo: checkout (fetch-depth: 0)
Prepare->>Detect: run release tag detection
Detect-->>Prepare: output release_tag
Prepare->>Manifest: assemble tags (latest, docker_tag, release_tag?)
Manifest-->>Prepare: output tags
Prepare-->>GH: set job outputs (docker_release_tag, tags)
GH->>Merge: invoke merge with provided tags
Merge->>Registry: push/merge images using tags
Registry-->>Merge: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/install.md (1)
82-90:⚠️ Potential issue | 🟡 MinorUpdate the narrative to match the new example test.
The example now runs
Tests/Models/CNN_Linear1, but the sentence below still sayssimpleRegression.✏️ Suggested fix
- to run the `simpleRegression` test on your workstation. + to run the `CNN_Linear1` test on your workstation.
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 6-8: Edit the CHANGELOG entry that currently reads "Update CLI
interface Across Project, Fix Tutorial, and Remove Legacy Test [`#157`]" and
remove the redundant word "interface" so it reads "Update CLI Across Project,
Fix Tutorial, and Remove Legacy Test [`#157`]"; update the exact PR title string
in the List of Pull Requests section accordingly.
In `@docs/tutorials/introduction.md`:
- Around line 199-203: The quoted multi-line command currently uses inline
backticks and should be converted to a fenced code block to satisfy
markdownlint; replace the inline backtick-wrapped block under the quoted lines
(the lines starting with ">`" and the command "python
deeployRunner_tiled_siracusa_w_neureka.py -t
Tests/Models/microLlama/microLlama64_parallel --cores=8 --l1 64000
--defaultMemLevel=L2") with a fenced block like a blockquote followed by
```bash, the command on its own line, and a closing ``` so the content remains
quoted but uses a proper fenced code block.
- Around line 37-45: The fenced code block containing the deeployRunner commands
lacks a language tag; update the block that lists the deeployRunner_* commands
(deeployRunner_generic.py, deeployRunner_cortexm.py, deeployRunner_mempool.py,
deeployRunner_snitch.py, deeployRunner_siracusa.py) by changing the opening
fence from ``` to ```bash so the block becomes a bash code block and keep the
closing fence unchanged.
- Around line 70-72: Update the narrative to use the correct test directory name
and hyphenation: replace any occurrences of "IntKernels/Add/Regular" with
"Tests/Kernels/Integer/Add/Regular", hyphenate "single node" to "single-node",
and ensure the text clarifies that CI test names map to directories under
DeeployTest/Tests/ rather than Python function names (search for the paragraph
mentioning IntKernels/Add/Regular and MobileNetv2/Transformer to apply these
edits).
Victor-Jung
force-pushed
the
pr/update-deeploy-tutorial
branch
from
dbf525f to
9a8eef6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/tutorials/introduction.md`:
- Line 34: Remove the stale NVIDIA PyPI extra-index flag from the install
instruction by deleting the substring
"--extra-index-url=https://pypi.ngc.nvidia.com" from the pip command ("pip
install -e . --extra-index-url=https://pypi.ngc.nvidia.com") so the line reads
simply "pip install -e ."; also verify related references (e.g., the v0.2.1
release notes mention) are consistent with the CI/Dockerfile cleanup so no other
docs still require the NVIDIA index.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/docker-build-deeploy.yml:
- Around line 148-160: The manifest_tags step uses a mismatched heredoc
delimiter (opens with tags<<'EOF' but closes with EOF) so the GITHUB_OUTPUT
multipart value is never set, and it also appends trailing commas to each tag
which creates an empty element for the docker-manifest-action; fix by either
replacing the heredoc with a simple single-line assignment that joins tags
(recommended) or ensure matching delimiters (tags<<'EOF' ... 'EOF' or tags<<EOF
... EOF) and remove trailing commas from each echo so the final comma is not
emitted; update the manifest_tags step (look for OWNER_LC_VALUE, tags<<'EOF',
and the echo lines emitting ghcr.io/${OWNER_LC_VALUE}/deeploy:...) accordingly.
🧹 Nitpick comments (5)
.github/workflows/package-publish.yml (3)
22-24: Consider scopingid-token: writeto only the publish jobs.The
id-token: writepermission is set at the workflow level, granting it to all jobs includingbuild-package, which doesn't need it. Moving it to job-levelpermissionson the two publish jobs follows the principle of least privilege.♻️ Suggested change
permissions: contents: read - id-token: write jobs: build-package: name: Build package artifacts runs-on: ubuntu-latest + permissions: + contents: read steps: ... publish-pypi: name: Publish to PyPI ... runs-on: ubuntu-latest + permissions: + contents: read + id-token: write steps: ... publish-test-pypi: name: Publish to Test PyPI ... runs-on: ubuntu-latest + permissions: + contents: read + id-token: write steps: ...
53-69: Consider adding a GitHubenvironmentto the publish jobs for trusted publishing.
uv publishhere relies on PyPI trusted publishing (OIDC) since no token is explicitly provided. If the PyPI project's trusted publisher is configured with an environment constraint, this job will fail without anenvironment:declaration. Even if not strictly required, using a deployment environment (e.g.,pypi) adds a layer of protection (approval gates, environment-scoped secrets) against accidental or unauthorized publishes.♻️ Suggested change
publish-pypi: name: Publish to PyPI if: github.event_name == 'push' needs: build-package runs-on: ubuntu-latest + environment: pypi steps:
71-87: Sameenvironmentrecommendation applies here — consider addingenvironment: test-pypi.This keeps Test PyPI and production PyPI publish paths symmetrically protected.
♻️ Suggested change
publish-test-pypi: name: Publish to Test PyPI if: github.event_name == 'workflow_dispatch' && inputs.publish_target == 'test-pypi' needs: build-package runs-on: ubuntu-latest + environment: test-pypi steps:.github/workflows/docker-build-deeploy.yml (2)
43-51: Tag detection logic looks sound; minor nondeterminism with multiple tags.
head -n 1picks the first tag alphabetically (fromgit tag --points-at HEAD), which is nondeterministic if multiple tags point at HEAD. If this is intentional (e.g., only one release tag is expected per commit), this is fine; otherwise consider filtering (e.g., by av*glob) for predictability:git tag --points-at HEAD -l 'v*' | head -n 1 || true
161-164:OWNER_LC_VALUEis redundantly recomputed.
OWNER_LCis already lowercased and exported to$GITHUB_ENVin the preceding "Lower Case Repository Name" step (lines 142-146), so it's available here. You can reference$OWNER_LCdirectly instead of recomputing from$OWNER.
|
@Xeratec I did several changes:
|
2 participants
Added
Changed
Removed
testDMA.pywas an old test; we now havetest_dmas.pyinstead.Fixed
PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.