-
Notifications
You must be signed in to change notification settings - Fork 39
add new states when the max_memory_scaler is updated #222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
We require contributors to sign our Contributor License Agreement (CLA), and we don't have record of your signature. In order for us to review and merge your code, please sign the CLA. |
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InFlightAutoBatcher
participant Iterator
User->>InFlightAutoBatcher: Initialize and call _get_first_batch()
InFlightAutoBatcher->>Iterator: _get_next_states()
Iterator-->>InFlightAutoBatcher: Return initial states
InFlightAutoBatcher->>InFlightAutoBatcher: Estimate and adjust max_memory_scaler
InFlightAutoBatcher->>Iterator: _get_next_states() (second call)
Iterator-->>InFlightAutoBatcher: Return more states
InFlightAutoBatcher->>InFlightAutoBatcher: Append new states to previous
InFlightAutoBatcher->>InFlightAutoBatcher: Concatenate all states for first batch
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Thank you for your contribution and similar to the other PR I didn't work on the autobatcher so I'm not too familiar with it at the moment. I'll have more free time on the weekend and will have better things to say then! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @kianpu34593! This is a smart and common sense fix for maximizing the first batch memory. I was frustrated by the difference and wish I'd thought of it myself! LGTM.
Thanks for this smart fix! |
Summary
This is related to the recent pull request #219. In addition to what @t-reents was suggesting, I found there is an improvement can be done in
InFlightAutoBatcher
during the initiation of batch.first_state
to estimate themax_memory_scaler
. After that, states are added usingstates = self._get_next_states()
, based on the estimatedmax_memory_scaler
. After this,max_memory_scaler
is being estimated again usingestimate_max_memory_scaler
. This function results in a highermax_memory_scaler
. This means that more states can be added in the first batch.What I added is these two lines:
This allows more states to be added based on the new
max_memory_scaler
in the first batch.Checklist
Before a pull request can be merged, the following items must be checked:
Run ruff on your code.
Summary by CodeRabbit