Skip to content
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

Use basePath from config.json as the desktop install location #750

Merged
merged 4 commits into from
Jan 28, 2025
Merged

Use basePath from config.json as the desktop install location #750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8149716
Use base path from desktop config
webfiltered Jan 24, 2025
ddedf14
Remove unused code
webfiltered Jan 24, 2025
69e9bfc
Update unit tests to reflect changes
webfiltered Jan 28, 2025
a72c1f5
Add readonly
huchenlei Jan 28, 2025
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
48 changes: 3 additions & 45 deletions src/main-process/comfyInstallation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,9 @@ export class ComfyInstallation {
return this.state === 'installed' && !this.hasIssues;
}

virtualEnvironment: VirtualEnvironment;
readonly virtualEnvironment: VirtualEnvironment;
comfySettings: ComfySettings;

_basePath: string;
/** The base path of the desktop app. Models, nodes, and configuration are saved here by default. */
get basePath() {
return this._basePath;
}
set basePath(value: string) {
// Duplicated in constructor to avoid non-nullable type assertions.
this._basePath = value;
this.virtualEnvironment = this.createVirtualEnvironment(value);
}

/**
* Called during/after each step of validation
* @param data The data to send to the renderer
Expand All @@ -54,13 +43,11 @@ export class ComfyInstallation {
/** Installation state, e.g. `started`, `installed`. See {@link DesktopSettings}. */
public state: DesktopInstallState,
/** The base path of the desktop app. Models, nodes, and configuration are saved here by default. */
basePath: string,
public readonly basePath: string,
/** The device type to use for the installation. */
public readonly telemetry: ITelemetry,
public device?: TorchDeviceType
) {
// TypeScript workaround: duplication of basePath setter
this._basePath = basePath;
this.comfySettings = new ComfySettings(basePath);
this.virtualEnvironment = this.createVirtualEnvironment(basePath);
}
Expand Down Expand Up @@ -111,7 +98,7 @@ export class ComfyInstallation {
}

// Validate base path
const basePath = await this.loadBasePath();
const basePath = useDesktopConfig().get('basePath');
if (basePath && (await pathAccessible(basePath))) {
validation.basePath = 'OK';
this.onUpdate?.(validation);
Expand Down Expand Up @@ -175,35 +162,6 @@ export class ComfyInstallation {
return validation.installState;
}

/**
* Loads the base path from YAML config. If it is unreadable, warns the user and quits.
* @returns The base path if read successfully, or `undefined`
* @throws If the config file is present but not readable
*/
async loadBasePath(): Promise<string | undefined> {
const readResult = await ComfyServerConfig.readBasePathFromConfig(ComfyServerConfig.configPath);
switch (readResult.status) {
case 'success':
// TODO: Check if config.json basePath different, then determine why it has changed (intentional?)
this.basePath = readResult.path;
return readResult.path;
case 'invalid':
// TODO: File was there, and was valid YAML. It just didn't have a valid base_path.
// Show path edit screen instead of reinstall.
return;
case 'notFound':
return;
default:
// 'error': Explain and quit
// TODO: Support link? Something?
throw new Error(`Unable to read the YAML configuration file. Please ensure this file is available and can be read:

${ComfyServerConfig.configPath}

If this problem persists, back up and delete the config file, then restart the app.`);
}
}

/**
* Migrates the config file to the latest format, after an upgrade of the desktop app executables.
*
Expand Down
16 changes: 12 additions & 4 deletions tests/unit/install/installationManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type { AppWindow } from '@/main-process/appWindow';
import { ComfyInstallation } from '@/main-process/comfyInstallation';
import type { InstallValidation } from '@/preload';
import type { ITelemetry } from '@/services/telemetry';
import { useDesktopConfig } from '@/store/desktopConfig';
import * as utils from '@/utils';

vi.mock('electron', () => ({
Expand All @@ -20,7 +21,14 @@ vi.mock('node:fs/promises', () => ({
rm: vi.fn(),
}));

vi.mock('@/store/desktopConfig');
vi.mock('@/store/desktopConfig', () => ({
useDesktopConfig: vi.fn().mockReturnValue({
get: vi.fn().mockImplementation((key: string) => {
if (key === 'installState') return 'installed';
if (key === 'basePath') return 'valid/base';
}),
}),
}));
vi.mock('electron-log/main');

vi.mock('@/utils', async () => {
Expand Down Expand Up @@ -123,9 +131,9 @@ describe('InstallationManager', () => {
{
scenario: 'detects invalid base path',
mockSetup: () => {
vi.mocked(ComfyServerConfig.readBasePathFromConfig).mockResolvedValue({
status: 'success',
path: 'invalid/base',
vi.mocked(useDesktopConfig().get).mockImplementation((key: string) => {
if (key === 'installState') return 'installed';
if (key === 'basePath') return 'invalid/base';
});
},
expectedErrors: ['basePath'],
Expand Down
Loading