Skip to content

Adopt: keep legacy extra_model_paths.yaml install-scoped #1105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Kosinkadink wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Adopt: keep legacy extra_model_paths.yaml install-scoped #1105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions src/main/lib/desktopAdopt.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ import {
parseExtraModelsSections,
deriveLaunchArgs,
computeModelsDirsToCarry,
preserveInstallExtraModelPaths,
getLegacyVenvUvPath,
type AdoptTools,
type AdoptDeps,
Expand Down Expand Up @@ -675,6 +676,63 @@ describe('computeModelsDirsToCarry', () => {
expect(result).not.toContain(path.resolve(path.join(yamlBase, 'models')))
expect(result).toContain(path.resolve(yamlBase))
})

it('does NOT carry extra_model_paths.yaml dirs into the global list (install-scoped)', () => {
// Only extra_models_config.yaml feeds the global modelsDirs; an
// extra_model_paths.yaml is handled install-locally elsewhere.
const basePath = path.join(tmpRoot, 'primary')
fs.mkdirSync(path.join(basePath, 'models'), { recursive: true })
const result = computeModelsDirsToCarry(basePath, null, [])
expect(result).toEqual([path.resolve(path.join(basePath, 'models'))])
})
})

describe('preserveInstallExtraModelPaths', () => {
let tmpRoot: string
beforeEach(() => {
tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'preserveExtra-'))
})
afterEach(() => {
fs.rmSync(tmpRoot, { recursive: true, force: true })
})

it('keeps the source tree file as-is (pre-swap copy) and never clobbers it', () => {
const basePath = path.join(tmpRoot, 'base')
const destSource = path.join(tmpRoot, 'install', 'ComfyUI')
fs.mkdirSync(destSource, { recursive: true })
fs.mkdirSync(basePath, { recursive: true })
fs.writeFileSync(path.join(destSource, 'extra_model_paths.yaml'), 'from_source: {}\n')
// A different basePath copy must NOT overwrite the authoritative source one.
fs.writeFileSync(path.join(basePath, 'extra_model_paths.yaml'), 'from_base: {}\n')
const ok = preserveInstallExtraModelPaths(basePath, destSource, () => {})
expect(ok).toBe(true)
expect(fs.readFileSync(path.join(destSource, 'extra_model_paths.yaml'), 'utf-8')).toContain(
'from_source'
)
})

it('recovers the file from basePath when the sourced tree lacks one (clone fallback)', () => {
const basePath = path.join(tmpRoot, 'base')
const destSource = path.join(tmpRoot, 'install', 'ComfyUI')
fs.mkdirSync(destSource, { recursive: true })
fs.mkdirSync(basePath, { recursive: true })
fs.writeFileSync(path.join(basePath, 'extra_model_paths.yaml'), 'from_base: {}\n')
const ok = preserveInstallExtraModelPaths(basePath, destSource, () => {})
expect(ok).toBe(true)
expect(fs.readFileSync(path.join(destSource, 'extra_model_paths.yaml'), 'utf-8')).toContain(
'from_base'
)
})

it('is a no-op (returns false) when no legacy extra_model_paths.yaml exists', () => {
const basePath = path.join(tmpRoot, 'base')
const destSource = path.join(tmpRoot, 'install', 'ComfyUI')
fs.mkdirSync(destSource, { recursive: true })
fs.mkdirSync(basePath, { recursive: true })
const ok = preserveInstallExtraModelPaths(basePath, destSource, () => {})
expect(ok).toBe(false)
expect(fs.existsSync(path.join(destSource, 'extra_model_paths.yaml'))).toBe(false)
})
})

