[editor] Functionality to run server on multiple configs without specifying port #624
Conversation
Ankush-lastmile
force-pushed
the
pr624
branch
7 times, most recently
from
65e7fdb to
94b5f78
Compare
…ifying port ## What: Before creating the server process, check if the port to be used is taken. If so, find the next available one before continuing ## Why When running the editor, user should be able to open multiple configs. Under the hood this means spinning up another server in an unused port. Frontend already supports dynamic ports. ## Testplan 1. yarn build in aiconfig/python/src/aiconfig/editor/client 2. Open two aiconfigs with "prod" editor" | Getting Started | Chain Of Verification | |------------|-------------| |  | | |  | |
Ankush-lastmile
changed the title
Ankush-lastmile
force-pushed
the
pr624
branch
from
94b5f78 to
4408be8
Compare
Ankush-lastmile
requested review from
saqadri,
rholinshead,
suyoglastmileai,
jonathanlastmileai and
rossdanlm
as code owners
|
In the second screenshot, the terminal output shows port 8082 but browser shows 8081. Just double-checking this is just an incorrect image / I assume you ran it once more after 8081 was already in use? |
Yes it was an incorrect usage/testplan, good catch. Re-executed test plan and the ports are as expected. Updated Testplan as well |
| edit_config_dict = edit_config.model_dump() | ||
| edit_config_dict["server_port"] = port | ||
| edit_config = EditServerConfig(**edit_config_dict) |
There was a problem hiding this comment.
model_copy is what you want here https://docs.pydantic.dev/latest/concepts/serialization/#model_copy
| edit_config_dict["server_port"] = port | ||
| edit_config = EditServerConfig(**edit_config_dict) | ||
|
|
||
| LOGGER.warning(f"Using {port}.") |
| @@ -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: | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
[editor] Functionality to run server on multiple configs without specifying port
What:
Before creating the server process, check if the port to be used is taken. If so, find the next available one before continuing
for context: this builds on top of #619
Why
When running the editor, user should be able to open multiple configs.
Under the hood this means spinning up another server in an unused port. Frontend already supports dynamic ports.
Testplan
Terminals showcase the command used to open editor. Each one has a seperate config file path. No port is passed in from the user.
Chain of verification Uses Port 8080
Getting Started in a new process using Port 8081