From 157a1a24f6c2b720be3e76c9b00bdb17f32e1b18 Mon Sep 17 00:00:00 2001 From: "sp.wack" <83104063+amanape@users.noreply.github.com> Date: Fri, 10 Jan 2025 20:54:31 +0400 Subject: [PATCH 1/2] fix(frontend): Wait for fetched settings instead of loading default ones (#6193) --- .../features/sidebar/sidebar.test.tsx | 103 +++++++++++++++++- .../account-settings-context-menu.tsx | 5 +- .../components/features/sidebar/sidebar.tsx | 13 ++- .../account-settings-form.tsx | 11 +- .../account-settings-modal.tsx | 2 +- .../shared/modals/settings/settings-form.tsx | 10 +- frontend/src/context/settings-context.tsx | 70 ++++++++++++ .../context/settings-up-to-date-context.tsx | 40 ------- frontend/src/entry.client.tsx | 10 +- .../src/hooks/mutation/use-save-settings.ts | 16 +-- frontend/src/hooks/query/use-settings.ts | 1 - .../src/hooks/use-maybe-migrate-settings.ts | 4 +- frontend/src/mocks/handlers.ts | 8 +- frontend/src/routes/_oh.app/route.tsx | 14 ++- frontend/src/routes/_oh/route.tsx | 4 +- frontend/test-utils.tsx | 22 ++-- 16 files changed, 230 insertions(+), 103 deletions(-) create mode 100644 frontend/src/context/settings-context.tsx delete mode 100644 frontend/src/context/settings-up-to-date-context.tsx diff --git a/frontend/__tests__/components/features/sidebar/sidebar.test.tsx b/frontend/__tests__/components/features/sidebar/sidebar.test.tsx index 1e80607301b0..9d96e7a49bdc 100644 --- a/frontend/__tests__/components/features/sidebar/sidebar.test.tsx +++ b/frontend/__tests__/components/features/sidebar/sidebar.test.tsx @@ -1,10 +1,12 @@ -import { screen } from "@testing-library/react"; +import { screen, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { renderWithProviders } from "test-utils"; import { createRoutesStub } from "react-router"; import { Sidebar } from "#/components/features/sidebar/sidebar"; import { MULTI_CONVERSATION_UI } from "#/utils/feature-flags"; +import OpenHands from "#/api/open-hands"; +import { MOCK_USER_PREFERENCES } from "#/mocks/handlers"; const renderSidebar = () => { const RouterStub = createRoutesStub([ @@ -43,4 +45,101 @@ describe("Sidebar", () => { ).not.toBeInTheDocument(); }, ); + + describe("Settings", () => { + const getSettingsSpy = vi.spyOn(OpenHands, "getSettings"); + const saveSettingsSpy = vi.spyOn(OpenHands, "saveSettings"); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it("should fetch settings data on mount", () => { + renderSidebar(); + expect(getSettingsSpy).toHaveBeenCalledOnce(); + }); + + it("should send all settings data when saving AI configuration", async () => { + const user = userEvent.setup(); + renderSidebar(); + + const settingsButton = screen.getByTestId("settings-button"); + await user.click(settingsButton); + + const settingsModal = screen.getByTestId("ai-config-modal"); + const saveButton = within(settingsModal).getByTestId( + "save-settings-button", + ); + await user.click(saveButton); + + expect(saveSettingsSpy).toHaveBeenCalledWith({ + ...MOCK_USER_PREFERENCES.settings, + // the actual values are falsey (null or "") but we're checking for undefined + llm_api_key: undefined, + llm_base_url: undefined, + security_analyzer: undefined, + }); + }); + + it("should send all settings data when saving account settings", async () => { + const user = userEvent.setup(); + renderSidebar(); + + const userAvatar = screen.getByTestId("user-avatar"); + await user.click(userAvatar); + + const menu = screen.getByTestId("account-settings-context-menu"); + const accountSettingsButton = within(menu).getByTestId( + "account-settings-button", + ); + await user.click(accountSettingsButton); + + const accountSettingsModal = screen.getByTestId("account-settings-form"); + const saveButton = + within(accountSettingsModal).getByTestId("save-settings"); + await user.click(saveButton); + + expect(saveSettingsSpy).toHaveBeenCalledWith({ + ...MOCK_USER_PREFERENCES.settings, + llm_api_key: undefined, // null or undefined + }); + }); + + it("should not reset AI configuration when saving account settings", async () => { + const user = userEvent.setup(); + renderSidebar(); + + const userAvatar = screen.getByTestId("user-avatar"); + await user.click(userAvatar); + + const menu = screen.getByTestId("account-settings-context-menu"); + const accountSettingsButton = within(menu).getByTestId( + "account-settings-button", + ); + await user.click(accountSettingsButton); + + const accountSettingsModal = screen.getByTestId("account-settings-form"); + + const languageInput = + within(accountSettingsModal).getByLabelText(/language/i); + await user.click(languageInput); + + const norskOption = screen.getByText(/norsk/i); + await user.click(norskOption); + + const tokenInput = + within(accountSettingsModal).getByLabelText(/github token/i); + await user.type(tokenInput, "new-token"); + + const saveButton = + within(accountSettingsModal).getByTestId("save-settings"); + await user.click(saveButton); + + expect(saveSettingsSpy).toHaveBeenCalledWith({ + ...MOCK_USER_PREFERENCES.settings, + language: "no", + llm_api_key: undefined, // null or undefined + }); + }); + }); }); diff --git a/frontend/src/components/features/context-menu/account-settings-context-menu.tsx b/frontend/src/components/features/context-menu/account-settings-context-menu.tsx index bf695cbc6ba7..92a3359e83c9 100644 --- a/frontend/src/components/features/context-menu/account-settings-context-menu.tsx +++ b/frontend/src/components/features/context-menu/account-settings-context-menu.tsx @@ -27,7 +27,10 @@ export function AccountSettingsContextMenu({ ref={ref} className="absolute left-full -top-1 z-10" > - + {t(I18nKey.ACCOUNT_SETTINGS$SETTINGS)} diff --git a/frontend/src/components/features/sidebar/sidebar.tsx b/frontend/src/components/features/sidebar/sidebar.tsx index f6a1728ce5ed..cee990f9e963 100644 --- a/frontend/src/components/features/sidebar/sidebar.tsx +++ b/frontend/src/components/features/sidebar/sidebar.tsx @@ -12,7 +12,7 @@ import { SettingsButton } from "#/components/shared/buttons/settings-button"; import { LoadingSpinner } from "#/components/shared/loading-spinner"; import { AccountSettingsModal } from "#/components/shared/modals/account-settings/account-settings-modal"; import { SettingsModal } from "#/components/shared/modals/settings/settings-modal"; -import { useSettingsUpToDate } from "#/context/settings-up-to-date-context"; +import { useCurrentSettings } from "#/context/settings-context"; import { useSettings } from "#/hooks/query/use-settings"; import { ConversationPanel } from "../conversation-panel/conversation-panel"; import { MULTI_CONVERSATION_UI } from "#/utils/feature-flags"; @@ -28,8 +28,13 @@ export function Sidebar() { const user = useGitHubUser(); const { data: isAuthed } = useIsAuthed(); const { logout } = useAuth(); - const { data: settings, isError: settingsIsError } = useSettings(); - const { isUpToDate: settingsAreUpToDate } = useSettingsUpToDate(); + const { + data: settings, + isError: settingsIsError, + isSuccess: settingsSuccessfulyFetched, + } = useSettings(); + + const { isUpToDate: settingsAreUpToDate } = useCurrentSettings(); const [accountSettingsModalOpen, setAccountSettingsModalOpen] = React.useState(false); @@ -106,7 +111,7 @@ export function Sidebar() { )} {settingsIsError || - (showSettingsModal && ( + (showSettingsModal && settingsSuccessfulyFetched && ( setSettingsModalIsOpen(false)} diff --git a/frontend/src/components/shared/modals/account-settings/account-settings-form.tsx b/frontend/src/components/shared/modals/account-settings/account-settings-form.tsx index fb162103eb5f..6e1363274944 100644 --- a/frontend/src/components/shared/modals/account-settings/account-settings-form.tsx +++ b/frontend/src/components/shared/modals/account-settings/account-settings-form.tsx @@ -13,7 +13,7 @@ import { ModalButton } from "../../buttons/modal-button"; import { CustomInput } from "../../custom-input"; import { FormFieldset } from "../../form-fieldset"; import { useConfig } from "#/hooks/query/use-config"; -import { useSaveSettings } from "#/hooks/mutation/use-save-settings"; +import { useCurrentSettings } from "#/context/settings-context"; interface AccountSettingsFormProps { onClose: () => void; @@ -30,10 +30,10 @@ export function AccountSettingsForm({ }: AccountSettingsFormProps) { const { gitHubToken, setGitHubToken, logout } = useAuth(); const { data: config } = useConfig(); - const { mutate: saveSettings } = useSaveSettings(); + const { saveUserSettings } = useCurrentSettings(); const { t } = useTranslation(); - const handleSubmit = (event: React.FormEvent) => { + const handleSubmit = async (event: React.FormEvent) => { event.preventDefault(); const formData = new FormData(event.currentTarget); @@ -50,7 +50,7 @@ export function AccountSettingsForm({ ({ label }) => label === language, )?.value; - if (languageKey) saveSettings({ LANGUAGE: languageKey }); + if (languageKey) await saveUserSettings({ LANGUAGE: languageKey }); } handleCaptureConsent(analytics); @@ -61,7 +61,7 @@ export function AccountSettingsForm({ }; return ( - +
@@ -137,6 +137,7 @@ export function AccountSettingsForm({
diff --git a/frontend/src/components/shared/modals/settings/settings-form.tsx b/frontend/src/components/shared/modals/settings/settings-form.tsx index 1265d6035514..b60bba79d7f3 100644 --- a/frontend/src/components/shared/modals/settings/settings-form.tsx +++ b/frontend/src/components/shared/modals/settings/settings-form.tsx @@ -19,10 +19,10 @@ import { CustomModelInput } from "../../inputs/custom-model-input"; import { SecurityAnalyzerInput } from "../../inputs/security-analyzers-input"; import { ModalBackdrop } from "../modal-backdrop"; import { ModelSelector } from "./model-selector"; -import { useSaveSettings } from "#/hooks/mutation/use-save-settings"; import { RuntimeSizeSelector } from "./runtime-size-selector"; import { useConfig } from "#/hooks/query/use-config"; +import { useCurrentSettings } from "#/context/settings-context"; interface SettingsFormProps { disabled?: boolean; @@ -41,7 +41,7 @@ export function SettingsForm({ securityAnalyzers, onClose, }: SettingsFormProps) { - const { mutateAsync: saveSettings } = useSaveSettings(); + const { saveUserSettings } = useCurrentSettings(); const endSession = useEndSession(); const { data: config } = useConfig(); @@ -95,7 +95,8 @@ export function SettingsForm({ const newSettings = extractSettings(formData); saveSettingsView(isUsingAdvancedOptions ? "advanced" : "basic"); - await saveSettings(newSettings, { onSuccess: onClose }); + await saveUserSettings(newSettings); + onClose(); resetOngoingSession(); posthog.capture("settings_saved", { @@ -107,7 +108,8 @@ export function SettingsForm({ }; const handleConfirmResetSettings = async () => { - await saveSettings(getDefaultSettings(), { onSuccess: onClose }); + await saveUserSettings(getDefaultSettings()); + onClose(); resetOngoingSession(); posthog.capture("settings_reset"); }; diff --git a/frontend/src/context/settings-context.tsx b/frontend/src/context/settings-context.tsx new file mode 100644 index 000000000000..4b85d2e27784 --- /dev/null +++ b/frontend/src/context/settings-context.tsx @@ -0,0 +1,70 @@ +import React from "react"; +import { + LATEST_SETTINGS_VERSION, + Settings, + settingsAreUpToDate, +} from "#/services/settings"; +import { useSettings } from "#/hooks/query/use-settings"; +import { useSaveSettings } from "#/hooks/mutation/use-save-settings"; + +interface SettingsContextType { + isUpToDate: boolean; + setIsUpToDate: (value: boolean) => void; + saveUserSettings: (newSettings: Partial) => Promise; + settings: Settings | undefined; +} + +const SettingsContext = React.createContext( + undefined, +); + +interface SettingsProviderProps { + children: React.ReactNode; +} + +export function SettingsProvider({ children }: SettingsProviderProps) { + const { data: userSettings } = useSettings(); + const { mutateAsync: saveSettings } = useSaveSettings(); + + const [isUpToDate, setIsUpToDate] = React.useState(settingsAreUpToDate()); + + const saveUserSettings = async (newSettings: Partial) => { + const updatedSettings: Partial = { + ...userSettings, + ...newSettings, + }; + await saveSettings(updatedSettings, { + onSuccess: () => { + if (!isUpToDate) { + localStorage.setItem( + "SETTINGS_VERSION", + LATEST_SETTINGS_VERSION.toString(), + ); + setIsUpToDate(true); + } + }, + }); + }; + + const value = React.useMemo( + () => ({ + isUpToDate, + setIsUpToDate, + saveUserSettings, + settings: userSettings, + }), + [isUpToDate, setIsUpToDate, saveUserSettings, userSettings], + ); + + return {children}; +} + +export function useCurrentSettings() { + const context = React.useContext(SettingsContext); + if (context === undefined) { + throw new Error( + "useCurrentSettings must be used within a SettingsProvider", + ); + } + return context; +} diff --git a/frontend/src/context/settings-up-to-date-context.tsx b/frontend/src/context/settings-up-to-date-context.tsx deleted file mode 100644 index e4a6341e262e..000000000000 --- a/frontend/src/context/settings-up-to-date-context.tsx +++ /dev/null @@ -1,40 +0,0 @@ -import React from "react"; -import { settingsAreUpToDate } from "#/services/settings"; - -interface SettingsUpToDateContextType { - isUpToDate: boolean; - setIsUpToDate: (value: boolean) => void; -} - -const SettingsUpToDateContext = React.createContext< - SettingsUpToDateContextType | undefined ->(undefined); - -interface SettingsUpToDateProviderProps { - children: React.ReactNode; -} - -export function SettingsUpToDateProvider({ - children, -}: SettingsUpToDateProviderProps) { - const [isUpToDate, setIsUpToDate] = React.useState(settingsAreUpToDate()); - - const value = React.useMemo( - () => ({ isUpToDate, setIsUpToDate }), - [isUpToDate, setIsUpToDate], - ); - - return ( - {children} - ); -} - -export function useSettingsUpToDate() { - const context = React.useContext(SettingsUpToDateContext); - if (context === undefined) { - throw new Error( - "useSettingsUpToDate must be used within a SettingsUpToDateProvider", - ); - } - return context; -} diff --git a/frontend/src/entry.client.tsx b/frontend/src/entry.client.tsx index ba17326b4699..47f0090489cf 100644 --- a/frontend/src/entry.client.tsx +++ b/frontend/src/entry.client.tsx @@ -20,7 +20,7 @@ import toast from "react-hot-toast"; import store from "./store"; import { useConfig } from "./hooks/query/use-config"; import { AuthProvider } from "./context/auth-context"; -import { SettingsUpToDateProvider } from "./context/settings-up-to-date-context"; +import { SettingsProvider } from "./context/settings-context"; function PosthogInit() { const { data: config } = useConfig(); @@ -79,12 +79,12 @@ prepareApp().then(() => - - + + - - + + , diff --git a/frontend/src/hooks/mutation/use-save-settings.ts b/frontend/src/hooks/mutation/use-save-settings.ts index f9731e981d5b..ef334b0f1db8 100644 --- a/frontend/src/hooks/mutation/use-save-settings.ts +++ b/frontend/src/hooks/mutation/use-save-settings.ts @@ -1,12 +1,6 @@ import { useMutation, useQueryClient } from "@tanstack/react-query"; -import { - ApiSettings, - DEFAULT_SETTINGS, - LATEST_SETTINGS_VERSION, - Settings, -} from "#/services/settings"; +import { ApiSettings, DEFAULT_SETTINGS, Settings } from "#/services/settings"; import OpenHands from "#/api/open-hands"; -import { useSettingsUpToDate } from "#/context/settings-up-to-date-context"; const saveSettingsMutationFn = async (settings: Partial) => { const apiSettings: Partial = { @@ -24,19 +18,11 @@ const saveSettingsMutationFn = async (settings: Partial) => { export const useSaveSettings = () => { const queryClient = useQueryClient(); - const { isUpToDate, setIsUpToDate } = useSettingsUpToDate(); return useMutation({ mutationFn: saveSettingsMutationFn, onSuccess: async () => { await queryClient.invalidateQueries({ queryKey: ["settings"] }); - if (!isUpToDate) { - localStorage.setItem( - "SETTINGS_VERSION", - LATEST_SETTINGS_VERSION.toString(), - ); - setIsUpToDate(true); - } }, }); }; diff --git a/frontend/src/hooks/query/use-settings.ts b/frontend/src/hooks/query/use-settings.ts index a4d430ccf88d..3daa8c8780b7 100644 --- a/frontend/src/hooks/query/use-settings.ts +++ b/frontend/src/hooks/query/use-settings.ts @@ -39,7 +39,6 @@ export const useSettings = () => { const query = useQuery({ queryKey: ["settings"], queryFn: getSettingsQueryFn, - initialData: DEFAULT_SETTINGS, }); React.useEffect(() => { diff --git a/frontend/src/hooks/use-maybe-migrate-settings.ts b/frontend/src/hooks/use-maybe-migrate-settings.ts index 4f5bbd4712a0..a2da7ef47be5 100644 --- a/frontend/src/hooks/use-maybe-migrate-settings.ts +++ b/frontend/src/hooks/use-maybe-migrate-settings.ts @@ -1,7 +1,7 @@ // Sometimes we ship major changes, like a new default agent. import React from "react"; -import { useSettingsUpToDate } from "#/context/settings-up-to-date-context"; +import { useCurrentSettings } from "#/context/settings-context"; import { getCurrentSettingsVersion, DEFAULT_SETTINGS, @@ -12,7 +12,7 @@ import { useSaveSettings } from "./mutation/use-save-settings"; // In this case, we may want to override a previous choice made by the user. export const useMaybeMigrateSettings = () => { const { mutateAsync: saveSettings } = useSaveSettings(); - const { isUpToDate } = useSettingsUpToDate(); + const { isUpToDate } = useCurrentSettings(); const maybeMigrateSettings = async () => { const currentVersion = getCurrentSettingsVersion(); diff --git a/frontend/src/mocks/handlers.ts b/frontend/src/mocks/handlers.ts index f24c4a55ddae..9d1a42d4305d 100644 --- a/frontend/src/mocks/handlers.ts +++ b/frontend/src/mocks/handlers.ts @@ -6,7 +6,7 @@ import { } from "#/api/open-hands.types"; import { DEFAULT_SETTINGS } from "#/services/settings"; -const userPreferences = { +export const MOCK_USER_PREFERENCES = { settings: { llm_model: DEFAULT_SETTINGS.LLM_MODEL, llm_base_url: DEFAULT_SETTINGS.LLM_BASE_URL, @@ -169,14 +169,14 @@ export const handlers = [ return HttpResponse.json(config); }), http.get("/api/settings", async () => - HttpResponse.json(userPreferences.settings), + HttpResponse.json(MOCK_USER_PREFERENCES.settings), ), http.post("/api/settings", async ({ request }) => { const body = await request.json(); if (body) { - userPreferences.settings = { - ...userPreferences.settings, + MOCK_USER_PREFERENCES.settings = { + ...MOCK_USER_PREFERENCES.settings, // @ts-expect-error - We know this is a settings object ...body, }; diff --git a/frontend/src/routes/_oh.app/route.tsx b/frontend/src/routes/_oh.app/route.tsx index 182a7a98e57c..701d6b3edb1e 100644 --- a/frontend/src/routes/_oh.app/route.tsx +++ b/frontend/src/routes/_oh.app/route.tsx @@ -176,13 +176,15 @@ function AppContent() { - + {settings && ( + + )}
diff --git a/frontend/src/routes/_oh/route.tsx b/frontend/src/routes/_oh/route.tsx index 16492b533343..ca5868d15572 100644 --- a/frontend/src/routes/_oh/route.tsx +++ b/frontend/src/routes/_oh/route.tsx @@ -63,10 +63,10 @@ export default function MainApp() { }); React.useEffect(() => { - if (settings.LANGUAGE) { + if (settings?.LANGUAGE) { i18n.changeLanguage(settings.LANGUAGE); } - }, [settings.LANGUAGE]); + }, [settings?.LANGUAGE]); const isInWaitlist = !isFetchingAuth && !isAuthed && config.data?.APP_MODE === "saas"; diff --git a/frontend/test-utils.tsx b/frontend/test-utils.tsx index 8eeb5e453fb6..42bd9ec6e0d9 100644 --- a/frontend/test-utils.tsx +++ b/frontend/test-utils.tsx @@ -11,7 +11,7 @@ import { vi } from "vitest"; import { AppStore, RootState, rootReducer } from "./src/store"; import { AuthProvider } from "#/context/auth-context"; import { ConversationProvider } from "#/context/conversation-context"; -import { SettingsUpToDateProvider } from "#/context/settings-up-to-date-context"; +import { SettingsProvider } from "#/context/settings-context"; // Mock useParams before importing components vi.mock("react-router", async () => { @@ -67,19 +67,19 @@ export function renderWithProviders( return ( - - + + {children} - - + + ); From 3876a00aa574d12075807268c0fd1427831b475a Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Fri, 10 Jan 2025 12:31:58 -0500 Subject: [PATCH 2/2] consolidate runtime startup command into an util function --- .../runtime/impl/docker/docker_runtime.py | 31 +++------- openhands/runtime/impl/modal/modal_runtime.py | 26 ++------- .../runtime/impl/remote/remote_runtime.py | 22 ++----- .../runtime/impl/runloop/runloop_runtime.py | 28 ++------- openhands/runtime/utils/command.py | 57 +++++++++++++------ 5 files changed, 62 insertions(+), 102 deletions(-) diff --git a/openhands/runtime/impl/docker/docker_runtime.py b/openhands/runtime/impl/docker/docker_runtime.py index 6eb51418b733..4945e6620649 100644 --- a/openhands/runtime/impl/docker/docker_runtime.py +++ b/openhands/runtime/impl/docker/docker_runtime.py @@ -21,6 +21,7 @@ from openhands.runtime.impl.docker.containers import remove_all_containers from openhands.runtime.plugins import PluginRequirement from openhands.runtime.utils import find_available_tcp_port +from openhands.runtime.utils.command import get_action_execution_server_startup_command from openhands.runtime.utils.log_streamer import LogStreamer from openhands.runtime.utils.runtime_build import build_runtime_image from openhands.utils.async_utils import call_sync_from_async @@ -186,11 +187,7 @@ def _init_docker_client() -> docker.DockerClient: def _init_container(self): self.log('debug', 'Preparing to start container...') self.send_status_message('STATUS$PREPARING_CONTAINER') - plugin_arg = '' - if self.plugins is not None and len(self.plugins) > 0: - plugin_arg = ( - f'--plugins {" ".join([plugin.name for plugin in self.plugins])} ' - ) + self._host_port = self._find_available_port(EXECUTION_SERVER_PORT_RANGE) self._container_port = self._host_port self._vscode_port = self._find_available_port(VSCODE_PORT_RANGE) @@ -203,8 +200,6 @@ def _init_container(self): use_host_network = self.config.sandbox.use_host_network network_mode: str | None = 'host' if use_host_network else None - use_host_network = self.config.sandbox.use_host_network - # Initialize port mappings port_mapping: dict[str, list[dict[str, str]]] | None = None if not use_host_network: @@ -257,26 +252,16 @@ def _init_container(self): f'Sandbox workspace: {self.config.workspace_mount_path_in_sandbox}', ) - if self.config.sandbox.browsergym_eval_env is not None: - browsergym_arg = ( - f'--browsergym-eval-env {self.config.sandbox.browsergym_eval_env}' - ) - else: - browsergym_arg = '' + command = get_action_execution_server_startup_command( + server_port=self._container_port, + plugins=self.plugins, + app_config=self.config, + ) try: self.container = self.docker_client.containers.run( self.runtime_container_image, - command=( - f'/openhands/micromamba/bin/micromamba run -n openhands ' - f'poetry run ' - f'python -u -m openhands.runtime.action_execution_server {self._container_port} ' - f'--working-dir "{self.config.workspace_mount_path_in_sandbox}" ' - f'{plugin_arg}' - f'--username {"openhands" if self.config.run_as_openhands else "root"} ' - f'--user-id {self.config.sandbox.user_id} ' - f'{browsergym_arg}' - ), + command=command, network_mode=network_mode, ports=port_mapping, working_dir='/openhands/code/', # do not change this! diff --git a/openhands/runtime/impl/modal/modal_runtime.py b/openhands/runtime/impl/modal/modal_runtime.py index 473c4ae97b10..6c2be0739615 100644 --- a/openhands/runtime/impl/modal/modal_runtime.py +++ b/openhands/runtime/impl/modal/modal_runtime.py @@ -13,7 +13,7 @@ ActionExecutionClient, ) from openhands.runtime.plugins import PluginRequirement -from openhands.runtime.utils.command import get_remote_startup_command +from openhands.runtime.utils.command import get_action_execution_server_startup_command from openhands.runtime.utils.runtime_build import ( BuildFromImageType, prep_build_folder, @@ -203,11 +203,6 @@ def _init_sandbox( ): try: self.log('debug', 'Preparing to start container...') - plugin_args = [] - if plugins is not None and len(plugins) > 0: - plugin_args.append('--plugins') - plugin_args.extend([plugin.name for plugin in plugins]) - # Combine environment variables environment: dict[str, str | None] = { 'port': str(self.container_port), @@ -216,24 +211,13 @@ def _init_sandbox( if self.config.debug: environment['DEBUG'] = 'true' - browsergym_args = [] - if self.config.sandbox.browsergym_eval_env is not None: - browsergym_args = [ - '-browsergym-eval-env', - self.config.sandbox.browsergym_eval_env, - ] - env_secret = modal.Secret.from_dict(environment) self.log('debug', f'Sandbox workspace: {sandbox_workspace_dir}') - sandbox_start_cmd = get_remote_startup_command( - self.container_port, - sandbox_workspace_dir, - 'openhands' if self.config.run_as_openhands else 'root', - self.config.sandbox.user_id, - plugin_args, - browsergym_args, - is_root=not self.config.run_as_openhands, # is_root=True when running as root + sandbox_start_cmd = get_action_execution_server_startup_command( + server_port=self.container_port, + plugins=self.plugins, + app_config=self.config, ) self.log('debug', f'Starting container with command: {sandbox_start_cmd}') self.sandbox = modal.Sandbox.create( diff --git a/openhands/runtime/impl/remote/remote_runtime.py b/openhands/runtime/impl/remote/remote_runtime.py index 0e0b7adc79e6..ebc1a86b384b 100644 --- a/openhands/runtime/impl/remote/remote_runtime.py +++ b/openhands/runtime/impl/remote/remote_runtime.py @@ -19,7 +19,7 @@ ActionExecutionClient, ) from openhands.runtime.plugins import PluginRequirement -from openhands.runtime.utils.command import get_remote_startup_command +from openhands.runtime.utils.command import get_action_execution_server_startup_command from openhands.runtime.utils.request import send_request from openhands.runtime.utils.runtime_build import build_runtime_image from openhands.utils.async_utils import call_sync_from_async @@ -194,22 +194,10 @@ def _build_runtime(self): def _start_runtime(self): # Prepare the request body for the /start endpoint - plugin_args = [] - if self.plugins is not None and len(self.plugins) > 0: - plugin_args = ['--plugins'] + [plugin.name for plugin in self.plugins] - browsergym_args = [] - if self.config.sandbox.browsergym_eval_env is not None: - browsergym_args = [ - '--browsergym-eval-env' - ] + self.config.sandbox.browsergym_eval_env.split(' ') - command = get_remote_startup_command( - self.port, - self.config.workspace_mount_path_in_sandbox, - 'openhands' if self.config.run_as_openhands else 'root', - self.config.sandbox.user_id, - plugin_args, - browsergym_args, - is_root=not self.config.run_as_openhands, # is_root=True when running as root + command = get_action_execution_server_startup_command( + server_port=self.port, + plugins=self.plugins, + app_config=self.config, ) start_request = { 'image': self.container_image, diff --git a/openhands/runtime/impl/runloop/runloop_runtime.py b/openhands/runtime/impl/runloop/runloop_runtime.py index 93f019561ff0..51628f54056d 100644 --- a/openhands/runtime/impl/runloop/runloop_runtime.py +++ b/openhands/runtime/impl/runloop/runloop_runtime.py @@ -13,7 +13,7 @@ ActionExecutionClient, ) from openhands.runtime.plugins import PluginRequirement -from openhands.runtime.utils.command import get_remote_startup_command +from openhands.runtime.utils.command import get_action_execution_server_startup_command from openhands.utils.tenacity_stop import stop_if_should_exit CONTAINER_NAME_PREFIX = 'openhands-runtime-' @@ -78,28 +78,10 @@ def _wait_for_devbox(self, devbox: DevboxView) -> DevboxView: def _create_new_devbox(self) -> DevboxView: # Note: Runloop connect - sandbox_workspace_dir = self.config.workspace_mount_path_in_sandbox - plugin_args = [] - if self.plugins is not None and len(self.plugins) > 0: - plugin_args.append('--plugins') - plugin_args.extend([plugin.name for plugin in self.plugins]) - - browsergym_args = [] - if self.config.sandbox.browsergym_eval_env is not None: - browsergym_args = [ - '-browsergym-eval-env', - self.config.sandbox.browsergym_eval_env, - ] - - # Copied from EventstreamRuntime - start_command = get_remote_startup_command( - self._sandbox_port, - sandbox_workspace_dir, - 'openhands' if self.config.run_as_openhands else 'root', - self.config.sandbox.user_id, - plugin_args, - browsergym_args, - is_root=not self.config.run_as_openhands, # is_root=True when running as root + start_command = get_action_execution_server_startup_command( + server_port=self._sandbox_port, + plugins=self.plugins, + app_config=self.config, ) # Add some additional commands based on our image diff --git a/openhands/runtime/utils/command.py b/openhands/runtime/utils/command.py index 3a32d45fb7e1..415f9fcb7f78 100644 --- a/openhands/runtime/utils/command.py +++ b/openhands/runtime/utils/command.py @@ -1,31 +1,52 @@ -def get_remote_startup_command( - port: int, - sandbox_workspace_dir: str, - username: str, - user_id: int, - plugin_args: list[str], - browsergym_args: list[str], - is_root: bool = False, +from openhands.core.config import AppConfig +from openhands.runtime.plugins import PluginRequirement + +DEFAULT_PYTHON_PREFIX = [ + '/openhands/micromamba/bin/micromamba', + 'run', + '-n', + 'openhands', + 'poetry', + 'run', +] + + +def get_action_execution_server_startup_command( + server_port: int, + plugins: list[PluginRequirement], + app_config: AppConfig, + python_prefix: list[str] = DEFAULT_PYTHON_PREFIX, ): + sandbox_config = app_config.sandbox + + # Plugin args + plugin_args = [] + if plugins is not None and len(plugins) > 0: + plugin_args = ['--plugins'] + [plugin.name for plugin in plugins] + + # Browsergym stuffs + browsergym_args = [] + if sandbox_config.browsergym_eval_env is not None: + browsergym_args = [ + '--browsergym-eval-env' + ] + sandbox_config.browsergym_eval_env.split(' ') + + is_root = not app_config.run_as_openhands + base_cmd = [ - '/openhands/micromamba/bin/micromamba', - 'run', - '-n', - 'openhands', - 'poetry', - 'run', + *python_prefix, 'python', '-u', '-m', 'openhands.runtime.action_execution_server', - str(port), + str(server_port), '--working-dir', - sandbox_workspace_dir, + app_config.workspace_mount_path_in_sandbox, *plugin_args, '--username', - username, + 'openhands' if app_config.run_as_openhands else 'root', '--user-id', - str(user_id), + str(sandbox_config.user_id), *browsergym_args, ]