Feature/allow full compute asset#156
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds support for raw (unpublished) datasets and algorithms in compute CLI commands. A new ChangesRaw compute input resolution and wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
The PR successfully introduces support for full ComputeAsset and ComputeAlgorithm JSON inputs, enabling the use of raw datasets and algorithms without DIDs. It significantly refactors and cleans up the CLI commands by extracting common resolution logic into a reusable and well-tested resolveComputeInputs helper. Excellent architectural improvement and test coverage.
Comments:
• [WARNING][style] These look like leftover debugging logs. Consider removing them to keep the CLI console output clean.
- console.log("Bucket ID:", bucketId);
- console.log("File Path:", filePath);
- console.log("File Name:", fileName);• [INFO][style] Passing algoDdo as Asset | DDO when algoDdo could be null (since raw algorithms return null for algoDdo) might cause unexpected behavior if isOrderable strictly checks for undefined but doesn't handle null. Though it's likely fine as null is falsy, it's safer to use the nullish coalescing operator to pass undefined.
- algoDdo as Asset | DDO
+ (algoDdo ?? undefined) as Asset | DDO• [INFO][other] The addition of this test suite is excellent! Great job covering all the edge cases like legacy arrays, raw/DID mixes, and fallback provider URLs.
![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 (1)
test/resolveComputeInputs.test.ts (1)
61-75: ⚡ Quick winAdd a regression test for unbracketed comma-separated DIDs.
Given backward-compat support is expected, add a case for
did:op:a,did:op:b(without brackets) so this contract stays protected.Suggested test case
+ it("supports legacy comma-separated dataset DIDs without brackets", async function () { + const res = await resolveComputeInputs( + "did:op:a,did:op:b", + "did:op:algo1", + makeAquarius(), + indexingParams, + FALLBACK + ); + expect(res).to.not.equal(null); + expect(res.assets.map((a) => a.documentId)).to.deep.equal([ + "did:op:a", + "did:op:b", + ]); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/resolveComputeInputs.test.ts` around lines 61 - 75, Add a new regression test case to ensure backward compatibility for unbracketed comma-separated DIDs. Create a test similar to the existing "supports the legacy unquoted [did:a,did:b] datasets form" test, but instead call resolveComputeInputs with the string "did:op:a,did:op:b" (without brackets) and verify that it produces the same results: 2 assets with documentIds "did:op:a" and "did:op:b", and all ddos values are non-null.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands.ts`:
- Around line 299-309: The isOrderable function is async but is being called
without await in three locations, causing validation checks to fail since a
Promise is always truthy. Add the await keyword before each call to isOrderable
at lines 304, 473, and 768 in src/commands.ts to ensure the function result is
properly evaluated before the conditional checks (such as if (!canStartCompute))
are executed.
In `@src/helpers.ts`:
- Around line 310-323: The function currently only splits comma-separated DIDs
when they are wrapped in brackets, causing plain comma-separated values like
did:op:a,did:op:b to be treated as a single token. Before the final return
statement that returns [trimmed], add a check to see if trimmed contains a
comma. If it does, split the trimmed string by comma and apply the same
processing (trim each element and filter out empty strings) that is already
being done for the bracketed case, to restore support for the legacy
comma-separated DID format.
---
Nitpick comments:
In `@test/resolveComputeInputs.test.ts`:
- Around line 61-75: Add a new regression test case to ensure backward
compatibility for unbracketed comma-separated DIDs. Create a test similar to the
existing "supports the legacy unquoted [did:a,did:b] datasets form" test, but
instead call resolveComputeInputs with the string "did:op:a,did:op:b" (without
brackets) and verify that it produces the same results: 2 assets with
documentIds "did:op:a" and "did:op:b", and all ddos values are non-null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c466a90b-98dc-4166-ab70-64fc4219e719
📒 Files selected for processing (5)
README.mdsrc/cli.tssrc/commands.tssrc/helpers.tstest/resolveComputeInputs.test.ts
| // Validate orderability only for DID-based datasets (raw fileObject assets | ||
| // have no DDO and are validated by the node). | ||
| for (let i = 0; i < assets.length; i++) { | ||
| const dataDdo = ddos[i]; | ||
| if (!dataDdo) continue; | ||
| const canStartCompute = isOrderable( | ||
| ddos[dataDdo], | ||
| ddos[dataDdo].services[0].id, | ||
| dataDdo, | ||
| dataDdo.services[0].id, | ||
| algo, | ||
| algoDdo | ||
| algoDdo as Asset | DDO | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify isOrderable is async
rg -nP 'export\s+async\s+function\s+isOrderable' src/helpers.ts
# Find current non-awaited call sites
rg -nP 'const\s+canStartCompute\s*=\s*isOrderable\(' src/commands.ts
# Confirm awaited call sites (should match count after fix)
rg -nP 'const\s+canStartCompute\s*=\s*await\s+isOrderable\(' src/commands.tsRepository: oceanprotocol/ocean-cli
Length of output: 247
Add await when calling isOrderable or validation is bypassed.
isOrderable is async but these three call sites evaluate the returned Promise directly without awaiting. This makes if (!canStartCompute) checks ineffective since a Promise is always truthy, allowing disallowed compute combinations to pass local validation.
Suggested fix
- const canStartCompute = isOrderable(
+ const canStartCompute = await isOrderable(
dataDdo,
dataDdo.services[0].id,
algo,
algoDdo as Asset | DDO
);Apply this change at lines 304, 473, and 768 in src/commands.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands.ts` around lines 299 - 309, The isOrderable function is async
but is being called without await in three locations, causing validation checks
to fail since a Promise is always truthy. Add the await keyword before each call
to isOrderable at lines 304, 473, and 768 in src/commands.ts to ensure the
function result is properly evaluated before the conditional checks (such as if
(!canStartCompute)) are executed.
alexcos20
requested review from
andreip136,
bogdanfazakas,
giurgiur99 and
ndrpp
as code owners
2 participants
Support raw datasets & algorithms in compute commands
What
startComputeandstartFreeComputepreviously accepted only DIDs for datasets and the algorithm. They now also accept fullComputeAsset/ComputeAlgorithmJSON objects with afileObject, so you can run compute on raw, unpublished data and algorithms (URL, IPFS, Arweave, S3, FTP, node persistent storage) — no publishing, no datatoken required.Both commands also support mixed inputs: a datasets argument can combine published DIDs and raw file objects in the same job.
Why
The underlying
ProviderInstance.computeStart/freeComputeStartalready supported these objects; the CLI just didn't expose them. This unlocks the common "run an algorithm on a file I haven't published" workflow.How
resolveComputeInputsturns the dataset/algorithm CLI strings into ready-to-use compute inputs, auto-detecting DID vs JSON per entry. This also removes ~330 lines of duplicated parsing logic across three functions.[did:a,did:b]form) is unchanged.Usage
JSON must be single-quoted on the shell; in mixed arrays, DIDs must be quoted.
Notes
resolveComputeInputs(DID, raw, mixed, legacy, and error cases).Summary by CodeRabbit
Documentation
New Features
Refactor