Skip to content
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

auto_qos form widget displays duplicate QOS #3933

Open
simonLeary42 opened this issue Nov 6, 2024 · 2 comments · May be fixed by #3955
Open

auto_qos form widget displays duplicate QOS #3933

simonLeary42 opened this issue Nov 6, 2024 · 2 comments · May be fixed by #3955
Assignees
Labels
bug Existing functionality not working as expected
Milestone

Comments

@simonLeary42
Copy link

simonLeary42 commented Nov 6, 2024

image

By inserting a log statement into /var/www/ood/apps/sys/dashboard/app/lib/smart_attributes/attributes/auto_qos.rb, I got the list of options for the widget, and I see the issue:

{:options=>[[\"long\", \"long\", {\"data-option-for-auto-accounts-pi_tbernard_umass_edu\"=>false}], [\"normal\", \"normal\", {\"data-option-for-auto-accounts-pi_tbernard_umass_edu\"=>false}], [\"short\", \"short\", {\"data-option-for-auto-accounts-pi_tbernard_umass_edu\"=>false}], [\"long\", \"long\", {\"data-option-for-auto-accounts-pi_simonleary_umass_edu\"=>false}], [\"normal\", \"normal\", {\"data-option-for-auto-accounts-pi_simonleary_umass_edu\"=>false}], [\"short\", \"short\", {\"data-option-for-auto-accounts-pi_simonleary_umass_edu\"=>false}]]}

These QOSes are available to my user from multiple slurm accounts. I'm not sure what is the significance of option-for-auto-accounts-*, but I would think that they can be combined into the same list.

--- /var/www/ood/apps/sys/dashboard.bak/app/lib/smart_attributes/attributes/auto_qos.rb	2024-10-22 20:29:51.000000000 +0000
+++ /var/www/ood/apps/sys/dashboard/app/lib/smart_attributes/attributes/auto_qos.rb	2024-11-06 06:08:37.187317768 +0000
@@ -12,11 +12,20 @@
     # @param opts [Hash] attribute's options
     # @return [Attributes::AutoQueues] the attribute object
     def self.build_auto_qos(opts = {})
-
+      # don't add the same QOS to options multiple times with different data options,
+      # just combine the data options
+      deduped_dynamic_qos = []
+      dynamic_qos.each do |dynamic_qos_element|
+        found = deduped_dynamic_qos.find { |x| x[0, 2] == dynamic_qos_element[0, 2] }
+        if found
+          found.concat(dynamic_qos_element[2..])
+        else
+          deduped_dynamic_qos.append(dynamic_qos_element)
+        end
+      end
       static_opts = {
-        options: dynamic_qos
+        options: deduped_dynamic_qos
       }.merge(opts.without(:options).to_h)

       Attributes::AutoQos.new('auto_qos', static_opts)
     end
   end

looking good.

image

{:options=>[[\"long\", \"long\", {\"data-option-for-auto-accounts-pi_tbernard_umass_edu\"=>false}, {\"data-option-for-auto-accounts-pi_simonleary_umass_edu\"=>false}], [\"normal\", \"normal\", {\"data-option-for-auto-accounts-pi_tbernard_umass_edu\"=>false}, {\"data-option-for-auto-accounts-pi_simonleary_umass_edu\"=>false}], [\"short\", \"short\", {\"data-option-for-auto-accounts-pi_tbernard_umass_edu\"=>false}, {\"data-option-for-auto-accounts-pi_simonleary_umass_edu\"=>false}]]}"

I would submit this as a merge request but I feel like this deduplication should actually happen in OOD core.

@osc-bot osc-bot added this to the Backlog milestone Nov 6, 2024
@johrstrom
Copy link
Contributor

Thanks for the issue. I think it should deduplciate here in OnDemand itself when it has to stitch it all together. I can't say for sure I can get this into 4.0, but I can try.

@johrstrom johrstrom modified the milestones: Backlog, 4.0 Nov 6, 2024
@johrstrom johrstrom added the bug Existing functionality not working as expected label Nov 6, 2024
@johrstrom johrstrom self-assigned this Nov 14, 2024
@johrstrom johrstrom linked a pull request Nov 14, 2024 that will close this issue
@johrstrom
Copy link
Contributor

Thanks again for the ticket. I guess I missed your note about patching it. In any case, I updated the logic in dynamic_qos directly instead of modifying the smart attribute there.

At any rate, thanks for opening the ticket and providing a solution (even if I missed it)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Existing functionality not working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants