Feat/vllm unsloth readme #143
Conversation
WalkthroughUpdates replace and consolidate documentation links, remove some external deployment references from READMEs, add detailed READMEs for Unsloth and vLLM, and enhance the Unsloth runner script to accept numeric offer IDs and additional data input forms. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (3)
Tunnel Client/chat/src/client/components/Main/Blocks/Description/helpers.ts (1)
24-29: Docs URL is fine; consider aligning trailing-slash styleThe new CLI docs URL works, but other places in this PR typically use
https://docs.superprotocol.com/cli(no trailing slash). Consider standardizing for consistency, though it’s not functionally required.unsloth/README.md (1)
1-172: Strong end‑to‑end README; only minor markdown/style nitsThe overall flow (TEE paths, SPCTL prerequisites, file vs Jupyter modes, dry‑run, dataset/model selection) is clear and matches how Unsloth is expected to run on Super Protocol. Content-wise this is in good shape. A few small polish points:
Markdown list indentation (MD007)
Nested bullets under “2) your model” and the “Provide your dataset…” steps are indented with 4 spaces before the dash. markdownlint flags these (lines 48–50 and 136–138) and expects 2-space indentation. For example, you can adjust patterns like:
- a path to the model's resource JSON file, if it was already uploaded with SPCTL
- model offer ID, if the model exists on the Marketplace
- a path to the local directory with the model to upload it using SPCTL.
- a path to the model's resource JSON file, if it was already uploaded with SPCTL
- model offer ID, if the model exists on the Marketplace
- a path to the local directory with the model to upload it using SPCTL.
Same applies to the analogous dataset bullets in both the file and Jupyter sections.
Step numbering in the Jupyter section
The Jupyter section has two items numbered “8.” (waiting for the Jupyter order and opening the link). Markdown renderers will auto-renumber, but it reads a bit confusing in raw form. Consider making the final step “9.” for clarity.Optional clarity: working directory
You mention copyingspctlandconfig.jsonintounsloth/scriptsand then show./run-unsloth.sh. Adding a brief “cd unsloth/scripts” before the first shell block would make it explicit where users should run the script from.Overall, the README is comprehensive and well aligned with the existing Unsloth behavior and workflows. Based on learnings, …
unsloth/scripts/run-unsloth.sh (1)
736-736: Remove unused variable.
DATA_ARCHIVE_NAMEis computed but never used. The script now performs direct folder uploads without archiving (as indicated by the "no archiving" message on line 754).Apply this diff:
# Compute descriptor name for suggestion regardless of existence DATA_DIR_BASE="$(basename -- "$DATA_DIR_VALUE")" - DATA_ARCHIVE_NAME="${DATA_DIR_BASE}-data.tar.gz" DATA_DESCRIPTOR_NAME="${DATA_DIR_BASE}-data.json"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
.vscode/extensions.json(1 hunks)Blockchain/README.md(0 hunks)Python/readme.md(1 hunks)Tunnel Client/chat/README.md(0 hunks)Tunnel Client/chat/src/client/components/Main/Blocks/Deploy/helpers.ts(1 hunks)Tunnel Client/chat/src/client/components/Main/Blocks/Description/helpers.ts(1 hunks)Tunnel Client/chat/src/client/components/Main/Blocks/Title/helpers.ts(1 hunks)Tunnel Client/chat/src/client/components/PageLinks/helpers.ts(1 hunks)Tunnel Client/chat/src/client/components/Room/InfoAccordion/helpers.tsx(1 hunks)Tunnel Client/minecraft/README.md(0 hunks)Tunnel Client/static-content/src/common/data.ts(1 hunks)Tunnel Client/static-content/src/views/Content/Marketplace/helpers.ts(1 hunks)Tunnel Client/static-content/src/views/Content/Rewards/helpers.ts(1 hunks)readme.md(1 hunks)unsloth/README.md(1 hunks)unsloth/scripts/run-unsloth.sh(4 hunks)vllm/README.md(1 hunks)
💤 Files with no reviewable changes (3)
- Blockchain/README.md
- Tunnel Client/chat/README.md
- Tunnel Client/minecraft/README.md
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-31T14:54:13.528Z
Learnt from: marchuk-vlad
Repo: Super-Protocol/solutions PR: 135
File: unsloth/Dockerfile:7-7
Timestamp: 2025-10-31T14:54:13.528Z
Learning: For Super Protocol solutions, Docker images that copy .env files are acceptable because images are encrypted and loaded in TEE (Trusted Execution Environment), which provides a different security model than traditional Docker deployments.
Applied to files:
unsloth/README.mdvllm/README.mdreadme.mdPython/readme.md
📚 Learning: 2025-10-31T14:58:50.082Z
Learnt from: marchuk-vlad
Repo: Super-Protocol/solutions PR: 135
File: unsloth/src/helpers.ts:59-65
Timestamp: 2025-10-31T14:58:50.082Z
Learning: In the unsloth solution (unsloth/src/helpers.ts), the runPythonFile and runIpynbFile functions intentionally swallow errors by logging and returning instead of throwing. This allows the Kubernetes pod to complete successfully and return logs even when execution fails, which is required for their operational workflow.
Applied to files:
unsloth/README.md
📚 Learning: 2025-10-31T15:01:54.857Z
Learnt from: marchuk-vlad
Repo: Super-Protocol/solutions PR: 135
File: unsloth/src/server.ts:26-83
Timestamp: 2025-10-31T15:01:54.857Z
Learning: In the unsloth project (unsloth/src/server.ts), the Jupyter notebook spawn arguments include embedded single quotes (e.g., `--NotebookApp.token=''`, `--NotebookApp.allow_origin='*'`, `--NotebookApp.password='${hashed}'`) and this pattern has been tested and confirmed to work correctly with Jupyter's argument parsing.
Applied to files:
unsloth/README.md
📚 Learning: 2025-10-31T14:56:52.188Z
Learnt from: marchuk-vlad
Repo: Super-Protocol/solutions PR: 135
File: unsloth/scripts/run-unsloth.sh:503-520
Timestamp: 2025-10-31T14:56:52.188Z
Learning: In the unsloth/scripts/run-unsloth.sh script, displaying sensitive credentials (TLS private keys and tunnel tokens) in suggestion hints and terminal output is an accepted risk for this project.
Applied to files:
unsloth/README.md
🪛 Gitleaks (8.30.0)
vllm/README.md
[high] 48-48: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
unsloth/README.md
48-48: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
136-136: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
137-137: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
138-138: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Shellcheck (0.11.0)
unsloth/scripts/run-unsloth.sh
[warning] 736-736: DATA_ARCHIVE_NAME appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (11)
.vscode/extensions.json (1)
2-2: Clarify the intent of removing ESLint recommendation.The removal of
dbaeumer.vscode-eslintfrom the recommendations is not mentioned in the PR objectives, which focus on documentation updates and script changes. Please confirm whether this change is intentional and related to the broader PR scope.Tunnel Client/static-content/src/views/Content/Marketplace/helpers.ts (1)
24-27: Marketplace footer URL update looks correctThe new docs URL for Marketplace is consistent with the rest of the PR and keeps the structure unchanged.
Tunnel Client/chat/src/client/components/Main/Blocks/Title/helpers.ts (1)
6-9: Guide link pointing to CLI docs is consistentRouting the guide button to the CLI docs aligns with other updated links and the overall docs consolidation in this PR.
Python/readme.md (1)
1-3: Concise Python image description looks goodThe shortened introduction is clear and avoids stale links, which fits the docs cleanup in this PR.
Tunnel Client/static-content/src/common/data.ts (1)
12-21: Docs and CLI links update is consistentRenaming “FAQ” to “Docs” and “Developers” to “Super Protocol CLI” with the new URLs matches the rest of the navigation changes and keeps the structure intact.
Tunnel Client/chat/src/client/components/Main/Blocks/Deploy/helpers.ts (1)
13-21: Deployment links now target the right docsPointing the GUI entry to the Marketplace overview and the CLI entry to the quick-guide aligns with the new documentation structure and keeps the array shape unchanged.
Tunnel Client/chat/src/client/components/Room/InfoAccordion/helpers.tsx (1)
11-12: Info accordion links updated consistentlyThe updated links for Testnet info and building on Super Protocol correctly route users to the main docs and CLI docs, in line with the rest of the navigation changes.
readme.md (1)
3-20: LGTM!The updated solution table comprehensively documents all available solutions including the new vLLM and Unsloth entries. The formatting and descriptions are clear and consistent.
Tunnel Client/static-content/src/views/Content/Rewards/helpers.ts (1)
8-16: LGTM!The documentation link updates align with the centralized documentation structure. The URLs are correctly formatted and point to appropriate CLI-focused guidance.
Tunnel Client/chat/src/client/components/PageLinks/helpers.ts (1)
3-6: LGTM!Consolidating multiple documentation links into a single Docs hub entry simplifies navigation and aligns with the centralized documentation approach across the PR.
unsloth/scripts/run-unsloth.sh (1)
686-764: LGTM!The enhanced input handling correctly supports:
- Numeric offer IDs for direct marketplace references
- JSON resource descriptors for pre-uploaded data
- Directory paths for new uploads
The logic properly handles suggest-only mode and provides appropriate error messages. The no-archiving upload flow aligns with modern best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
unsloth/scripts/run-unsloth.sh (2)
736-736: Remove unused archive name variables.
DATA_ARCHIVE_NAME(line 736) andMODEL_ARCHIVE_NAME(line 819) are defined but never referenced. These appear to be artifacts from older archiving logic and should be deleted to keep the code clean.- DATA_DIR_BASE="$(basename -- "$DATA_DIR_VALUE")" - DATA_ARCHIVE_NAME="${DATA_DIR_BASE}-data.tar.gz" - DATA_DESCRIPTOR_NAME="${DATA_DIR_BASE}-data.json" + DATA_DIR_BASE="$(basename -- "$DATA_DIR_VALUE")" + DATA_DESCRIPTOR_NAME="${DATA_DIR_BASE}-data.json"- MODEL_DIR_BASE="$(basename -- "$MODEL_DIR_VALUE")" - MODEL_ARCHIVE_NAME="${MODEL_DIR_BASE}-model.tar.gz" - MODEL_DESCRIPTOR_NAME="${MODEL_DIR_BASE}-model.json" + MODEL_DIR_BASE="$(basename -- "$MODEL_DIR_VALUE")" + MODEL_DESCRIPTOR_NAME="${MODEL_DIR_BASE}-model.json"Also applies to: 819-819
773-773: Remove the extra blank line.Line 773 is a blank line that appears unintended. If section separation is desired, a single blank line between logical sections is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
unsloth/scripts/run-unsloth.sh(4 hunks)vllm/README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T14:54:13.528Z
Learnt from: marchuk-vlad
Repo: Super-Protocol/solutions PR: 135
File: unsloth/Dockerfile:7-7
Timestamp: 2025-10-31T14:54:13.528Z
Learning: For Super Protocol solutions, Docker images that copy .env files are acceptable because images are encrypted and loaded in TEE (Trusted Execution Environment), which provides a different security model than traditional Docker deployments.
Applied to files:
vllm/README.md
🪛 Gitleaks (8.30.0)
vllm/README.md
[high] 48-48: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Shellcheck (0.11.0)
unsloth/scripts/run-unsloth.sh
[warning] 736-736: DATA_ARCHIVE_NAME appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (12)
unsloth/scripts/run-unsloth.sh (5)
242-243: ✓ Typo fix approved.Line 243 now correctly reads "Model input:" instead of the previous "Model inputc:". The updated prompt also appropriately reflects the new capability to accept resource JSON paths, numeric offer IDs, or folder paths.
686-705: Numeric ID support for data resources looks solid.The logic correctly distinguishes between numeric offer IDs (which bypass file existence checks) and file-based resource JSON paths. The conditional flow and error handling align well with the suggest-only vs. strict-mode behavior.
709-709: ✓ Updated prompt reflects expanded input forms.The prompt now clearly communicates the three accepted input types: resource JSON path, numeric offer ID, or folder path. This improves user clarity.
719-764: Multi-form data input handling is well-structured.The logic correctly:
- Routes numeric IDs directly to
DATA_DESCRIPTORSwithout file checks- Detects JSON descriptors via grep (fields: "hash", "encryption", "resource") and reclassifies them as
DATA_RESOURCE- Handles folder uploads with proper error messaging in both suggest-only and strict modes
- Records suggestions appropriately for non-interactive re-runs
775-799: Model resource handling mirrors data resource pattern correctly.The numeric ID and file-path handling for models is consistent with the data resource logic and appropriately integrates with the
DATA_DESCRIPTORSarray used downstream.vllm/README.md (7)
3-3: Past review concern already resolved.The previous comment flagged an incorrect reference to "Unsloth workloads" in the vLLM README. This has been corrected—line 3 now properly references "vLLM workloads."
45-51: Static analysis false positive—example output is safe.The gitleaks finding on line 48 detects a generic API key pattern in the example output block. This is expected and harmless; it's clearly a dummy key shown for documentation purposes to illustrate the deployment output format, not a real exposed credential.
1-8: Introduction is clear and accurate.The overview effectively communicates the purpose, TEE capability, and key implementation detail that the script uses a pre-existing solution offer.
11-16: Prerequisites are clear and actionable.The section correctly identifies required dependencies and provides clear setup instructions. The link to SPCTL documentation appears well-formed.
18-53: Order placement workflow is comprehensive and well-documented.The interactive dialog steps are clearly explained with good examples. The enhancement to accept numeric offer IDs (line 35) is properly documented, aligning with the script improvements mentioned in the PR objectives.
55-62: API endpoints are accurately documented with proper reference.The list of main endpoints is representative, and the link to the full vLLM API documentation is appropriate for readers needing more detailed information.
64-82: Dry run workflow is well-explained with practical examples.The --suggest-only flag, dialog completion process, and example output are clearly documented. The generated command example (lines 77-82) provides a useful pattern for non-interactive usage.
1 participant
Update run-unsloth.sh to accept a dataset as an existing offer
Add README to Unsloth and vLLM
Remove old and broken Docs links
Update general README
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.