-
Notifications
You must be signed in to change notification settings - Fork 207
add hotaisle backend #2935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add hotaisle backend #2935
Conversation
|
How to test 1. Add hotaisle creds 2. GPUHunt 3. Serve Qwen |
jvstme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bihan, sorry for the delayed review. The PR looks good overall, but please see my suggestions about some details
| def _validate_user_and_team(self) -> None: | ||
| url = f"{API_URL}/user/" | ||
| response = self._make_request("GET", url) | ||
|
|
||
| if response.ok: | ||
| user_data = response.json() | ||
| else: | ||
| response.raise_for_status() | ||
|
|
||
| teams = user_data.get("teams", []) | ||
| if not teams: | ||
| raise ValueError("No Hotaisle teams found for this user") | ||
|
|
||
| available_teams = [team["handle"] for team in teams] | ||
| if self.team_handle not in available_teams: | ||
| raise ValueError(f"Hotaisle Team '{self.team_handle}' not found.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(optional) This validation is already better than in most our backends, but we can further improve it by validating the roles assigned to the key, so that users can see permission-related errors earlier - when configuring the backend rather than when creating instances.
It should be possible to validate everything (the key, the user role, and the team roles) by calling only GET /user/api_keys/{prefix}/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Will plan to update it in the next iteration.
jvstme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bihan, thank you. I've left some nit-picking comments about the latest changes, but feel free to address them in a separate PR or ignore them, they are not that important. I think the PR is good to merge now
|
|
||
| class HotAisleInstanceBackendData(CoreModel): | ||
| ip_address: str | ||
| vm_id: Optional[str] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) This field is unused, so I wouldn't include it in the model
| except ValueError as e: | ||
| error_message = str(e) | ||
| if "No Hot Aisle teams found" in error_message: | ||
| raise_invalid_credentials_error( | ||
| fields=[["creds", "api_key"]], | ||
| details="Valid API key but no teams found for this user", | ||
| ) | ||
| elif "not found" in error_message: | ||
| raise_invalid_credentials_error( | ||
| fields=[["team_handle"]], details=f"Team handle '{self.team_handle}' not found" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) Looking for patterns in our own error messages and then raising with another error message looks quite redundant. It's also error-prone, because we can change the error message in _validate_user_and_team and forget to change it here.
Some alternatives I can suggest:
- Raise with the same error message -
raise_invalid_credentials_error(details=str(e), ...) - Call
raise_invalid_credentials_errordirectly in_validate_user_and_team - (my favorite) Merge
validate_api_keyand_validate_user_and_teaminto one method and callraise_invalid_credentials_errordirectly
|
|
||
| logger = get_logger(__name__) | ||
|
|
||
| MAX_INSTANCE_NAME_LEN = 60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) Unused
Notes:
api_keyandteam_handle