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

Jump to bottom

[editor] Functionality to run server on multiple configs without specifying port #624

Merged
merged 1 commit into from
Dec 26, 2023
Merged

[editor] Functionality to run server on multiple configs without specifying port #624

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 26 additions & 0 deletions python/src/aiconfig/scripts/aiconfig_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import signal
import subprocess
import sys
import socket

import lastmile_utils.lib.core.api as core_utils
import result
Expand Down Expand Up @@ -64,8 +65,33 @@ def _sigint(procs: list[subprocess.Popen[bytes]]) -> Result[str, str]:
p.send_signal(signal.SIGINT)
return Ok("Sent SIGINT to frontend servers.")

def is_port_in_use(port: int) -> bool:
Copy link
Contributor

@jonathanlastmileai jonathanlastmileai Dec 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is fine for now, but there are both functional and UX problems with this approach.

Functional: It might be unlikely, but technically you can have a race condition where the port gets used after this returns but before the flask server starts.

UX: If I pass a port explicitly and it's already in use, I expect an error. I don't want the app to find the next open port for me.

Here's what I would have done:

  • If user provides port, don't do any special logic
  • Otherwise, loop over all possible ports (e.g. starting at 8080) and try to start the server until one works, catching an exception for in use.

cc @saqadri , @rossdanlm

Copy link
Contributor

@rossdanlm rossdanlm Dec 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense, but I think this could be seen as an enhancement instead of a bug. I made a GH issue for this in #633 as a bootcamp task that others can do if they like

"""
Checks if a port is in use at localhost.

Creates a temporary connection.
Context manager will automatically close the connection
"""
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
return s.connect_ex(('localhost', port)) == 0


def _run_editor_servers(edit_config: EditServerConfig) -> Result[list[str], str]:

port = edit_config.server_port

while is_port_in_use(port):
LOGGER.warning(f"Port {port} is in use. Checking next port.")
port += 1

# Must reconstruct, EditServerConfig is an immutable type (frozen dataclass)
edit_config_dict = edit_config.model_dump()
edit_config_dict["server_port"] = port
edit_config = EditServerConfig(**edit_config_dict)
Comment on lines +88 to +90
Copy link
Contributor

Choose a reason for hiding this comment

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

model_copy is what you want here https://docs.pydantic.dev/latest/concepts/serialization/#model_copy


LOGGER.warning(f"Using {port}.")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: .info


# Check if server is already running
LOGGER.info("Running editor servers")
frontend_procs = _run_frontend_server_background() if edit_config.server_mode in [ServerMode.DEBUG_SERVERS] else Ok([])
match frontend_procs:
Expand Down