Skip to content
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

fix: Assign the initial value of occupying_slots to the value of requested_slots #2730

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Yaminyam
Copy link
Member

@Yaminyam Yaminyam commented Aug 19, 2024

In #1115, we did something to initialize occupied_slots to the value of requested_slots when creating the kernel.
The session's data value also ensures that occupying_slots is not empty, so that it has a value even when the session is in PREPARE or PENDING state.

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version

@Yaminyam Yaminyam requested a review from fregataa August 19, 2024 17:28
@github-actions github-actions bot added the comp:manager Related to Manager component label Aug 19, 2024
@github-actions github-actions bot added the size:XS ~10 LoC label Aug 19, 2024
@Yaminyam Yaminyam added this to the 23.09 milestone Aug 19, 2024
@Yaminyam Yaminyam requested review from achimnol and removed request for kyujin-cho August 19, 2024 17:30
@lizable lizable added the urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE! label Aug 20, 2024 — with Graphite App
@@ -1273,6 +1273,7 @@ async def _enqueue() -> None:

session_data["environ"] = environ
session_data["requested_slots"] = session_requested_slots
session_data["occupying_slots"] = session_requested_slots
Copy link
Member

@fregataa fregataa Aug 20, 2024

Choose a reason for hiding this comment

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

This will double the value of sessions.occupying_slots because the value of actual allocated resource is added to this in finalize_running().
We should update the finalize_running() or change this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:manager Related to Manager component size:XS ~10 LoC urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants