[Metrics] Refactor LoRA state tracking #26801
Conversation
markmc
requested review from
WoosukKwon,
alexm-redhat,
comaniac,
njhill,
robertgshaw2-redhat and
ywang96
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request provides a solid refactoring of the LoRA state tracking logic. The changes effectively address several issues, including memory leaks when log_stats is disabled, ambiguity around method idempotency, and overly complex state management. The new implementation is much clearer, more robust, and easier to maintain. The introduction of a comprehensive unit test is a significant improvement that validates the correctness of the new logic and will help prevent future regressions. Overall, this is an excellent contribution that improves the quality and reliability of the codebase.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
This comment was marked as resolved.
This comment was marked as resolved.
markmc
force-pushed
the
lora-state-metrics
branch
from
af4c449 to
3f1ba30
Compare
|
@njhill Could you please look at this PR？ |
Signed-off-by: Mark McLoughlin <[email protected]>
LoRARequestStates serves a simple purpose - keep track on a per-lora basis which requests are running, and which are waiting. The current implementation is somewhat sloppy and over-complicated: - LoRAStats is never freed if a LoRA becomes unused or unloaded - We leak if log_stats is disabled and LoRA is enabled - It's not clear whether finish_request() should be idempotent - The static methods are a bit weird - We're adding each request to the waiting set twice - ... Hopefully this implementation is more clear and easier to verify that e.g. there are no leaks. Signed-off-by: Mark McLoughlin <[email protected]>
markmc
force-pushed
the
lora-state-metrics
branch
from
3f1ba30 to
cae669c
Compare
SchedulerStats is the right place for this really, just like the regular running/waiting counts. Make sure to call LoRARequestStates.update_scheduler_stats() even where there was no engine core outputs. Signed-off-by: Mark McLoughlin <[email protected]>
markmc
force-pushed
the
lora-state-metrics
branch
from
cae669c to
e6aa440
Compare
jeejeelee
left a comment
jeejeelee
left a comment
There was a problem hiding this comment.
Let's land this PR first
Signed-off-by: Mark McLoughlin <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
Signed-off-by: Mark McLoughlin <[email protected]>
2 participants
LoRARequestStates serves a simple purpose - keep track on a per-lora basis which requests are running, and which are waiting.
The current implementation is somewhat sloppy and over-complicated:
Hopefully this implementation is more clear and easier to verify that e.g. there are no leaks. And now with a unit test!