(shortfin-sd) Usability and logging improvements. #491
Conversation
monorimet
commented
Nov 13, 2024
•
monorimet
commented
- Port configuration fixes
- Reduce need for topology footguns by implementing simple topology config artifacts for 4 setups (cpx:single/multi, spx:single/multi). These set server/device topologies to "known good" configurations.
- Fix example client script (send_request.py -> simple_client.py) and include in python package, arg problems fixed as well (--save)
- Remove need for source code
- Safer failures for invalid output dims
- Don't report server startup under uvicorn.error
- Updates sd README with new example CLI inputs
- Adds help for client CLI args
monorimet
requested review from
stellaraccident,
kumardeepakamd,
pdhirajkumarprasad and
amd-chrissosa
monorimet
changed the title
| @@ -38,19 +38,15 @@ def __init__(self): | |||
| native_handler.setFormatter(NativeFormatter()) | |||
|
|
|||
| # TODO: Source from env vars. | |||
| logger.setLevel(logging.INFO) | |||
| logger.setLevel(logging.DEBUG) | |||
There was a problem hiding this comment.
For release should we move this back to info/warn? if you want I can just clean up the logging after you submit. I feel tempted to just add a debug/verbose option and pipe that through here.
| } | ||
|
|
||
|
|
||
| def bytes_to_img(bytes, idx=0, width=1024, height=1024, outputdir="./gen_imgs"): |
There was a problem hiding this comment.
Since you have this as an arg from the arg parser no need to have a second default.
|
|
||
| def get_batched(request, arg, idx): | ||
| if isinstance(request[arg], list): | ||
| if len(request[arg]) == 1: |
There was a problem hiding this comment.
Why ignore the index if there is only one item? Is the index -1 or something? Maybe add a comment
| print(f"Error: Received {response.status} from server") | ||
| raise Exception | ||
| except Exception as e: | ||
| print(f"Request failed: {e}") |
There was a problem hiding this comment.
This except clause seems redundant with the internal one. Based on docs for async with you already have a try except inside there right?
| p.add_argument( | ||
| "--file", | ||
| type=str, | ||
| default="default", |
There was a problem hiding this comment.
Cleaner to just check for None than a hard coded default string
| parser.add_argument( | ||
| "--tuning_spec", | ||
| type=str, | ||
| default="", |
There was a problem hiding this comment.
I'd stick to "" or None for string defaults.
| from shortfin_apps.sd.components.config_struct import ModelParams | ||
|
|
||
| this_dir = os.path.dirname(os.path.abspath(__file__)) | ||
| parent = os.path.dirname(this_dir) |
There was a problem hiding this comment.
do you use this anywhere?
There was a problem hiding this comment.
Nope, copied from the other builder. Thanks.
| @@ -24,7 +24,7 @@ | |||
| sfnp.bfloat16: "bf16", | |||
| } | |||
|
|
|||
| ARTIFACT_VERSION = "11022024" | |||
| ARTIFACT_VERSION = "11132024" | |||
There was a problem hiding this comment.
Can artifact version bump be automated in future?
| STROBE_SHORT_DELAY = 0.1 | ||
| STROBE_LONG_DELAY = 0.25 | ||
| STROBE_SHORT_DELAY = 0.5 | ||
| STROBE_LONG_DELAY = 1 |
There was a problem hiding this comment.
How does this impact user experience?
There was a problem hiding this comment.
these are the increments by which the batcher checks for incoming requests
|
Merging and will follow up with addressing review comments |
