Conversation
Summary of ChangesHello, 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 refines how GPU identifiers are generated and managed, particularly for Multi-Instance GPU (MIG) setups. The primary goal is to ensure that the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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 correctly updates the GPU generation logic to use a shared physical GPU UUID for all MIG slices of a single GPU, which was the main goal. The handling of mig_instance_id has also been updated to be more flexible. I've provided one suggestion to improve the consistency of mig_instance_id generation to avoid potential confusion based on the input type from YAML. The accompanying test changes are good and a new test has been added to validate the core change, which is great.
| raw_mig_id = mig_spec.get("mig_instance_id") | ||
| if raw_mig_id is not None: | ||
| if isinstance(raw_mig_id, str): | ||
| mig_partition_id = raw_mig_id | ||
| elif isinstance(raw_mig_id, int): | ||
| mig_partition_id = f"MIG-{raw_mig_id}" | ||
| else: | ||
| try: | ||
| mig_partition_id = f"MIG-{int(raw_mig_id)}" | ||
| except (ValueError, TypeError) as exc: | ||
| raise ValueError( | ||
| f"pod {pod_name}: mig_instance_id must be a string or integer, " | ||
| f"got {type(raw_mig_id).__name__}" | ||
| ) from exc | ||
| else: | ||
| mig_partition_id = f"MIG-{uuid5(NAMESPACE_DNS, mig_name)}" |
There was a problem hiding this comment.
The current logic for determining mig_partition_id handles integers and string representations of integers inconsistently. For example, a mig_instance_id of 1 (integer) becomes "MIG-1", while "1" (string) is used as-is, resulting in "1". This can lead to unexpected behavior depending on how the value is specified in the input YAML.
The suggested change refactors this logic to be more predictable. It attempts to interpret the mig_instance_id as a number first. If successful, it prefixes the value with "MIG-". If not, it treats it as a string identifier. This ensures that numeric IDs are handled consistently, whether they are provided as integers or strings, while still allowing for arbitrary string-based IDs.
Additionally, it would be beneficial to add test cases to verify this new behavior:
- A case where
mig_instance_idis an integer (e.g.,1). - A case where
mig_instance_idis a numeric string (e.g.,"1"). - A case where
mig_instance_idis a non-numeric string (e.g.,"my-partition").
This will ensure the logic is robust and prevent future regressions.
| raw_mig_id = mig_spec.get("mig_instance_id") | |
| if raw_mig_id is not None: | |
| if isinstance(raw_mig_id, str): | |
| mig_partition_id = raw_mig_id | |
| elif isinstance(raw_mig_id, int): | |
| mig_partition_id = f"MIG-{raw_mig_id}" | |
| else: | |
| try: | |
| mig_partition_id = f"MIG-{int(raw_mig_id)}" | |
| except (ValueError, TypeError) as exc: | |
| raise ValueError( | |
| f"pod {pod_name}: mig_instance_id must be a string or integer, " | |
| f"got {type(raw_mig_id).__name__}" | |
| ) from exc | |
| else: | |
| mig_partition_id = f"MIG-{uuid5(NAMESPACE_DNS, mig_name)}" | |
| raw_mig_id = mig_spec.get("mig_instance_id") | |
| if raw_mig_id is None: | |
| mig_partition_id = f"MIG-{uuid5(NAMESPACE_DNS, mig_name)}" | |
| else: | |
| try: | |
| # Handle integers and string representations of integers consistently. | |
| mig_partition_id = f"MIG-{int(raw_mig_id)}" | |
| except (ValueError, TypeError): | |
| # If it's not numeric, treat it as a string ID if possible. | |
| if isinstance(raw_mig_id, str): | |
| mig_partition_id = raw_mig_id | |
| else: | |
| raise ValueError( | |
| f"pod {pod_name}: mig_instance_id must be a string or integer, " | |
| f"got {type(raw_mig_id).__name__}" | |
| ) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #614 +/- ##
=====================================
Coverage 93.6% 93.7%
=====================================
Files 56 56
Lines 4787 4794 +7
Branches 669 672 +3
=====================================
+ Hits 4483 4490 +7
Misses 164 164
Partials 140 140 🚀 New features to boost your workflow:
|
2 participants
Use parent gpu uuid for gpu_uuid.