feat: add upgrade model support in fileManager remove operation #138
Conversation
📝 WalkthroughWalkthroughCentralizes file-removal into retry helpers, aggregates upgrade and regular removal tasks into a single Promise.all flow, exposes Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ArtModelService
participant DevClient
participant RetryUtil
participant Storage
User->>ArtModelService: removeModel(name, params)
ArtModelService->>DevClient: initClient()
alt upgrade files present
ArtModelService->>ArtModelService: create _removeUpgradeSingleFile tasks
end
ArtModelService->>ArtModelService: create regular _removeSingleFile tasks
loop each removal task
ArtModelService->>RetryUtil: _removeFileWithRetry(fileName, mountConfig)
RetryUtil->>DevClient: retry fileManager.rm (with mountConfig)
DevClient->>Storage: remove file (NAS/OSS)
Storage-->>DevClient: success / NoSuchFile / error
DevClient-->>RetryUtil: result or error
alt init-error
RetryUtil->>DevClient: retry with backoff
end
RetryUtil-->>ArtModelService: resolved or rejected result
end
ArtModelService->>ArtModelService: await Promise.all(allRemovePromises)
ArtModelService-->>User: aggregated results / throw on failures
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/subCommands/model/fileManager.ts (1)
193-235: Fix TDZ onprocessedOssMountPointsand don’t skip upgrade-only removals.
processedOssMountPointsis referenced in the upgrade branch before it’s declared, which will throw at runtime (and is a TS error). Also, returning early whenfilesis empty skips upgrade removals entirely. ComputeprocessedOssMountPointsfirst and only short‑circuit when bothfilesandupgradeare empty.🐛 Proposed fix
- const { files, upgrade } = modelConfig; - if (_.isEmpty(files)) { - logger.info('[Remove-model] No files specified for removal.'); - return; - } - let allRemovePromises = []; + const { files, upgrade } = modelConfig; + const processedOssMountPoints = extractOssMountDir(ossMountPoints); + if (_.isEmpty(files) && _.isEmpty(upgrade)) { + logger.info('[Remove-model] No files specified for removal.'); + return; + } + let allRemovePromises = []; if (!_.isEmpty(upgrade)) { logger.info('[Remove-model] Upgrade model, only support once model.'); const removeUpgradePromises = Object.keys(upgrade).map((key) => this._removeUpgradeSingleFile(devClient, upgrade[key], key, { name, nasMountPoints, ossMountPoints: processedOssMountPoints, role, vpcConfig, modelConfig, region, timeout: modelConfig?.timeout, }), ); // 将升级文件删除任务添加到总的任务列表中 allRemovePromises = allRemovePromises.concat(removeUpgradePromises); } - const processedOssMountPoints = extractOssMountDir(ossMountPoints);
🧹 Nitpick comments (1)
src/subCommands/model/fileManager.ts (1)
308-326: Confirm upgrade paths are already resolved filepaths.
_removeUpgradeSingleFileforwardspathdirectly to the remover. Please verify thatupgradevalues are already absolute file paths (notnas://auto,oss://auto, or relative). If they can be URIs, reuse the same normalization as_removeSingleFile.
mozhou52
force-pushed
the
fileManager-remove
branch
from
29dd1fe to
dfae8ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/subCommands/model/fileManager.ts (1)
262-264: Error message reports incorrect total count.The error message at line 263 uses
files.lengthbut doesn't account for upgrade files. When upgrade files are present, the total count will be inaccurate.🐛 Proposed fix
- throw new Error( - `[Remove-model] ${failedRemovals.length} out of ${files.length} files failed to remove.`, - ); + throw new Error( + `[Remove-model] ${failedRemovals.length} out of ${removeResults.length} files failed to remove.`, + );
🤖 Fix all issues with AI agents
In `@__tests__/ut/commands/artModelService_test.ts`:
- Around line 86-92: The object definition for artModelService.createResource
set via Object.defineProperty is missing a trailing comma after "configurable:
true", causing a formatting error; update the Object.defineProperty call that
defines artModelService.createResource to include the trailing comma (i.e.,
ensure the property descriptor ends with a comma after configurable: true) so
the file is validly formatted.
- Around line 541-584: The test supplies upgrade: { 'v1.0':
'/mnt/test/v1.0/file1.txt' } but the code in fileManager.ts expects
upgrade?.history, so update the test data in
__tests__/ut/commands/artModelService_test.ts to use upgrade: { history: {
'v1.0': '/mnt/test/v1.0/file1.txt' } } so the upgrade removal branch is
exercised when calling artModelService.removeModel; keep the rest of the test
(including the mock for retryFileManagerRm and the expectation) unchanged.
In `@src/subCommands/model/fileManager.ts`:
- Around line 200-217: The upgrade handling currently checks upgrade?.history
and accesses upgrade.history[key], but test data provides upgrade as a flat
object, so change the condition to check upgrade (e.g., if (upgrade &&
!_.isEmpty(upgrade))) and update the mapping to use upgrade[key] when calling
this._removeUpgradeSingleFile so removeUpgradePromises is populated; this
touches the variables allRemovePromises, upgrade, and the call site
this._removeUpgradeSingleFile in fileManager.ts.
| let allRemovePromises = []; | ||
| if (upgrade?.history && !_.isEmpty(upgrade.history)) { | ||
| logger.info('[Remove-model] Upgrade model, only support once model.'); | ||
| const removeUpgradePromises = Object.keys(upgrade.history).map((key) => | ||
| this._removeUpgradeSingleFile(devClient, upgrade.history[key], key, { | ||
| name, | ||
| nasMountPoints, | ||
| ossMountPoints: processedOssMountPoints, | ||
| role, | ||
| vpcConfig, | ||
| modelConfig, | ||
| region, | ||
| timeout: modelConfig?.timeout, | ||
| }), | ||
| ); | ||
| // 将升级文件删除任务添加到总的任务列表中 | ||
| allRemovePromises = allRemovePromises.concat(removeUpgradePromises); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "fileManager.ts" -o -name "*test*.ts" | head -20Repository: devsapp/fc3
Length of output: 285
🏁 Script executed:
cat -n src/subCommands/model/fileManager.ts | sed -n '195,225p'Repository: devsapp/fc3
Length of output: 1375
🏁 Script executed:
rg -n "upgrade" src/subCommands/model/fileManager.ts -B2 -A2Repository: devsapp/fc3
Length of output: 726
🏁 Script executed:
# Search for test files that might contain the test data
fd -t f "\.test\.ts$\|\.spec\.ts$" | xargs rg -l "upgrade.*v1\.0" 2>/dev/null | head -5Repository: devsapp/fc3
Length of output: 37
🏁 Script executed:
fd -type f -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.js"Repository: devsapp/fc3
Length of output: 225
🏁 Script executed:
rg -l "upgrade" --type=ts | head -10Repository: devsapp/fc3
Length of output: 249
🏁 Script executed:
# Search for where upgrade is defined in modelConfig or test data
rg "upgrade.*v1\.0\|history.*upgrade" --type=ts -B2 -A2Repository: devsapp/fc3
Length of output: 37
🏁 Script executed:
cat -n __tests__/ut/commands/model_test.ts | sed -n '550,560p'Repository: devsapp/fc3
Length of output: 437
🏁 Script executed:
# Search for upgrade structure in test files more broadly
rg -n "upgrade" __tests__/ut/commands/model_test.ts -B2 -A2 | head -50Repository: devsapp/fc3
Length of output: 337
🏁 Script executed:
# Also check artModelService_test.ts for upgrade patterns
rg -n "upgrade" __tests__/ut/commands/artModelService_test.ts -B2 -A2 | head -50Repository: devsapp/fc3
Length of output: 361
🏁 Script executed:
# Check the test that uses this upgrade data to see expected behavior
cat -n __tests__/ut/commands/artModelService_test.ts | sed -n '541,580p'Repository: devsapp/fc3
Length of output: 1573
🏁 Script executed:
# Search for any transformation or type definitions of upgrade
rg -n "upgrade.*history\|history.*upgrade" --type=ts -B3 -A3Repository: devsapp/fc3
Length of output: 37
🏁 Script executed:
# Look for type definitions of modelConfig
rg -n "interface.*modelConfig\|type.*modelConfig" --type=ts -B2 -A10Repository: devsapp/fc3
Length of output: 37
🏁 Script executed:
# Search more broadly for upgrade field definitions and usage
rg -n "upgrade\s*[={:]" --type=ts | head -30Repository: devsapp/fc3
Length of output: 331
🏁 Script executed:
# Check for any TODO or FIXME comments related to upgrade
rg -n "TODO|FIXME|BUG" src/subCommands/model/fileManager.tsRepository: devsapp/fc3
Length of output: 37
🏁 Script executed:
# Look at the full removeModel method signature and how upgrade is used
cat -n src/subCommands/model/fileManager.ts | sed -n '175,220p'Repository: devsapp/fc3
Length of output: 2001
🏁 Script executed:
# Check _removeUpgradeSingleFile method to understand expected structure
cat -n src/subCommands/model/fileManager.ts | sed -n '309,330p'Repository: devsapp/fc3
Length of output: 717
Fix structural mismatch in upgrade property access.
The code at line 201 checks for upgrade?.history and accesses upgrade.history[key], but the actual data structure from test data (artModelService_test.ts line 554-556) provides upgrade as a flat object: { 'v1.0': '/mnt/test/v1.0/file1.txt' }, not nested under a history property. This causes the condition to fail and the entire upgrade file removal logic to never execute.
Change line 201 from:
if (upgrade?.history && !_.isEmpty(upgrade.history)) {
to:
if (upgrade && !_.isEmpty(upgrade)) {
And update line 203-204 to access upgrade[key] instead of upgrade.history[key].
🤖 Prompt for AI Agents
In `@src/subCommands/model/fileManager.ts` around lines 200 - 217, The upgrade
handling currently checks upgrade?.history and accesses upgrade.history[key],
but test data provides upgrade as a flat object, so change the condition to
check upgrade (e.g., if (upgrade && !_.isEmpty(upgrade))) and update the mapping
to use upgrade[key] when calling this._removeUpgradeSingleFile so
removeUpgradePromises is populated; this touches the variables
allRemovePromises, upgrade, and the call site this._removeUpgradeSingleFile in
fileManager.ts.
dfae8ea to
c3e16b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/subCommands/model/fileManager.ts`:
- Around line 285-306: The code builds filepath from the local variable path but
does not strip leading slashes, causing possible double-slashes; before using
path in the uri/nas/oss branches in the function that calls
_removeFileWithRetry, normalize path the same way as _getDestinationPath does
(e.g., remove any leading '/' characters from file.target.path into a
sanitizedPath variable) and use that sanitizedPath when constructing filepath
for the nasMountPoints/ossMountPoints and normalizedUri branches so filepath is
always formed without double slashes; update references to path in this block
and keep _removeFileWithRetry(filepath, fileName, config) unchanged.
♻️ Duplicate comments (1)
__tests__/ut/commands/artModelService_test.ts (1)
541-584: Test data structure still doesn't match implementation.The test uses
upgrade: { 'v1.0': '/mnt/test/v1.0/file1.txt' }(lines 554-556), but the implementation infileManager.tsline 201 checks forupgrade?.history. This test will pass but won't exercise the upgrade removal code path.🐛 Proposed fix for test data
upgrade: { - 'v1.0': '/mnt/test/v1.0/file1.txt', + history: { + 'v1.0': '/mnt/test/v1.0/file1.txt', + }, },
mozhou52
force-pushed
the
fileManager-remove
branch
2 times, most recently
from
8c25614 to
9d6f916
Compare
- Add upgrade property to modelConfig for handling upgrade models - Implement _removeUpgradeSingleFile method to handle upgrade file removal - Refactor remove operation to support both regular and upgrade files - Improve error logging order in retryFileManagerRm function - Consolidate remove promises into a single execution list Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
mozhou52
force-pushed
the
fileManager-remove
branch
from
9d6f916 to
fce20c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@__tests__/ut/commands/artModelService_test.ts`:
- Around line 554-558: The object literal in the test contains upgrade ->
history -> 'v1.0' without a trailing comma; update the history entry in
__tests__/ut/commands/artModelService_test.ts so the 'v1.0':
'/mnt/test/v1.0/file1.txt' line ends with a comma (i.e., add a trailing comma to
the history entry) to satisfy the formatter; locate the upgrade and history
objects in the test (references: upgrade, history, 'v1.0') and add the comma.
In `@src/subCommands/model/fileManager.ts`:
- Around line 193-217: The early return when files is empty prevents processing
upgrade artifacts; modify the logic in the block that reads const { files,
upgrade } = modelConfig so that if files is empty you do not return early when
upgrade?.history exists and is non-empty — allow the upgrade.history branch to
run. Concretely, remove or change the return after logger.info('[Remove-model]
No files specified for removal.') and ensure processedOssMountPoints,
allRemovePromises and the mapping that calls this._removeUpgradeSingleFile(...)
still execute when upgrade.history contains entries so upgrade-only removals are
scheduled and added to allRemovePromises.
| upgrade: { | ||
| history: { | ||
| 'v1.0': '/mnt/test/v1.0/file1.txt' | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Fix trailing comma to satisfy formatter.
CI reports a formatting failure here; add the trailing comma after the upgrade history entry.
🔧 Proposed fix
upgrade: {
history: {
- 'v1.0': '/mnt/test/v1.0/file1.txt'
+ 'v1.0': '/mnt/test/v1.0/file1.txt',
},
},📝 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.
| upgrade: { | |
| history: { | |
| 'v1.0': '/mnt/test/v1.0/file1.txt' | |
| }, | |
| }, | |
| upgrade: { | |
| history: { | |
| 'v1.0': '/mnt/test/v1.0/file1.txt', | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In `@__tests__/ut/commands/artModelService_test.ts` around lines 554 - 558, The
object literal in the test contains upgrade -> history -> 'v1.0' without a
trailing comma; update the history entry in
__tests__/ut/commands/artModelService_test.ts so the 'v1.0':
'/mnt/test/v1.0/file1.txt' line ends with a comma (i.e., add a trailing comma to
the history entry) to satisfy the formatter; locate the upgrade and history
objects in the test (references: upgrade, history, 'v1.0') and add the comma.
| const { files, upgrade } = modelConfig; | ||
| if (_.isEmpty(files)) { | ||
| logger.info('[Remove-model] No files specified for removal.'); | ||
| return; | ||
| } | ||
| const processedOssMountPoints = extractOssMountDir(ossMountPoints); | ||
|
|
||
| let allRemovePromises = []; | ||
| if (upgrade?.history && !_.isEmpty(upgrade.history)) { | ||
| logger.info('[Remove-model] Upgrade model, only support once model.'); | ||
| const removeUpgradePromises = Object.keys(upgrade.history).map((key) => | ||
| this._removeUpgradeSingleFile(devClient, upgrade.history[key], key, { | ||
| name, | ||
| nasMountPoints, | ||
| ossMountPoints: processedOssMountPoints, | ||
| role, | ||
| vpcConfig, | ||
| modelConfig, | ||
| region, | ||
| timeout: modelConfig?.timeout, | ||
| }), | ||
| ); | ||
| // 将升级文件删除任务添加到总的任务列表中 | ||
| allRemovePromises = allRemovePromises.concat(removeUpgradePromises); | ||
| } |
There was a problem hiding this comment.
Avoid skipping upgrade-only removals.
Line 194 returns early when files is empty, which prevents upgrade history removals from running. If a model only has upgrade artifacts, they’ll never be deleted.
🔧 Suggested fix
- const { files, upgrade } = modelConfig;
- if (_.isEmpty(files)) {
- logger.info('[Remove-model] No files specified for removal.');
- return;
- }
+ const { files, upgrade } = modelConfig;
+ const hasFiles = !_.isEmpty(files);
+ const hasUpgradeHistory = !!upgrade?.history && !_.isEmpty(upgrade.history);
+ if (!hasFiles && !hasUpgradeHistory) {
+ logger.info('[Remove-model] No files specified for removal.');
+ return;
+ }🤖 Prompt for AI Agents
In `@src/subCommands/model/fileManager.ts` around lines 193 - 217, The early
return when files is empty prevents processing upgrade artifacts; modify the
logic in the block that reads const { files, upgrade } = modelConfig so that if
files is empty you do not return early when upgrade?.history exists and is
non-empty — allow the upgrade.history branch to run. Concretely, remove or
change the return after logger.info('[Remove-model] No files specified for
removal.') and ensure processedOssMountPoints, allRemovePromises and the mapping
that calls this._removeUpgradeSingleFile(...) still execute when upgrade.history
contains entries so upgrade-only removals are scheduled and added to
allRemovePromises.
Summary by CodeRabbit
Refactor
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.