add validate() method to InferenceConfig#105
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 significantly enhances the robustness of 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. 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. 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 introduces a validate() method to the InferenceConfig class, significantly improving configuration robustness by catching invalid or inconsistent settings early. The from_dict() method now automatically calls this validation, ensuring that configurations loaded from external sources are always valid. A new InferenceConfigValidationError exception is also added for clearer error reporting. The changes are well-implemented and enhance the reliability of the system.
| # comm_backend must be one of the known values | ||
| if self.comm_backend not in _VALID_COMM_BACKENDS: | ||
| errors.append( | ||
| f"comm_backend must be one of {sorted(_VALID_COMM_BACKENDS)}, got {self.comm_backend!r}" |
There was a problem hiding this comment.
The sorted(_VALID_COMM_BACKENDS) call creates a new list object every time the validate method is invoked. While this is a minor overhead for small sets, it's an unnecessary repeated computation. Additionally, formatting the list directly in the f-string might not produce the most user-friendly output. Consider joining the sorted elements for a cleaner error message.
| f"comm_backend must be one of {sorted(_VALID_COMM_BACKENDS)}, got {self.comm_backend!r}" | |
| f"comm_backend must be one of {', '.join(sorted(_VALID_COMM_BACKENDS))}, got {self.comm_backend!r}" |
| f"weights_exchange_ipc_backend must be one of {sorted(_VALID_IPC_BACKENDS)}, " | ||
| f"got {self.weights_exchange_ipc_backend!r}" |
There was a problem hiding this comment.
Similar to the _VALID_COMM_BACKENDS comment, calling sorted(_VALID_IPC_BACKENDS) repeatedly can lead to minor inefficiencies. For improved readability of the error message, consider joining the sorted elements into a comma-separated string.
| f"weights_exchange_ipc_backend must be one of {sorted(_VALID_IPC_BACKENDS)}, " | |
| f"got {self.weights_exchange_ipc_backend!r}" | |
| f"weights_exchange_ipc_backend must be one of {', '.join(sorted(_VALID_IPC_BACKENDS))}, " | |
| f"got {self.weights_exchange_ipc_backend!r}" |
2 participants
What does this PR do?
InferenceConfig has no validation at all, so bad values like engine_rank >= num_engines, an unknown comm_backend, or dump_weights_list_for_validation set without a dump dir fail silently or blow up somewhere deep at runtime. Added a validate() method that checks field consistency (rank bounds, backend names, parallelism sizes >= 1, ep dependency rules, etc.) and raises InferenceConfigValidationError with a clear message listing all problems at once. from_dict() now calls it automatically since that is the main path for loading external configs. validated() is also available for chaining when constructing manually.
Related issues
N/A
Does this PR introduce any user-facing change?