Conversation
📝 WalkthroughWalkthroughThe change adds explicit mapping of the "FAILED_SETUP" SkyPilot status string to ClusterState.FAILED in cluster status retrieval methods, replacing a previous fallback mechanism that used direct enum lookup with KeyError handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/transformerlab/compute_providers/skypilot.py (1)
702-709: Consider centralizing cluster-state mapping logic.The same status normalization appears in two methods. A small shared helper would reduce drift risk when new SkyPilot states are added.
♻️ Suggested refactor
+ def _map_skypilot_cluster_state(self, state_str: str) -> ClusterState: + if state_str == "FAILED_SETUP": + return ClusterState.FAILED + try: + return ClusterState[state_str] + except KeyError: + return ClusterState.UNKNOWN- if state_str == "FAILED_SETUP": - state = ClusterState.FAILED - else: - try: - state = ClusterState[state_str] - except KeyError: - state = ClusterState.UNKNOWN + state = self._map_skypilot_cluster_state(state_str)Also applies to: 858-865
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/transformerlab/compute_providers/skypilot.py` around lines 702 - 709, Create a single helper (e.g., map_skypilot_state or _map_skypilot_state) that accepts the raw SkyPilot state_str and returns the appropriate ClusterState, centralizing the logic that maps "FAILED_SETUP" to ClusterState.FAILED and falls back to ClusterState.UNKNOWN for unknown values (currently done with try: ClusterState[state_str] except KeyError). Replace the duplicated mapping blocks in the two places in skypilot.py (the mapping near the shown block and the similar block around the other occurrence) to call this helper so all state normalization is in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/transformerlab/compute_providers/skypilot.py`:
- Around line 702-709: Create a single helper (e.g., map_skypilot_state or
_map_skypilot_state) that accepts the raw SkyPilot state_str and returns the
appropriate ClusterState, centralizing the logic that maps "FAILED_SETUP" to
ClusterState.FAILED and falls back to ClusterState.UNKNOWN for unknown values
(currently done with try: ClusterState[state_str] except KeyError). Replace the
duplicated mapping blocks in the two places in skypilot.py (the mapping near the
shown block and the similar block around the other occurrence) to call this
helper so all state normalization is in one place.
Paragon SummaryThis pull request review identified 1 issue across 1 category in 1 file. The review analyzed code changes, potential bugs, security vulnerabilities, performance issues, and code quality concerns using automated analysis tools. This PR adds a new terminal state to the skypilot compute provider, likely to better handle job or instance lifecycle completion in the skypilot integration. The change is isolated to the skypilot compute provider module. Key changes:
Confidence score: 5/5
1 file reviewed, 1 comment Severity breakdown: Low: 1 Tip: |
![paragon-review[bot]](https://avatars.githubusercontent.com/in/1544258?s=60&v=4){kind=small}
| state = ClusterState[state_str] | ||
| except KeyError: | ||
| state = ClusterState.UNKNOWN | ||
| # Map SkyPilot's FAILED_SETUP directly to our FAILED state |
There was a problem hiding this comment.
Bug: Identical state-mapping block is duplicated in two methods
Identical state-mapping block is duplicated in two methods. Future state additions may miss one site. Extract a shared helper method.
View Details
Location: api/transformerlab/compute_providers/skypilot.py (lines 702)
Analysis
Identical state-mapping block is duplicated in two methods
| What fails | No runtime failure, but the same 8-line state-mapping block is copy-pasted in get_cluster_status and list_clusters, creating a maintenance risk. |
| Result | Two identical blocks must be kept in sync manually; a future mapping change could be applied to only one. |
| Expected | A single private helper (e.g., _map_skypilot_state) should own the mapping logic, called from both methods. |
| Impact | Low risk now, but divergence on the next state-mapping change could silently misreport cluster status. |
How to reproduce
grep -n 'FAILED_SETUP' api/transformerlab/compute_providers/skypilot.py # shows two identical blocksAI Fix Prompt
Fix this issue: Identical state-mapping block is duplicated in two methods. Future state additions may miss one site. Extract a shared helper method.
Location: api/transformerlab/compute_providers/skypilot.py (lines 702)
Problem: No runtime failure, but the same 8-line state-mapping block is copy-pasted in get_cluster_status and list_clusters, creating a maintenance risk.
Current behavior: Two identical blocks must be kept in sync manually; a future mapping change could be applied to only one.
Expected: A single private helper (e.g., _map_skypilot_state) should own the mapping logic, called from both methods.
Steps to reproduce: grep -n 'FAILED_SETUP' api/transformerlab/compute_providers/skypilot.py # shows two identical blocks
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
Summary by CodeRabbit