Skip to content

[Core][GPU fraction][1/n] Unify node feasibility and availability checking #59278

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.

Sign up for GitHub

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

Open
Yicheng-Lu-llll wants to merge 2 commits into
base: master
Choose a base branch
from
+5 −12
Open

[Core][GPU fraction][1/n] Unify node feasibility and availability checking #59278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7900ecd
Unify node feasibility and availability checking
Dec 8, 2025
f71ddad
add comment
Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions src/ray/raylet/scheduling/policy/scorer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@ namespace raylet_scheduling_policy {

double LeastResourceScorer::Score(const ResourceRequest &required_resources,
const NodeResources &node_resources) {
// Check if the node has required labels before scoring on the resources.
const auto &label_selector = required_resources.GetLabelSelector();
if (!node_resources.HasRequiredLabels(label_selector)) {
if (!node_resources.IsAvailable(required_resources)) {
Copy link
Member Author

@Yicheng-Lu-llll Yicheng-Lu-llll Dec 9, 2025

Choose a reason for hiding this comment

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

IsAvailable already checks the required labels.

return -1.;
}

Expand All @@ -42,22 +40,16 @@ double LeastResourceScorer::Score(const ResourceRequest &required_resources,
for (auto &resource_id : required_resources.ResourceIds()) {
const auto &request_resource = required_resources.Get(resource_id);
const auto &node_available_resource = node_resources_ptr->available.Get(resource_id);
auto score = Calculate(request_resource, node_available_resource);
if (score < 0.) {
return -1.;
}
node_score += score;
node_score += Calculate(request_resource, node_available_resource);
Copy link
Contributor

@MengjinYan MengjinYan Dec 9, 2025

Choose a reason for hiding this comment

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

From my understanding of the code:

  • Previously: we first checks the labels, then potentially update the available resources, the checks whether there are available resources in the Calculate function
  • Now: we first checks both the labels and the the available resources, then potentially update the available resources, the calculate the score assuming the node has enough resources for the request

I might miss something but looks like with the new implementation, there could be case where the node will have available resources before the resource update but not after which will make the new implementation not consistent with the previous implementation, is that right?

Copy link
Member Author

@Yicheng-Lu-llll Yicheng-Lu-llll Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry about that — I should have mentioned this!

IsAvailable already handles the normal_task_resources subtraction internally:

ray/src/ray/common/scheduling/cluster_resource_data.cc

Lines 98 to 103 in de7ac7d

if (!this->normal_task_resources.IsEmpty()) {
auto available_resources = this->available;
available_resources -= this->normal_task_resources;
return available_resources >= resource_request.GetResourceSet();
}
return this->available >= resource_request.GetResourceSet();

Copy link
Contributor

@MengjinYan MengjinYan Dec 10, 2025

Choose a reason for hiding this comment

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

Got it. I missed that. Thanks for the context!

The current implementation is a bit confusion, as part of the project of improving the resource tracking effort, we should make it cleaner.

Copy link
Member Author

@Yicheng-Lu-llll Yicheng-Lu-llll Dec 10, 2025

Choose a reason for hiding this comment

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

Sure, I will look deep into it and draft an follow up pr if needed！

}
return node_score;
}

// This function assumes the resource request has already passed the availability check
Copy link
Contributor

@MengjinYan MengjinYan Dec 9, 2025

Choose a reason for hiding this comment

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

nit: Can you add the this also to the comment of the function in the .h file?

Copy link
Member Author

@Yicheng-Lu-llll Yicheng-Lu-llll Dec 10, 2025

Choose a reason for hiding this comment

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

Added, Thank you!

double LeastResourceScorer::Calculate(const FixedPoint &requested,
const FixedPoint &available) {
RAY_CHECK(available >= 0) << "Available resource " << available.Double()
<< " should be nonnegative.";
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 9, 2025

Choose a reason for hiding this comment

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

medium

The removal of the requested > available check relies on node_resources.IsAvailable() being correct. To make the code more robust against potential bugs in the availability check, it's safer to add a RAY_CHECK(requested <= available). This enforces the assumption that Calculate is only called for feasible requests and will help catch issues early.

Suggested change
<< " should be nonnegative.";
<< " should be nonnegative.";
RAY_CHECK(requested <= available)
<< "Requested resource " << requested.Double()
<< " should not be greater than available resource " << available.Double()
<< ". This indicates a bug in the feasibility check.";

Copy link
Member Author

@Yicheng-Lu-llll Yicheng-Lu-llll Dec 9, 2025

Choose a reason for hiding this comment

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

No need. From a design perspective, the logic for determining whether a node is schedulable and the logic for choosing the best node should be completely separate and follow a strict order.

if (requested > available) {
return -1;
}

if (available == 0) {
return 0;
Expand Down
3 changes: 2 additions & 1 deletion src/ray/raylet/scheduling/policy/scorer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ class LeastResourceScorer : public NodeScorer {

private:
/// \brief Calculate one of the resource scores.
///
/// This function assumes the resource request has already passed the availability
/// check.
/// \param requested Quantity of one of the required resources.
/// \param available Quantity of one of the available resources.
/// \return Score of the node.
Expand Down