describe('adoptDesktopInstall', () => {
Expand Down
62 changes: 60 additions & 2 deletions src/main/lib/desktopAdopt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ const ADOPT_INSTALL_NAME = 'ComfyUI'
const COMFY_SETTINGS_FILE = 'comfy.settings.json'
const DESKTOP_CONFIG_FILE = 'config.json'
const EXTRA_MODELS_YAML = 'extra_models_config.yaml'
// The other model-paths file: ComfyUI auto-loads it from the source dir and
// the launcher displays it read-only. Carried alongside EXTRA_MODELS_YAML so
// its dirs survive into v2's modelsDirs (not just the adopted ComfyUI's own
// search), including in the git-clone source fallback where it isn't copied.
const EXTRA_MODEL_PATHS_YAML = 'extra_model_paths.yaml'
Comment on lines +57 to +61

@coderabbitai coderabbitai Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Correct the EXTRA_MODEL_PATHS_YAML comment to match install-local behavior.

Line 57-61 currently implies global modelsDirs carry-over, but the implementation intentionally keeps extra_model_paths.yaml install-scoped, so this comment should be shortened and aligned to avoid future drift.

As per coding guidelines, “Write concise comments — one sentence is better than five paragraphs to justify a small change” and “Don't narrate history in comments (e.g., 'This used to do X, now it does Y'); rely on git log for that information.”

🤖 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/main/lib/desktopAdopt.ts` around lines 57 - 61, The multi-line comment
preceding the EXTRA_MODEL_PATHS_YAML constant declaration is too verbose and
misleadingly suggests the file carries over globally across model directories,
when the actual implementation keeps it install-scoped. Shorten this comment to
a single concise sentence that accurately describes what the constant is and its
install-local scope, removing narrative details about history and implementation
specifics.

Source: Coding guidelines

const WINDOW_FILE = 'window.json'
const VENV_VALIDATE_TIMEOUT_MS = 30_000

Expand Down Expand Up @@ -429,10 +434,16 @@ function isDangerousRoot(resolved: string): boolean {

/**
* Cross-install model dirs to register in `settings.modelsDirs` for the
* adopted record. The goal for migrations is to carry EVERY real model
* directory the legacy `extra_models_config.yaml` referenced so nothing
* adopted record. The goal for migrations is to carry the real model
* directories the legacy `extra_models_config.yaml` referenced so nothing
* shows as "missing" after adoption.
*
* Only the desktop-managed `extra_models_config.yaml` feeds the GLOBAL
* shared list here. Any `extra_model_paths.yaml` is deliberately NOT carried
* globally — its (often install-specific / external) paths stay scoped to the
* adopted install via `preserveInstallExtraModelPaths`, so they don't leak
* into every other install. See that function for the install-local handling.
*
* For each section we register a models ROOT (a dir whose type subfolders
* `buildYaml` scans):
* - Always `<basePath>/models` (the primary install), first.
Expand Down Expand Up @@ -523,6 +534,43 @@ function isUnderRoot(child: string, root: string): boolean {
return rel === '' || (!rel.startsWith('..') && !path.isAbsolute(rel))
}

/**
* Keep the legacy `extra_model_paths.yaml` as an INSTALL-LOCAL file in the
* adopted source tree, so ComfyUI auto-loads it for this install only (it reads
* `<sourceDir>/extra_model_paths.yaml` from `dirname(main.py)`) and the Storage
* tab reports it via `buildExtraModelPathsView`. Deliberately install-scoped —
* its arbitrary/external paths are never merged into the global shared model
* dirs, so they can't leak into other installs; users who want them elsewhere
* can open the file and copy paths out manually.
*
* No-op when the sourced tree already has one (a pre-swap copy carries the
* legacy file verbatim — never clobber it). Otherwise (e.g. the git-clone
* fallback, whose fresh checkout ships only `extra_model_paths.yaml.example`)
* recover a copy from the reused legacy `basePath` when present. Best-effort:
* a missing source or copy failure is logged and swallowed.
*
* Returns true when a file is present in the install afterward (carried or
* recovered), for telemetry/visibility.
*/
export function preserveInstallExtraModelPaths(
basePath: string,
destSource: string,
sendOutput: (t: string) => void
): boolean {
const dest = path.join(destSource, EXTRA_MODEL_PATHS_YAML)
if (fs.existsSync(dest)) return true // pre-swap copy already carried it
const legacy = path.join(basePath, EXTRA_MODEL_PATHS_YAML)
try {
if (!fs.existsSync(legacy)) return false
fs.copyFileSync(legacy, dest)
sendOutput('Preserved legacy extra_model_paths.yaml as an install-local config.\n')
return true
} catch (err) {
sendOutput(`Warning: could not preserve extra_model_paths.yaml: ${(err as Error).message}\n`)
return false
}
}

/**
* Best-effort copy of legacy userData files into a timestamped backup folder.
* Logged on failure but never throws so adoption can continue.
Expand Down Expand Up @@ -1150,6 +1198,15 @@ async function runAdoption(
// 'retry' loops.
}

// Keep the legacy extra_model_paths.yaml as an install-local config (loaded
// by this install's ComfyUI and shown read-only in the Storage tab). Scoped
// to this install — never merged into the global shared model dirs.
const hasInstallExtraModelPaths = preserveInstallExtraModelPaths(
info.basePath,
destSource,
sendOutput
)

// Adoption preserves the user's existing ComfyUI checkout as-is — it is
// not auto-updated to latest stable. A "frozen" install must stay on
// whatever version the user was running; ComfyUI updates are opt-in per
Expand Down Expand Up @@ -1331,6 +1388,7 @@ async function runAdoption(
adopted_source_mode: sourceMode,
has_venv: info.hasVenv,
has_extra_models_yaml: fs.existsSync(path.join(info.configDir, EXTRA_MODELS_YAML)),
has_install_extra_model_paths_yaml: hasInstallExtraModelPaths,
models_dir_count: carry.addedModelsDirs.length,
carried_keys: carry.carriedKeys,
carry_skipped_keys: carry.carrySkippedKeys,
Expand Down
Loading