-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Conversation
- Add hardcoded port mapping for 4141 - Add support for custom port mappings from config - Add port_mappings field to SandboxConfig
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM
This is great! Looking for this feature! |
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)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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
There was a problem hiding this 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!
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]>
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]>
This PR adds support for port mappings
Backend
Runtime
instanceport_mappings
field to SandboxConfigUsers can add custom port mappings in their config like this:
Frontend
localhost:4141
in intervals to conditionally render online/offline text (refetch interval is set to 3000ms)Example (Vite + React frontend)
yes | npm create vite my-react-app -- --template react
(this creates a Vite + React app inmy-react-app
)cd my-react-app && npm install
vite.config.js
tonpm run dev
or ask OpenHands to run the serverTo run this PR locally, use the following command: