Conversation
ataymano
commented
May 7, 2025
ataymano
commented
- v0: original v0 implementation of single node
- node: refactored v0 + mock instance implementation
- master: master node for routing
- client: debug clients (for single instance, node and master)
| @@ -0,0 +1,121 @@ | |||
| import os | |||
| from fastapi import FastAPI, HTTPException, Response | |||
| from fastapi.responses import JSONResponse, StreamingResponse | |||
There was a problem hiding this comment.
Before merging remove original worker node (omniboxes/v0) as latest single worker node is already under omniboxes/node
There was a problem hiding this comment.
To align with Kubernetes terminology rename folder from node to worker or workernode (if call master folder masternode)
ThomasDh-C
left a comment
ThomasDh-C
left a comment
There was a problem hiding this comment.
Interface and class improvements from V0! General refactor needed to split up larger file and ensure overall class reuse.
| def do_and_show(self, command, waitTime): | ||
| response = self.execute(command) | ||
| if response.status_code != 200: | ||
| raise Exception(f"Error: {response.status_code} - {response.text}") |
There was a problem hiding this comment.
There is a chance that the subprocess runs so you get back status of 200, but the python program you executed failed for some reason so subprocess doesn't return a status code of 0. Add a check after this 200 check to check the return code from the actual execute step
https://github.com/microsoft/OmniParser/blob/5171b092483ab3e74ca50b9357e225f9f3571f18/omnitool/omnibox/vm/win11setup/setupscripts/server/main.py#L49C13-L61C20
| response = self.execute(command) | ||
| if response.status_code != 200: | ||
| raise Exception(f"Error: {response.status_code} - {response.text}") | ||
| time.sleep(waitTime) |
There was a problem hiding this comment.
Do we want to make this a class variable and then build it into execute function and wait before actual execute request is sent to windows vm?
| self.output])) | ||
|
|
||
|
|
||
| class MasterClient: |
There was a problem hiding this comment.
Break this file up into its individual clients
| @@ -0,0 +1,8 @@ | |||
| import logging | |||
|
|
|||
| def default_logger(): | |||
There was a problem hiding this comment.
remove - duplicated in node_manager.py
| import requests | ||
| import argparse | ||
|
|
||
| # Configure logging |
There was a problem hiding this comment.
NodeManager will make its own logger if not passed in so this can be removed
| from fastapi import FastAPI, HTTPException, Response | ||
| import requests | ||
|
|
||
| class InstanceClient: |
There was a problem hiding this comment.
This should import from client folder - maybe we want to rename that to clients
| response = requests.post(f"{self.url}/execute", json=command_data, timeout=5) | ||
| return JSONResponse(content=response.json(), status_code=response.status_code) | ||
| except requests.RequestException as e: | ||
| raise HTTPException(status_code=500, detail=f"Error communicating with theinstance {self.url}: {str(e)}") |
| from logging_utils import default_logger | ||
|
|
||
|
|
||
| class MockApp: |
There was a problem hiding this comment.
Clean up names + docstrings of mock
| draw = ImageDraw.Draw(image) | ||
|
|
||
| try: | ||
| font = ImageFont.truetype("DejaVuSans.ttf", 14) |
There was a problem hiding this comment.
Not needed for mock - just use default
There was a problem hiding this comment.
Can this be notebook be moved into a separate environment_management folder or sth similar?
2 participants
