feat: Add InferenceGraph resource #2361
Conversation
WalkthroughThe changes introduce a new class, Changes
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
israel-hdez
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
ocp_resources/inference_graph.py (3)
9-12: Update the class documentation with proper description.The class docstring is currently a placeholder without any actual description of what an InferenceGraph resource is or how it should be used. Consider adding information about the purpose of this resource in KServe, its relationship to inference services, and any important characteristics that users should know.
16-57: Update parameter documentation with proper descriptions.The docstrings for all parameters are currently placeholders. Consider consulting the KServe API documentation to add accurate descriptions for each parameter, explaining:
- What
affinitycontrols in the context of inference graphs- The purpose of
max_replicasandmin_replicasfor scaling- What the required
nodesparameter represents and its structure- What
resourcesshould contain- How
scale_metricandscale_targetaffect scaling behavior- What the
timeoutparameter controlsThis will significantly improve the usability of this class.
65-90: Consider using a utility function for snake_case to camelCase conversion.The method manually converts snake_case parameter names to camelCase for the API (e.g.,
max_replicastomaxReplicas). This could be prone to errors if new parameters are added. Consider using a utility function for this conversion to improve maintainability.Also, consider enhancing the structure building pattern to be more concise:
- if self.affinity is not None: - _spec["affinity"] = self.affinity - - if self.max_replicas is not None: - _spec["maxReplicas"] = self.max_replicas - - if self.min_replicas is not None: - _spec["minReplicas"] = self.min_replicas - - if self.resources is not None: - _spec["resources"] = self.resources - - if self.scale_metric is not None: - _spec["scaleMetric"] = self.scale_metric - - if self.scale_target is not None: - _spec["scaleTarget"] = self.scale_target - - if self.timeout is not None: - _spec["timeout"] = self.timeout + # Map snake_case attributes to camelCase API fields + attr_to_field = { + "affinity": "affinity", + "max_replicas": "maxReplicas", + "min_replicas": "minReplicas", + "resources": "resources", + "scale_metric": "scaleMetric", + "scale_target": "scaleTarget", + "timeout": "timeout" + } + + for attr, field in attr_to_field.items(): + value = getattr(self, attr) + if value is not None: + _spec[field] = value
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/inference_graph.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: tox
- GitHub Check: python-module-install
- GitHub Check: conventional-title
🔇 Additional comments (3)
ocp_resources/inference_graph.py (3)
14-14: LGTM! Correct API group set for KServe resource.The API group is correctly set to the SERVING_KSERVE_IO constant from the parent class.
61-64: LGTM! Proper validation for required nodes parameter.The code correctly checks if the required
nodesparameter is provided and raises an appropriate error when it's missing.
59-59:❌ Incorrect review comment
Missing return value from super().to_dict() call.
The parent method's return value is not being used. You should either capture the return value or check if the parent's implementation modifies
self.resdirectly.
🏁 Script executed:
#!/bin/bash # Check if the parent class's to_dict method returns a value or modifies self.res rg -A 10 "def to_dict" --glob "ocp_resources/resource.py"Length of output: 1300
Clarification on
to_dict()BehaviorAfter verifying the parent implementation in
ocp_resources/resource.py, it’s clear that the parent’sto_dict()method is designed to update the resource state (via_base_body()) rather than return a dictionary. The method’s signature explicitly indicates a return type ofNone, so capturing a return value isn’t necessary in this context.Likely an incorrect or invalid review comment.
|
/retest conventional-title |
Short description:
Add InferenceGraph resource
More details:
Add the InferenceGraph resource from the KServe project.
What this PR does / why we need it:
The new resource is going to be used for testing the InferenceGraph feature of Open Data Hub project.
Summary by CodeRabbit