fix(eval): thread python.interpreter from session settings and expand ~
Resolve the explicit interpreter from the session's Settings instance (ToolSession.settings / AgentSession.settings) instead of re-reading the process-global Settings.init() singleton, so project-scoped and cloned session settings take effect. The availability cache is now keyed by cwd + interpreter, and PythonKernel.start/executePython accept the resolved interpreter as an option. Also expand home-relative paths (~/...) before resolving against cwd, and document the contract of resolveExplicitPythonRuntime. Addresses review feedback on #2204.
This commit is contained in:
@@ -42,6 +42,11 @@ export interface PythonExecutorOptions {
|
||||
kernelOwnerId?: string;
|
||||
/** Kernel mode (session reuse vs per-call) */
|
||||
kernelMode?: PythonKernelMode;
|
||||
/**
|
||||
* Explicit interpreter path (`python.interpreter` resolved from the
|
||||
* session's settings). Skips automatic runtime discovery when set.
|
||||
*/
|
||||
interpreter?: string;
|
||||
/** Restart the kernel before executing */
|
||||
reset?: boolean;
|
||||
/** Session file path for accessing task outputs */
|
||||
@@ -326,6 +331,7 @@ async function startKernel(cwd: string, options: PythonExecutorOptions): Promise
|
||||
env: buildKernelEnv(options),
|
||||
signal: options.signal,
|
||||
deadlineMs: options.deadlineMs,
|
||||
interpreter: options.interpreter,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -587,7 +593,10 @@ async function executeWithKernel(
|
||||
}
|
||||
|
||||
async function ensureKernelAvailable(cwd: string, options: PythonExecutorOptions): Promise<void> {
|
||||
const availability = await waitForPromiseWithCancellation(checkPythonKernelAvailability(cwd), options);
|
||||
const availability = await waitForPromiseWithCancellation(
|
||||
checkPythonKernelAvailability(cwd, options.interpreter),
|
||||
options,
|
||||
);
|
||||
if (!availability.ok) {
|
||||
throw new Error(availability.reason ?? "Python kernel unavailable");
|
||||
}
|
||||
|
||||
@@ -19,13 +19,17 @@ function readSetting<T>(session: ToolSession, key: string): T | undefined {
|
||||
return settings?.get?.(key);
|
||||
}
|
||||
|
||||
function readInterpreterSetting(session: ToolSession): string | undefined {
|
||||
return readSetting<string>(session, "python.interpreter")?.trim() || undefined;
|
||||
}
|
||||
|
||||
export default {
|
||||
id: "python",
|
||||
label: "Python",
|
||||
highlightLang: "python",
|
||||
|
||||
async isAvailable(session: ToolSession): Promise<boolean> {
|
||||
const availability = await checkPythonKernelAvailability(session.cwd);
|
||||
const availability = await checkPythonKernelAvailability(session.cwd, readInterpreterSetting(session));
|
||||
return availability.ok;
|
||||
},
|
||||
|
||||
@@ -37,6 +41,7 @@ export default {
|
||||
signal: opts.signal,
|
||||
sessionId: namespaceSessionId(opts.sessionId),
|
||||
kernelMode,
|
||||
interpreter: readInterpreterSetting(opts.session),
|
||||
sessionFile: opts.sessionFile,
|
||||
artifactsDir: opts.session.getArtifactsDir?.() ?? undefined,
|
||||
localRoots: resolveEvalUrlRoots(opts.session),
|
||||
|
||||
@@ -102,6 +102,11 @@ interface KernelLifecycleOptions {
|
||||
interface KernelStartOptions extends KernelLifecycleOptions {
|
||||
cwd: string;
|
||||
env?: Record<string, string | undefined>;
|
||||
/**
|
||||
* Explicit interpreter path (`python.interpreter` from the session's
|
||||
* settings). When set, runtime discovery is skipped entirely.
|
||||
*/
|
||||
interpreter?: string;
|
||||
}
|
||||
|
||||
interface KernelShutdownOptions {
|
||||
@@ -135,20 +140,24 @@ function throwIfAborted(signal: AbortSignal | undefined, fallbackReason: string)
|
||||
throw createAbortError("AbortError", typeof reason === "string" ? reason : fallbackReason);
|
||||
}
|
||||
|
||||
// Cache successful probes per resolved cwd: every cell otherwise pays one (or
|
||||
// two — backend.isAvailable + ensureKernelAvailable) interpreter spawns even
|
||||
// when the kernel is already hot. Failures are not cached so installing a
|
||||
// Python mid-session is picked up on the next attempt.
|
||||
// Cache successful probes per resolved cwd + explicit interpreter: every cell
|
||||
// otherwise pays one (or two — backend.isAvailable + ensureKernelAvailable)
|
||||
// interpreter spawns even when the kernel is already hot. Failures are not
|
||||
// cached so installing a Python mid-session is picked up on the next attempt.
|
||||
const availabilityCache = new Map<string, Promise<PythonKernelAvailability>>();
|
||||
|
||||
export async function checkPythonKernelAvailability(cwd: string): Promise<PythonKernelAvailability> {
|
||||
export async function checkPythonKernelAvailability(
|
||||
cwd: string,
|
||||
interpreter?: string,
|
||||
): Promise<PythonKernelAvailability> {
|
||||
if (isBunTestRuntime() || $flag("PI_PYTHON_SKIP_CHECK")) {
|
||||
return { ok: true };
|
||||
}
|
||||
const key = path.resolve(cwd);
|
||||
const resolvedCwd = path.resolve(cwd);
|
||||
const key = `${resolvedCwd}\0${interpreter ?? ""}`;
|
||||
const cached = availabilityCache.get(key);
|
||||
if (cached) return await cached;
|
||||
const probe = probePythonKernelAvailability(key);
|
||||
const probe = probePythonKernelAvailability(resolvedCwd, interpreter);
|
||||
availabilityCache.set(key, probe);
|
||||
const result = await probe;
|
||||
if (!result.ok && availabilityCache.get(key) === probe) {
|
||||
@@ -157,14 +166,13 @@ export async function checkPythonKernelAvailability(cwd: string): Promise<Python
|
||||
return result;
|
||||
}
|
||||
|
||||
async function probePythonKernelAvailability(cwd: string): Promise<PythonKernelAvailability> {
|
||||
async function probePythonKernelAvailability(cwd: string, interpreter?: string): Promise<PythonKernelAvailability> {
|
||||
try {
|
||||
const settings = await Settings.init();
|
||||
const { env } = settings.getShellConfig();
|
||||
const baseEnv = filterEnv(env);
|
||||
const explicitInterpreter = settings.get("python.interpreter")?.trim();
|
||||
const runtimes = explicitInterpreter
|
||||
? [resolveExplicitPythonRuntime(explicitInterpreter, cwd, baseEnv)]
|
||||
const runtimes = interpreter
|
||||
? [resolveExplicitPythonRuntime(interpreter, cwd, baseEnv)]
|
||||
: enumeratePythonRuntimes(cwd, baseEnv);
|
||||
if (runtimes.length === 0) {
|
||||
return { ok: false, reason: "Python executable not found on PATH" };
|
||||
@@ -248,6 +256,7 @@ export class PythonKernel {
|
||||
"PythonKernel.start:availabilityCheck",
|
||||
checkPythonKernelAvailability,
|
||||
options.cwd,
|
||||
options.interpreter,
|
||||
);
|
||||
if (!availability.ok) {
|
||||
throw new Error(availability.reason ?? "Python kernel unavailable");
|
||||
@@ -259,11 +268,9 @@ export class PythonKernel {
|
||||
// PI_PYTHON_SKIP_CHECK), where no candidate was probed.
|
||||
let runtime = availability.runtime;
|
||||
if (!runtime) {
|
||||
const settings = await Settings.init();
|
||||
const { env: shellEnv } = settings.getShellConfig();
|
||||
const explicitInterpreter = settings.get("python.interpreter")?.trim();
|
||||
runtime = explicitInterpreter
|
||||
? resolveExplicitPythonRuntime(explicitInterpreter, options.cwd, filterEnv(shellEnv))
|
||||
const { env: shellEnv } = (await Settings.init()).getShellConfig();
|
||||
runtime = options.interpreter
|
||||
? resolveExplicitPythonRuntime(options.interpreter, options.cwd, filterEnv(shellEnv))
|
||||
: resolvePythonRuntime(options.cwd, filterEnv(shellEnv));
|
||||
}
|
||||
const spawnEnv: Record<string, string> = {};
|
||||
|
||||
@@ -5,6 +5,7 @@
|
||||
* for both the shared gateway and local kernel spawning.
|
||||
*/
|
||||
import * as fs from "node:fs";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import { $env, $which, getPythonEnvDir } from "@oh-my-pi/pi-utils";
|
||||
|
||||
@@ -191,18 +192,33 @@ function detectExplicitVenv(pythonPath: string): { venvPath: string; binDir: str
|
||||
return undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve an explicitly configured interpreter (`python.interpreter`) into a
|
||||
* runtime, bypassing discovery. Does not probe or validate the executable —
|
||||
* callers must check it actually runs. `~` expands to the home directory and
|
||||
* relative paths resolve against `cwd`. When the interpreter sits inside a
|
||||
* virtualenv (a `pyvenv.cfg` above its bin dir), the venv activation env is
|
||||
* applied so subprocesses and `pip` resolve consistently.
|
||||
*/
|
||||
export function resolveExplicitPythonRuntime(
|
||||
interpreter: string,
|
||||
cwd: string,
|
||||
baseEnv: Record<string, string | undefined>,
|
||||
): PythonRuntime {
|
||||
const pythonPath = path.isAbsolute(interpreter) ? interpreter : path.resolve(cwd, interpreter);
|
||||
const expanded =
|
||||
interpreter === "~"
|
||||
? os.homedir()
|
||||
: interpreter.startsWith("~/")
|
||||
? path.join(os.homedir(), interpreter.slice(2))
|
||||
: interpreter;
|
||||
const pythonPath = path.isAbsolute(expanded) ? expanded : path.resolve(cwd, expanded);
|
||||
const venv = detectExplicitVenv(pythonPath);
|
||||
if (venv) {
|
||||
return { pythonPath, env: applyVenvEnv(baseEnv, venv.venvPath, venv.binDir), venvPath: venv.venvPath };
|
||||
}
|
||||
return { pythonPath, env: { ...baseEnv } };
|
||||
}
|
||||
|
||||
/**
|
||||
* Enumerate candidate Python runtimes in priority order: an active/project venv,
|
||||
* the managed `~/.omp/python-env`, then the system interpreter on PATH. Every
|
||||
|
||||
@@ -8690,6 +8690,7 @@ export class AgentSession {
|
||||
sessionId: namespacePythonSessionId(sessionId),
|
||||
kernelOwnerId: this.#evalKernelOwnerId,
|
||||
kernelMode: this.settings.get("python.kernelMode"),
|
||||
interpreter: this.settings.get("python.interpreter")?.trim() || undefined,
|
||||
onChunk,
|
||||
signal: abortController.signal,
|
||||
});
|
||||
|
||||
@@ -463,7 +463,12 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P
|
||||
!allowJs &&
|
||||
(requestedTools === undefined || requestedTools.includes("eval"))
|
||||
) {
|
||||
const availability = await logger.time("createTools:pythonCheck", checkPythonKernelAvailability, session.cwd);
|
||||
const availability = await logger.time(
|
||||
"createTools:pythonCheck",
|
||||
checkPythonKernelAvailability,
|
||||
session.cwd,
|
||||
session.settings.get("python.interpreter")?.trim() || undefined,
|
||||
);
|
||||
pythonAvailable = availability.ok;
|
||||
if (!availability.ok) {
|
||||
logger.warn("Python kernel unavailable and JS backend disabled; eval will be unavailable", {
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { afterEach, describe, expect, it, vi } from "bun:test";
|
||||
import * as fs from "node:fs";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import {
|
||||
enumeratePythonRuntimes,
|
||||
@@ -126,6 +127,29 @@ describe("enumeratePythonRuntimes", () => {
|
||||
expect(runtime.env.VIRTUAL_ENV).toBe(venvDir);
|
||||
expect(runtime.env.PATH).toBe(`${binDir}${path.delimiter}${path.join(path.sep, "usr", "bin")}`);
|
||||
});
|
||||
|
||||
it("expands a home-relative explicit interpreter instead of resolving against cwd", () => {
|
||||
const home = path.join(path.sep, "home", "tester");
|
||||
vi.spyOn(os, "homedir").mockReturnValue(home);
|
||||
vi.spyOn(fs, "existsSync").mockReturnValue(false);
|
||||
|
||||
const runtime = resolveExplicitPythonRuntime("~/venvs/py/bin/python", path.join(path.sep, "work"), {});
|
||||
|
||||
expect(runtime.pythonPath).toBe(path.join(home, "venvs", "py", "bin", "python"));
|
||||
});
|
||||
|
||||
it("resolves a relative explicit interpreter against cwd", () => {
|
||||
vi.spyOn(fs, "existsSync").mockReturnValue(false);
|
||||
|
||||
const runtime = resolveExplicitPythonRuntime(
|
||||
path.join(".venv", "bin", "python"),
|
||||
path.join(path.sep, "work"),
|
||||
{},
|
||||
);
|
||||
|
||||
expect(runtime.pythonPath).toBe(path.join(path.sep, "work", ".venv", "bin", "python"));
|
||||
});
|
||||
|
||||
it("throws from resolvePythonRuntime when no interpreter can be found", () => {
|
||||
vi.spyOn(piUtils, "getPythonEnvDir").mockReturnValue(managedDir);
|
||||
vi.spyOn(piUtils, "$which").mockReturnValue(null);
|
||||
|
||||
Reference in New Issue
Block a user