diff --git a/src/apps/containers.clj b/src/apps/containers.clj index cd2316b6..03cc5a6c 100644 --- a/src/apps/containers.clj +++ b/src/apps/containers.clj @@ -7,11 +7,13 @@ container-devices container-volumes container-volumes-from + container-gpu-models data-containers interapps-proxy-settings ports]] [apps.persistence.tools :refer [update-tool]] [apps.util.assertions :refer [assert-not-nil]] + [apps.util.config :as config] [apps.util.conversions :refer [remove-nil-vals remove-empty-vals]] [apps.util.db :refer [transaction]] [apps.validation :refer [validate-image-not-used-in-public-apps @@ -19,6 +21,7 @@ validate-tool-not-used-in-public-apps]] [cheshire.core :as json] [clojure.tools.logging :as log] + [clojure.string :as string] [clojure-commons.exception-util :as cxu] [kameleon.uuids :refer [uuidify]] [korma.core :as sql]) @@ -160,6 +163,36 @@ (select-keys volume-map [:host_path :container_path]) {:container_settings_id (uuidify settings-uuid)})))) +(defn- gpu-model-mapping? + "Returns true if the combination of container_settings UUID and GPU model + already exists in the database." + [settings-uuid gpu-model] + (pos? (count (sql/select container-gpu-models + (sql/where (and (= :container_settings_id (uuidify settings-uuid)) + (= :gpu_model gpu-model))))))) + +(defn- validate-gpu-model + "Validates that a GPU model is in the list of valid GPU models." + [gpu-model] + (let [valid-models (set (config/valid-gpu-models))] + (when-not (contains? valid-models gpu-model) + (cxu/bad-request (str "Invalid GPU model: " gpu-model ". Valid models are: " (string/join ", " valid-models)))))) + +(defn- add-gpu-model + "Associates a GPU model with the given container_settings UUID." + [settings-uuid gpu-model] + (validate-gpu-model gpu-model) + (when (gpu-model-mapping? settings-uuid gpu-model) + (cxu/exists (str "GPU model mapping already exists: " settings-uuid " " gpu-model))) + (sql/insert container-gpu-models + (sql/values {:container_settings_id (uuidify settings-uuid) + :gpu_model gpu-model}))) + +(defn list-valid-gpu-models + "Returns the list of valid GPU models that can be configured." + [] + {:gpu_models (vec (config/valid-gpu-models))}) + (defn- data-container "Returns a map describing a data container." [data-container-id] @@ -331,12 +364,15 @@ (sql/fields :name_prefix :read_only) (sql/with container-images (sql/fields :name :tag :url :deprecated :osg_image_path)))) + (sql/with container-gpu-models + (sql/fields :gpu_model)) (sql/with ports (sql/fields :host_port :container_port :bind_to_host :id)) (sql/where {:tools_id id})) first add-interapps-info (update :container_volumes_from add-data-container-auth :auth? auth?) + (update :gpu_models #(mapv :gpu_model %)) (merge {:image (tool-image-info tool-uuid :auth? auth?)}) filter-returns)))) @@ -394,6 +430,7 @@ volumes (:container_volumes info-map) vfs (:container_volumes_from info-map) ports (:container_ports info-map) + gpu-models (:gpu_models info-map) proxy-settings (:interactive_apps info-map) info-map (assoc info-map :tools_id (uuidify tool-uuid))] (log/warn "adding container information for tool" tool-uuid ":" info-map) @@ -414,13 +451,15 @@ (add-settings-volumes-from settings-uuid vf)) (doseq [p ports] (add-port settings-uuid p)) + (doseq [gm gpu-models] + (add-gpu-model settings-uuid gm)) (tool-container-info tool-uuid))))) (defn set-tool-container "Removes all existing container settings for the given tool-id, replacing them with the given settings." [tool-id overwrite-public - {:keys [container_devices container_volumes container_volumes_from container_ports interactive_apps] :as settings}] + {:keys [container_devices container_volumes container_volumes_from container_ports gpu_models interactive_apps] :as settings}] (when-not overwrite-public (validate-tool-not-used-in-public-apps tool-id)) (transaction @@ -441,5 +480,7 @@ (add-settings-volumes-from settings-id volume)) (doseq [port container_ports] (add-port settings-id port)) + (doseq [gpu-model gpu_models] + (add-gpu-model settings-id gpu-model)) (tool-container-info tool-id)))) diff --git a/src/apps/persistence/app_metadata.clj b/src/apps/persistence/app_metadata.clj index c95461cc..0b837c7e 100644 --- a/src/apps/persistence/app_metadata.clj +++ b/src/apps/persistence/app_metadata.clj @@ -1076,19 +1076,27 @@ (defn get-resource-requirements-for-task [task-id] - (-> (select* [entities/container-settings :c]) - (join [:tasks :t] {:t.tool_id :c.tools_id}) - (fields :memory_limit - :min_memory_limit - :min_cpu_cores - :max_cpu_cores - :min_gpus - :max_gpus - :min_disk_space) - (where {:t.id task-id}) - select - first - remove-nil-vals)) + (when-let [reqs (-> (select* [entities/container-settings :c]) + (join [:tasks :t] {:t.tool_id :c.tools_id}) + (fields :c.id + :memory_limit + :min_memory_limit + :min_cpu_cores + :max_cpu_cores + :min_gpus + :max_gpus + :min_disk_space) + (where {:t.id task-id}) + select + first + remove-nil-vals)] + (let [gpu-models (mapv :gpu_model + (select entities/container-gpu-models + (fields :gpu_model) + (where {:container_settings_id (:id reqs)})))] + (-> reqs + (dissoc :id) + (cond-> (seq gpu-models) (assoc :gpu_models gpu-models)))))) (defn list-app-publication-requests [app-id requestor include-completed] diff --git a/src/apps/persistence/entities.clj b/src/apps/persistence/entities.clj index c2c474ac..beae1990 100644 --- a/src/apps/persistence/entities.clj +++ b/src/apps/persistence/entities.clj @@ -51,7 +51,8 @@ container-settings container-devices container-volumes - container-volumes-from) + container-volumes-from + container-gpu-models) ;; Users who have logged into the DE. Multiple entities are associated with ;; the same table in order to allow us to have multiple relationships between @@ -129,6 +130,7 @@ (sql/has-many container-devices) (sql/has-many container-volumes) (sql/has-many container-volumes-from) + (sql/has-many container-gpu-models) (sql/has-many ports) (sql/has-one interapps-proxy-settings)) @@ -145,6 +147,10 @@ (sql/belongs-to container-settings) (sql/belongs-to data-containers)) +(sql/defentity container-gpu-models + (sql/table :container_gpu_models :gpu_models) + (sql/belongs-to container-settings)) + ;; Information about a deployed tool. (sql/defentity tools (sql/belongs-to integration_data) diff --git a/src/apps/routes.clj b/src/apps/routes.clj index c6920bc7..280a31ee 100644 --- a/src/apps/routes.clj +++ b/src/apps/routes.clj @@ -57,6 +57,7 @@ {:name "pipelines", :description "Pipeline endpoints."} {:name "analyses", :description "Analysis endpoints."} {:name "bootstrap", :description "Bootstrap endpoints."} + {:name "gpu-models", :description "GPU Model Configuration endpoints."} {:name "tools", :description "Tool endpoints."} {:name "workspaces", :description "Workspace endpoints."} {:name "webhooks", :description "Webhooks endpoints."} @@ -132,6 +133,9 @@ (schema/context "/bootstrap" [] :tags ["bootstrap"] bootstrap-routes/bootstrap) + (schema/context "/tools/gpu-models" [] + :tags ["gpu-models"] + tool-routes/gpu-models) (schema/context "/tools" [] :tags ["tools"] tool-routes/tools) diff --git a/src/apps/routes/schemas/containers.clj b/src/apps/routes/schemas/containers.clj index 8a2ef148..6c2f2bda 100644 --- a/src/apps/routes/schemas/containers.clj +++ b/src/apps/routes/schemas/containers.clj @@ -35,3 +35,13 @@ (->optional-param :name) (dissoc :id)) "A map for updating data container settings.")) + +(s/defschema GpuModel + (describe + s/Str + "A GPU model name (e.g., 'NVIDIA-A16').")) + +(s/defschema GpuModels + (describe + {:gpu_models [GpuModel]} + "A list of valid GPU model names.")) diff --git a/src/apps/routes/tools.clj b/src/apps/routes/tools.clj index 82dd54a6..210bfe74 100644 --- a/src/apps/routes/tools.clj +++ b/src/apps/routes/tools.clj @@ -7,6 +7,7 @@ image-info image-public-tools list-images + list-valid-gpu-models modify-data-container modify-image-info]] [apps.metadata.tool-requests :as tool-requests] @@ -14,6 +15,7 @@ [apps.routes.schemas.containers :refer [DataContainerIdParam DataContainerUpdateRequest + GpuModels ImageId ImageUpdateParams ImageUpdateRequest @@ -120,6 +122,14 @@ :description "Updates a data container's settings." (ok (modify-data-container data-container-id body)))) +(defroutes gpu-models + (GET "/" [] + :query [params SecuredQueryParams] + :return GpuModels + :summary "List Valid GPU Models" + :description "Returns the list of valid GPU model names that can be configured for tools." + (ok (list-valid-gpu-models)))) + (defroutes tools (GET "/" [] :query [params ToolSearchParams] diff --git a/src/apps/service/apps/de/job_view.clj b/src/apps/service/apps/de/job_view.clj index 0d7243e8..3c9bfaf4 100644 --- a/src/apps/service/apps/de/job_view.clj +++ b/src/apps/service/apps/de/job_view.clj @@ -23,11 +23,14 @@ (defn- format-step-resource-requirements [requirements step-number add-defaults?] (if add-defaults? - (merge {:max_cpu_cores (config/default-cpu-limit) - :memory_limit (config/default-memory-limit) - :max_gpus (config/default-gpu-limit) - :step_number step-number} - requirements) + (let [defaults {:max_cpu_cores (config/default-cpu-limit) + :memory_limit (config/default-memory-limit) + :max_gpus (config/default-gpu-limit) + :step_number step-number} + merged (merge defaults requirements)] + (if (and (seq (config/default-gpu-models)) (empty? (:gpu_models merged))) + (assoc merged :gpu_models (vec (config/default-gpu-models))) + merged)) (assoc requirements :step_number step-number))) (defn- get-step-resource-requirements diff --git a/src/apps/service/apps/de/jobs/common.clj b/src/apps/service/apps/de/jobs/common.clj index be059c4d..604f463d 100644 --- a/src/apps/service/apps/de/jobs/common.clj +++ b/src/apps/service/apps/de/jobs/common.clj @@ -9,6 +9,7 @@ [apps.tools :as t] [apps.util.assertions :refer [assert-not-nil]] [apps.util.conversions :refer [remove-nil-vals]] + [clojure.set :as set] [clojure.string :as string] [kameleon.uuids :refer [uuid]] [korma.core :refer [fields join order select select* where]] @@ -54,6 +55,18 @@ (mapcat (partial build-environment-entries config default-values)) (into {}))) +(defn- reconcile-gpu-models + "Returns the effective GPU model list for a job step. + Filters user-requested models to only those in the tool's allowed set. + Uses the filtered user subset if non-empty, otherwise falls back to + the tool's full list. Returns nil if the tool has no GPU models." + [container requirements] + (let [tool-models (set (:gpu_models container)) + user-models (set (:gpu_models requirements)) + valid-user (set/intersection user-models tool-models)] + (when (seq tool-models) + (vec (if (seq valid-user) valid-user tool-models))))) + (defn- reconcile-container-requirements "reconcile submission requirement requests with tool requirements" [container requirements] @@ -64,7 +77,8 @@ :max_cpu_cores (resources/get-max-cpus container requirements) :min_gpus (resources/get-required-gpus container requirements) :max_gpus (resources/get-max-gpus container requirements) - :min_disk_space (resources/get-required-disk-space container requirements)) + :min_disk_space (resources/get-required-disk-space container requirements) + :gpu_models (reconcile-gpu-models container requirements)) remove-nil-vals)) (defn- add-container-info diff --git a/src/apps/service/apps/jobs/params.clj b/src/apps/service/apps/jobs/params.clj index 43649fca..6b9218c4 100644 --- a/src/apps/service/apps/jobs/params.clj +++ b/src/apps/service/apps/jobs/params.clj @@ -161,13 +161,15 @@ :min_gpus :max_gpus :min_memory_limit - :min_disk_space]) + :min_disk_space + :gpu_models]) (sets/rename-keys {:max_cpu_cores :default_max_cpu_cores :min_cpu_cores :default_cpu_cores :min_gpus :default_gpus :max_gpus :default_max_gpus :min_memory_limit :default_memory - :min_disk_space :default_disk_space}))))) + :min_disk_space :default_disk_space + :gpu_models :default_gpu_models}))))) (defn- update-resources-reqs "Converts resource requests from the original submission JSON into default requirement settings, diff --git a/src/apps/tools.clj b/src/apps/tools.clj index bc440c68..7b44fcf7 100644 --- a/src/apps/tools.clj +++ b/src/apps/tools.clj @@ -57,10 +57,13 @@ (defn format-container-settings [container-settings include-defaults] (if include-defaults - (merge {:max_cpu_cores (config/default-cpu-limit) - :memory_limit (config/default-memory-limit) - :max_gpus (config/default-gpu-limit)} - container-settings) + (let [defaults {:max_cpu_cores (config/default-cpu-limit) + :memory_limit (config/default-memory-limit) + :max_gpus (config/default-gpu-limit)} + merged (merge defaults container-settings)] + (if (and (seq (config/default-gpu-models)) (empty? (:gpu_models merged))) + (assoc merged :gpu_models (vec (config/default-gpu-models))) + merged)) container-settings)) (defn- filter-listing-tool-ids diff --git a/src/apps/util/config.clj b/src/apps/util/config.clj index b84e7ba8..b025e9b1 100644 --- a/src/apps/util/config.clj +++ b/src/apps/util/config.clj @@ -144,6 +144,16 @@ [props config-valid configs] "apps.tools.default.gpu-limit" 0) +(cc/defprop-optvec valid-gpu-models + "The list of valid GPU model names that can be configured for tools." + [props config-valid configs] + "apps.tools.valid-gpu-models" ["NVIDIA-A16"]) + +(cc/defprop-optvec default-gpu-models + "The default GPU models to use when a tool does not specify any and defaults are requested." + [props config-valid configs] + "apps.tools.default.gpu-models" []) + (cc/defprop-optstr workspace-root-app-category "The name of the root app category in a user's workspace." [props config-valid configs] diff --git a/test.properties b/test.properties index d8ca9548..96f90830 100644 --- a/test.properties +++ b/test.properties @@ -41,6 +41,9 @@ apps.batch.path-list.info-type = ht-analysis-path-list apps.batch.path-list.max-paths = 16 apps.batch.path-list.max-size = 1048576 +# Tool settings. +apps.tools.valid-gpu-models = ["NVIDIA-A16", "NVIDIA-A100-SXM4-40GB", "NVIDIA-A100-SXM4-80GB"] + # Agave connection settings. apps.agave.base-url = https://agave.iplantc.org apps.agave.key = not_a_key diff --git a/test/apps/service/apps/de/job_view_test.clj b/test/apps/service/apps/de/job_view_test.clj index 43199896..04b723c2 100644 --- a/test/apps/service/apps/de/job_view_test.clj +++ b/test/apps/service/apps/de/job_view_test.clj @@ -37,3 +37,24 @@ (some? (:max_gpus result)) (not (contains? requirements :max_gpus)))) "Should not add max_gpus default when add-defaults is false")))) + +;; --------------------------------------------------------------------------- +;; GPU model preservation and non-injection +;; --------------------------------------------------------------------------- + +(deftest test-format-step-resource-requirements-preserves-gpu-models + (testing "Existing gpu_models in requirements are preserved by format-step-resource-requirements" + (let [requirements {:gpu_models ["A16" "A40"] + :max_gpus 2} + step-number 1 + result (#'job-view/format-step-resource-requirements requirements step-number true)] + (is (= ["A16" "A40"] (:gpu_models result)) + "Should preserve the tool's gpu_models list when present")))) + +(deftest test-format-step-resource-requirements-no-gpu-models-when-not-interactive + (testing "gpu_models not injected when add-defaults is false, even if absent" + (let [requirements {:max_gpus 2} + step-number 1 + result (#'job-view/format-step-resource-requirements requirements step-number false)] + (is (not (contains? result :gpu_models)) + "Should not inject gpu_models when add-defaults is false")))) diff --git a/test/apps/service/apps/de/jobs/common_test.clj b/test/apps/service/apps/de/jobs/common_test.clj index 7245becb..e3604db1 100644 --- a/test/apps/service/apps/de/jobs/common_test.clj +++ b/test/apps/service/apps/de/jobs/common_test.clj @@ -67,3 +67,89 @@ "Should not include min_gpus when not specified") (is (not (contains? result :max_gpus)) "Should not include max_gpus when not specified")))) + +;; --------------------------------------------------------------------------- +;; reconcile-gpu-models — direct tests of the GPU model intersection logic +;; --------------------------------------------------------------------------- + +(deftest test-reconcile-gpu-models-user-subset + (testing "User's valid subset of tool models is used" + (let [container {:gpu_models ["A16" "A40" "A100"]} + requirements {:gpu_models ["A16" "A40"]} + result (#'common/reconcile-gpu-models container requirements)] + (is (= (set result) #{"A16" "A40"}) + "Should return exactly the user's valid selections")))) + +(deftest test-reconcile-gpu-models-invalid-dropped + (testing "Invalid user selections are silently dropped" + (let [container {:gpu_models ["A16" "A40"]} + requirements {:gpu_models ["A16" "BOGUS"]} + result (#'common/reconcile-gpu-models container requirements)] + (is (= (set result) #{"A16"}) + "Should keep only models that appear in the tool's allowed list")))) + +(deftest test-reconcile-gpu-models-all-invalid-falls-back + (testing "Falls back to full tool list when all user selections are invalid" + (let [container {:gpu_models ["A16" "A40"]} + requirements {:gpu_models ["BOGUS"]} + result (#'common/reconcile-gpu-models container requirements)] + (is (= (set result) #{"A16" "A40"}) + "Should fall back to full tool list when intersection is empty")))) + +(deftest test-reconcile-gpu-models-empty-user-falls-back + (testing "Falls back to full tool list when user sends empty gpu_models" + (let [container {:gpu_models ["A16" "A40"]} + requirements {:gpu_models []} + result (#'common/reconcile-gpu-models container requirements)] + (is (= (set result) #{"A16" "A40"}) + "Should fall back to full tool list when user list is empty")))) + +(deftest test-reconcile-gpu-models-nil-user-falls-back + (testing "Falls back to full tool list when user gpu_models is nil" + (let [container {:gpu_models ["A16" "A40"]} + requirements {} + result (#'common/reconcile-gpu-models container requirements)] + (is (= (set result) #{"A16" "A40"}) + "Should fall back to full tool list when user list is nil")))) + +(deftest test-reconcile-gpu-models-tool-no-models-returns-nil + (testing "Returns nil when tool has no GPU models" + (let [result-empty (#'common/reconcile-gpu-models {:gpu_models []} {:gpu_models ["A16"]}) + result-nil (#'common/reconcile-gpu-models {} {:gpu_models ["A16"]})] + (is (nil? result-empty) + "Should return nil when tool's gpu_models is empty") + (is (nil? result-nil) + "Should return nil when tool has no gpu_models key")))) + +;; --------------------------------------------------------------------------- +;; reconcile-container-requirements — GPU model field in end-to-end result +;; --------------------------------------------------------------------------- + +(deftest test-reconcile-container-requirements-gpu-models-intersection + (testing "Container requirements result includes gpu_models as the user/tool intersection" + (let [container {:min_gpus 0 :max_gpus 4 + :gpu_models ["A16" "A40" "A100"]} + requirements {:min_gpus 1 :max_gpus 2 + :gpu_models ["A16" "A100"]} + result (#'common/reconcile-container-requirements container requirements)] + (is (= (set (:gpu_models result)) #{"A16" "A100"}) + "Should contain the intersection of user and tool GPU models")))) + +(deftest test-reconcile-container-requirements-gpu-models-fallback + (testing "Container requirements falls back to full tool model list when user list is empty" + (let [container {:min_gpus 0 :max_gpus 4 + :gpu_models ["A16" "A40"]} + requirements {:min_gpus 1 :max_gpus 2 + :gpu_models []} + result (#'common/reconcile-container-requirements container requirements)] + (is (= (set (:gpu_models result)) #{"A16" "A40"}) + "Should fall back to full tool list when user sends empty gpu_models")))) + +(deftest test-reconcile-container-requirements-gpu-models-absent-when-tool-has-none + (testing "gpu_models key is absent from result when tool has no GPU models" + (let [container {:min_gpus 0 :max_gpus 2} + requirements {:min_gpus 1 :max_gpus 2 + :gpu_models ["A16"]} + result (#'common/reconcile-container-requirements container requirements)] + (is (not (contains? result :gpu_models)) + "gpu_models key must be absent (not nil or empty) when tool has no models")))) diff --git a/test/apps/service/apps/jobs/params_test.clj b/test/apps/service/apps/jobs/params_test.clj new file mode 100644 index 00000000..9aee5e32 --- /dev/null +++ b/test/apps/service/apps/jobs/params_test.clj @@ -0,0 +1,67 @@ +(ns apps.service.apps.jobs.params-test + (:require + [apps.service.apps.jobs.params :as params] + [clojure.test :as t :refer [deftest is testing]])) + +;; --------------------------------------------------------------------------- +;; update-resource-reqs — step matching and key renaming +;; --------------------------------------------------------------------------- + +(deftest test-update-resource-reqs-renames-all-gpu-keys + (testing "All resource requirement keys including GPU fields are correctly renamed" + (let [requested-reqs [{:step_number 0 + :max_cpu_cores 4 + :min_cpu_cores 2 + :min_gpus 1 + :max_gpus 3 + :min_memory_limit 2147483648 + :min_disk_space 1073741824 + :gpu_models ["A16" "A40"]}] + step-reqs {:step_number 0} + result (#'params/update-resource-reqs requested-reqs step-reqs)] + (is (= 4 (:default_max_cpu_cores result)) + "max_cpu_cores should be renamed to default_max_cpu_cores") + (is (= 2 (:default_cpu_cores result)) + "min_cpu_cores should be renamed to default_cpu_cores") + (is (= 1 (:default_gpus result)) + "min_gpus should be renamed to default_gpus") + (is (= 3 (:default_max_gpus result)) + "max_gpus should be renamed to default_max_gpus") + (is (= 2147483648 (:default_memory result)) + "min_memory_limit should be renamed to default_memory") + (is (= 1073741824 (:default_disk_space result)) + "min_disk_space should be renamed to default_disk_space") + (is (= ["A16" "A40"] (:default_gpu_models result)) + "gpu_models should be renamed to default_gpu_models")))) + +(deftest test-update-resource-reqs-wrong-step-returns-nil + (testing "Returns nil when no requested requirement matches the step number" + (let [requested-reqs [{:step_number 0 :max_cpu_cores 4}] + step-reqs {:step_number 1} + result (#'params/update-resource-reqs requested-reqs step-reqs)] + (is (nil? result) + "Should return nil when step numbers don't match")))) + +(deftest test-update-resource-reqs-missing-gpu-models-not-included + (testing "No default_gpu_models key when gpu_models is absent from the request" + (let [requested-reqs [{:step_number 0 :max_cpu_cores 4 :min_gpus 1}] + step-reqs {:step_number 0} + result (#'params/update-resource-reqs requested-reqs step-reqs)] + (is (not (contains? result :default_gpu_models)) + "Should not contain default_gpu_models when gpu_models was not in request") + (is (= 1 (:default_gpus result)) + "Other GPU fields should still be renamed")))) + +(deftest test-update-resource-reqs-merges-with-step-reqs + (testing "Existing step-reqs fields are preserved alongside renamed request fields" + (let [requested-reqs [{:step_number 0 :max_gpus 2 :gpu_models ["A16"]}] + step-reqs {:step_number 0 :memory_limit 4294967296 :max_cpu_cores 8} + result (#'params/update-resource-reqs requested-reqs step-reqs)] + (is (= 4294967296 (:memory_limit result)) + "Existing memory_limit from step-reqs should be preserved") + (is (= 8 (:max_cpu_cores result)) + "Existing max_cpu_cores from step-reqs should be preserved") + (is (= 2 (:default_max_gpus result)) + "Renamed max_gpus from request should be present") + (is (= ["A16"] (:default_gpu_models result)) + "Renamed gpu_models from request should be present"))))