Skip to content

Cleanup __init__#647

Merged
sjmonson merged 2 commits intomainfrom
fix/init_cleanup
Mar 19, 2026
Merged

Cleanup __init__#647
sjmonson merged 2 commits intomainfrom
fix/init_cleanup

Conversation

@sjmonson
Copy link
Copy Markdown
Collaborator

Summary

Clean up some __init__ package code and move uvloop config to __init__.

Details

This clean up was originally a part of #641 but as that PR is blocked I decided to split it out. Removing the transformers logging config does not seem to have any real affect; I do not get logs either way. Importing any huggingface libraries incurs a significant time cost so this is a prereq to improving CLI responsiveness. Additionally uvloop should be configured as early as possible so moved the setup to __init__.


  • "I certify that all code in this PR is my own, except as noted below."

Use of AI

  • Includes AI-assisted code completion
  • Includes code generated by an AI application
  • Includes AI-generated tests (NOTE: AI written tests should have a docstring that includes ## WRITTEN BY AI ##)

@sjmonson sjmonson requested review from dbutenhof, jaredoconnell and markurtz and removed request for markurtz March 19, 2026 18:19
@sjmonson sjmonson changed the title Fix/init cleanup Cleanup __init__ Mar 19, 2026
Signed-off-by: Samuel Monson <smonson@redhat.com>
Signed-off-by: Samuel Monson <smonson@redhat.com>
Copy link
Copy Markdown
Collaborator

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

So the Environment stuff was never even used except in the test? Huh...

Copy link
Copy Markdown
Collaborator

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

The new code looks a lot simpler. Are we losing any functionality with this change?
Approving because I don't see any potential issues with this code.

@sjmonson
Copy link
Copy Markdown
Collaborator Author

@Mergifyio queue

@mergify
Copy link
Copy Markdown

mergify bot commented Mar 19, 2026

queue

⚠️ Configuration not compatible with a branch protection setting

Details

The branch protection setting Require branches to be up to date before merging is not compatible with draft PR checks. To keep this branch protection enabled, update your Mergify configuration to enable in-place checks: set merge_queue.max_parallel_checks: 1, set every queue rule batch_size: 1, and avoid two-step CI (make merge_conditions identical to queue_conditions). Otherwise, disable this branch protection.

@sjmonson
Copy link
Copy Markdown
Collaborator Author

⚠️ Configuration not compatible with a branch protection setting

😢 Maybe next time then.

@sjmonson sjmonson merged commit 2af12ab into main Mar 19, 2026
30 of 31 checks passed
@sjmonson sjmonson deleted the fix/init_cleanup branch March 19, 2026 20:00
@dbutenhof dbutenhof mentioned this pull request Mar 19, 2026
4 tasks
@sjmonson sjmonson mentioned this pull request Mar 20, 2026
4 tasks
sjmonson added a commit that referenced this pull request Mar 20, 2026
## Summary

Fixes spawn and forkserver multi-process contexts.

## Details

I was hoping that after #647 we could switch to `forkserver` by default.
However it turns out that `forkserver` and `spawn` will import the
calling processes entrypoint (E.g. `__main__.py`) so we run into the
same blocker as #641. However, I was able to confirm that striping every
heavy import out of `__main__.py` solves the issue. So we should be good
to switch in v0.7.0.

On my machine there is about a ~10s overhead for `forkserver` and
slightly more for `spawn`, which is not the worst for a default.
However, the overhead may be more on other systems:

### `time guidellm benchmark run --profile poisson --rate 5 --data
prompt_tokens=128,output_tokens=128 --max-seconds 30 --outputs json`

| Context    | real      | user      | sys      |
| ---------- | --------- | --------- | -------- |
| Fork       | 0m37.874s | 0m17.356s | 0m1.883s |
| Forkserver | 0m47.344s | 0m14.862s | 0m0.860s |
| Spawn      | 0m49.515s | 1m51.230s | 0m8.915s |

### `time guidellm benchmark run --profile concurrent --rate 400 --data
prompt_tokens=128,output_tokens=128 --max-seconds 30 --outputs json`

| Context    | real      | user      | sys       |
| ---------- | --------- | --------- | --------- |
| Fork       | 0m39.324s | 0m37.602s | 0m5.623s  |
| Forkserver | 0m49.609s | 0m19.710s | 0m1.311s  |
| Spawn      | 0m50.399s | 2m9.724s  | 0m11.374s |

### `time guidellm benchmark run --profile concurrent --rate 400 --data
prompt_tokens=128,output_tokens=128 --max-seconds 120 --outputs json`

| Context    | real      | user      | sys       |
| ---------- | --------- | --------- | --------- |
| Fork       | 2m15.309s | 1m42.911s | 0m15.957s |
| Forkserver | 2m25.964s | 0m38.891s | 0m2.802s  |
| Spawn      | 2m27.454s | 3m24.325s | 0m22.531s |

## Test Plan

Set `GUIDELLM__MP_CONTEXT_TYPE=forkserver` and confirm benchmarks run.

---

- [x] "I certify that all code in this PR is my own, except as noted
below."

## Use of AI

- [x] Includes AI-assisted code completion
- [ ] Includes code generated by an AI application
- [ ] Includes AI-generated tests (NOTE: AI written tests should have a
docstring that includes `## WRITTEN BY AI ##`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants