[COST-7044] Add support for unallocated GPU #596
Conversation
|
|
Summary of ChangesHello @myersCody, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the OCP generator by adding support for tracking and reporting unallocated GPU resources. Previously, the system only accounted for GPUs explicitly assigned to pods. With this change, GPUs provisioned at the node level, even if not currently in use by a workload, are now included in reports. This provides a more comprehensive understanding of GPU utilization and helps in identifying potential cost savings from idle resources. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 adds support for generating usage data for unallocated GPUs, i.e., GPUs that are present on a node but not assigned to any pod. The changes correctly modify the data generation logic to account for GPUs defined at the node level in the static configuration file and update the hourly usage generation to include these unallocated GPUs.
The implementation is mostly correct, but I've identified a couple of areas for improvement regarding code complexity and performance. My comments focus on simplifying a convoluted piece of logic for finding node specifications and optimizing a lookup that happens inside a nested loop. These changes will make the code more readable, maintainable, and performant.
| for gpu_key, gpu_list in self.gpus.items(): | ||
| if isinstance(gpu_key, tuple): | ||
| node_name = gpu_key[0] | ||
| node_obj = next((n for n in self.nodes if n.get("name") == node_name), None) |
There was a problem hiding this comment.
This next() call to find a node by name is inside a nested loop (over hours and GPUs), which can lead to poor performance if there are many nodes, GPUs, and a long time range. The complexity is O(hours * gpus * nodes).
To optimize this, you could create a name-to-node mapping before the loops. This would reduce the complexity to O(nodes + hours * gpus).
Example:
def _gen_hourly_gpu_usage(self, **kwargs):
node_map = {n.get("name"): n for n in self.nodes}
for hour in self.hours:
start = hour.get("start")
end = hour.get("end")
for gpu_key, gpu_list in self.gpus.items():
if isinstance(gpu_key, tuple):
node_name = gpu_key[0]
node_obj = node_map.get(node_name)
if not node_obj:
continue
# ... rest of the logicSince this change would be outside the current diff, I'm providing it as an example for you to consider.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #596 +/- ##
=======================================
- Coverage 93.5% 93.2% -0.3%
=======================================
Files 56 56
Lines 4730 4750 +20
Branches 663 669 +6
=======================================
+ Hits 4422 4428 +6
- Misses 165 178 +13
- Partials 143 144 +1 🚀 New features to boost your workflow:
|
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
a gpu that is present but not allocated to a pod will simply not appear in the report. currently we gather metrics for gpu utilization |
Example Export: