Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(gpu): introduce GPU request spec#1156
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
814cf56dee6d895a02faba7188c79af4d04File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -39,17 +39,18 @@ use openshell_core::proto::{ | ||
| GetClusterInferenceRequest, GetDraftHistoryRequest, GetDraftPolicyRequest, | ||
| GetGatewayConfigRequest, GetProviderProfileRequest, GetProviderRefreshStatusRequest, | ||
| GetProviderRequest, GetSandboxConfigRequest, GetSandboxLogsRequest, | ||
| GetSandboxPolicyStatusRequest, GetSandboxRequest, GetServiceRequest, HealthRequest, | ||
| ImportProviderProfilesRequest, LintProviderProfilesRequest, ListProviderProfilesRequest, | ||
| ListProvidersRequest, ListSandboxPoliciesRequest, ListSandboxProvidersRequest, | ||
| ListSandboxesRequest, ListServicesRequest, PlatformEvent, PolicySource, PolicyStatus, Provider, | ||
| ProviderCredentialRefreshStatus, ProviderCredentialRefreshStrategy, ProviderProfile, | ||
| ProviderProfileDiagnostic, ProviderProfileImportItem, RejectDraftChunkRequest, | ||
| RevokeSshSessionRequest, RotateProviderCredentialRequest, Sandbox, SandboxPhase, SandboxPolicy, | ||
| SandboxSpec, SandboxTemplate, ServiceEndpointResponse, SetClusterInferenceRequest, | ||
| SettingScope, SettingValue, TcpForwardFrame, TcpForwardInit, TcpRelayTarget, | ||
| UpdateConfigRequest, UpdateProviderRequest, WatchSandboxRequest, exec_sandbox_event, | ||
| setting_value, tcp_forward_init, | ||
| GetSandboxPolicyStatusRequest, GetSandboxRequest, GetServiceRequest, GpuResourceRequirement, | ||
| HealthRequest, ImportProviderProfilesRequest, LintProviderProfilesRequest, | ||
| ListProviderProfilesRequest, ListProvidersRequest, ListSandboxPoliciesRequest, | ||
| ListSandboxProvidersRequest, ListSandboxesRequest, ListServicesRequest, PlatformEvent, | ||
| PolicySource, PolicyStatus, Provider, ProviderCredentialRefreshStatus, | ||
| ProviderCredentialRefreshStrategy, ProviderProfile, ProviderProfileDiagnostic, | ||
| ProviderProfileImportItem, RejectDraftChunkRequest, RevokeSshSessionRequest, | ||
| RotateProviderCredentialRequest, Sandbox, SandboxPhase, SandboxPolicy, | ||
| SandboxResourceRequirements, SandboxSpec, SandboxTemplate, ServiceEndpointResponse, | ||
| SetClusterInferenceRequest, SettingScope, SettingValue, TcpForwardFrame, TcpForwardInit, | ||
| TcpRelayTarget, UpdateConfigRequest, UpdateProviderRequest, WatchSandboxRequest, | ||
| exec_sandbox_event, setting_value, tcp_forward_init, | ||
| }; | ||
| use openshell_core::settings::{self, SettingValueKind}; | ||
| use openshell_core::{ObjectId, ObjectName}; | ||
| @@ -1745,6 +1746,7 @@ pub async fn sandbox_create( | ||
| keep: bool, | ||
| gpu: bool, | ||
| gpu_device: Option<&str>, | ||
| gpu_count: Option<u32>, | ||
| cpu: Option<&str>, | ||
| memory: Option<&str>, | ||
Comment on lines
1747
to
1751
MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For follow-up: Let's add a struct that handles these resources together instead of adding new arguments. | ||
| driver_config_json: Option<&str>, | ||
| @@ -1799,8 +1801,6 @@ pub async fn sandbox_create( | ||
| } | ||
| None => None, | ||
| }; | ||
| let requested_gpu = gpu || image.as_deref().is_some_and(image_requests_gpu); | ||
| let providers_v2_enabled = gateway_providers_v2_enabled(&mut client).await?; | ||
| let inferred_types: Vec<String> = if providers_v2_enabled { | ||
| Vec::new() | ||
| @@ -1820,6 +1820,11 @@ pub async fn sandbox_create( | ||
| let driver_config = driver_config_json | ||
| .map(parse_driver_config_json) | ||
| .transpose()?; | ||
| let resource_requirements = | ||
| resource_requirements_from_cli(image.as_deref(), gpu, gpu_device, gpu_count); | ||
| let requested_gpu = resource_requirements | ||
| .as_ref() | ||
| .is_some_and(|requirements| requirements.gpu.is_some()); | ||
| let template = if image.is_some() || resource_limits.is_some() || driver_config.is_some() { | ||
| Some(SandboxTemplate { | ||
| @@ -1834,8 +1839,7 @@ pub async fn sandbox_create( | ||
| let request = CreateSandboxRequest { | ||
| spec: Some(SandboxSpec { | ||
| gpu: requested_gpu, | ||
| gpu_device: gpu_device.unwrap_or_default().to_string(), | ||
| resource_requirements, | ||
| policy, | ||
| providers: configured_providers, | ||
| template, | ||
| @@ -2270,6 +2274,29 @@ pub async fn sandbox_create( | ||
| } | ||
| } | ||
| fn resource_requirements_from_cli( | ||
| image: Option<&str>, | ||
| gpu: bool, | ||
| gpu_device: Option<&str>, | ||
| gpu_count: Option<u32>, | ||
| ) -> Option<SandboxResourceRequirements> { | ||
| let device_ids = gpu_device | ||
| .filter(|device_id| !device_id.is_empty()) | ||
| .map(|device_id| vec![device_id.to_string()]) | ||
| .unwrap_or_default(); | ||
| let requested_gpu = gpu | ||
| || gpu_count.is_some() | ||
| || !device_ids.is_empty() | ||
| || image.is_some_and(image_requests_gpu); | ||
| requested_gpu.then_some(SandboxResourceRequirements { | ||
| gpu: Some(GpuResourceRequirement { | ||
| device_ids, | ||
| count: gpu_count, | ||
| }), | ||
| }) | ||
| } | ||
| /// Resolved source for the `--from` flag on `sandbox create`. | ||
| #[derive(Debug)] | ||
| enum ResolvedSource { | ||
| @@ -7525,8 +7552,8 @@ mod tests { | ||
| parse_credential_pairs, parse_driver_config_json, plaintext_gateway_is_remote, | ||
| progress_step_from_metadata, provider_profile_allows_refresh_bootstrap, | ||
| provisioning_timeout_message, ready_false_condition_message, refresh_status_header, | ||
| refresh_status_row, resolve_from, sandbox_should_persist, sandbox_upload_plan, | ||
| service_expose_status_error, service_url_for_gateway, | ||
| refresh_status_row, resolve_from, resource_requirements_from_cli, sandbox_should_persist, | ||
| sandbox_upload_plan, service_expose_status_error, service_url_for_gateway, | ||
| }; | ||
| use crate::TEST_ENV_LOCK; | ||
| use hyper::StatusCode; | ||
| @@ -8055,6 +8082,67 @@ mod tests { | ||
| } | ||
| } | ||
| #[test] | ||
| fn resource_requirements_from_cli_uses_presence_for_default_gpu() { | ||
| let requirements = resource_requirements_from_cli(None, true, None, None) | ||
| .expect("resource requirements should be present"); | ||
| let gpu = requirements.gpu.expect("GPU requirement should be present"); | ||
| assert!(gpu.device_ids.is_empty()); | ||
| assert_eq!(gpu.count, None); | ||
| } | ||
| #[test] | ||
| fn resource_requirements_from_cli_maps_gpu_device_to_one_device_id() { | ||
| let requirements = resource_requirements_from_cli(None, false, Some("0000:2d:00.0"), None) | ||
| .expect("resource requirements should be present"); | ||
| let gpu = requirements.gpu.expect("GPU requirement should be present"); | ||
| assert_eq!(gpu.device_ids, vec!["0000:2d:00.0"]); | ||
| assert_eq!(gpu.count, None); | ||
| } | ||
| #[test] | ||
| fn resource_requirements_from_cli_maps_gpu_count() { | ||
| let requirements = resource_requirements_from_cli(None, false, None, Some(2)) | ||
| .expect("requirements should exist"); | ||
| let gpu = requirements.gpu.expect("GPU requirement should be present"); | ||
| assert!(gpu.device_ids.is_empty()); | ||
| assert_eq!(gpu.count, Some(2)); | ||
| } | ||
| #[test] | ||
| fn resource_requirements_from_cli_preserves_device_and_gpu_count_for_gateway_validation() { | ||
| let requirements = | ||
| resource_requirements_from_cli(None, false, Some("nvidia.com/gpu=0"), Some(2)) | ||
| .expect("requirements should exist"); | ||
| let gpu = requirements.gpu.expect("GPU requirement should be present"); | ||
| assert_eq!(gpu.device_ids, vec!["nvidia.com/gpu=0"]); | ||
| assert_eq!(gpu.count, Some(2)); | ||
| } | ||
| #[test] | ||
| fn resource_requirements_from_cli_omits_gpu_request_when_not_requested() { | ||
| assert!(resource_requirements_from_cli(None, false, None, None).is_none()); | ||
| } | ||
| #[test] | ||
| fn resource_requirements_from_cli_infers_gpu_from_image() { | ||
| let requirements = resource_requirements_from_cli( | ||
| Some("ghcr.io/nvidia/openshell-community/sandboxes/nvidia-gpu:latest"), | ||
| false, | ||
| None, | ||
| None, | ||
| ) | ||
| .expect("resource requirements should be present"); | ||
| let gpu = requirements.gpu.expect("GPU requirement should be present"); | ||
| assert!(gpu.device_ids.is_empty()); | ||
| assert_eq!(gpu.count, None); | ||
| } | ||
| #[test] | ||
| fn resolve_from_classifies_existing_dockerfile_path() { | ||
| let temp = tempfile::tempdir().expect("failed to create tempdir"); | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
As a follow-up: Does it make sense to introduce a type for gpu and other resources going forward?