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.

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

Add port mappings support #5577

Merged
merged 61 commits into from
Jan 9, 2025
Merged

Add port mappings support #5577

merged 61 commits into from
Jan 9, 2025

Conversation

amanape
Copy link
Member

@amanape amanape commented Dec 13, 2024

This PR adds support for port mappings

Backend

  • Conditionally extend instructions given Runtime instance
  • Add support for custom port mappings from config
  • Add port_mappings field to SandboxConfig

Users can add custom port mappings in their config like this:

config = AppConfig(
    sandbox=SandboxConfig(
        port_mappings={
            8080: 8080,  # map container port 8080 to host port 8080
            5000: 5001,  # map container port 5000 to host port 5001
        }
    )
)

Frontend

image

image
  • Create a new tab to display forwarded content
  • Query localhost:4141 in intervals to conditionally render online/offline text (refetch interval is set to 3000ms)

Example (Vite + React frontend)

  1. Start new chat
  2. [In Terminal] yes | npm create vite my-react-app -- --template react (this creates a Vite + React app in my-react-app)
  3. Wait to complete
  4. [In Terminal] cd my-react-app && npm install
  5. Wait to complete (may take a few seconds)
  6. Edit vite.config.js to
    import { defineConfig } from 'vite'
    import react from '@vitejs/plugin-react'
    
    // https://vite.dev/config/
    export default defineConfig({
      plugins: [react()],
      server: {
        host: "0.0.0.0", // Important to expose port outside the container
        port: 4141, // Target port
      },
    })
  7. Run npm run dev or ask OpenHands to run the server
  8. You should be able to see the same page as in the screenshot above
image

To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:ae92c8f-nikolaik   --name openhands-app-ae92c8f   docker.all-hands.dev/all-hands-ai/openhands:ae92c8f

- Add hardcoded port mapping for 4141
- Add support for custom port mappings from config
- Add port_mappings field to SandboxConfig
@amanape amanape marked this pull request as draft December 13, 2024 06:33
@amanape amanape self-assigned this Dec 13, 2024
@amanape amanape marked this pull request as ready for review December 13, 2024 16:52
@All-Hands-AI All-Hands-AI deleted a comment Dec 13, 2024
@amanape amanape marked this pull request as draft December 16, 2024 17:49
@amanape
Copy link
Member Author

amanape commented Dec 17, 2024

I have extended the remote and base runtime classes to include port mapping as well as extend the conversation route that the FE now depends on to retrieve the potentially available ports. Might be missing something though, would love some insight @xingyaoww @rbren

@amanape amanape marked this pull request as ready for review December 17, 2024 08:50
@amanape amanape requested review from xingyaoww and rbren December 17, 2024 08:50
@amanape amanape changed the title Add port mappings support to sandbox config Add port mappings support Dec 18, 2024
Copy link
Member Author

@amanape amanape left a comment

Choose a reason for hiding this comment

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

Nice! LGTM

@carnalim
Copy link

carnalim commented Jan 4, 2025

This is great! Looking for this feature!

@neubig neubig requested review from li-boxuan and rbren January 5, 2025 06:25
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)
self._app_ports = [self._find_available_port(APP_PORT_RANGE) for _ in range(2)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems this would return two identical ports?

Copy link
Collaborator

Choose a reason for hiding this comment

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

huh...it doesn't, but now that you say it I don't know why....

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's grabbing random ports, so there's a 1-in-10k chance of a collision. Will fix

Copy link
Collaborator

@rbren rbren left a comment

Choose a reason for hiding this comment

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

Excited to say this appears to be working in all environments!

@rbren rbren enabled auto-merge (squash) January 8, 2025 17:59
@rbren rbren disabled auto-merge January 8, 2025 18:09
@rbren rbren enabled auto-merge (squash) January 8, 2025 18:10
@rbren rbren merged commit f6bed82 into main Jan 9, 2025
15 checks passed
@rbren rbren deleted the add-port-mappings branch January 9, 2025 15:02
AlexCuadron pushed a commit to AlexCuadron/OpenHands that referenced this pull request Jan 13, 2025
Co-authored-by: openhands <[email protected]>
Co-authored-by: tofarr <[email protected]>
Co-authored-by: Robert Brennan <[email protected]>
Co-authored-by: Robert Brennan <[email protected]>
AlexCuadron pushed a commit to AlexCuadron/OpenHands that referenced this pull request Jan 13, 2025
Co-authored-by: openhands <[email protected]>
Co-authored-by: tofarr <[email protected]>
Co-authored-by: Robert Brennan <[email protected]>
Co-authored-by: Robert Brennan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants