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.

Sign up for GitHub

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

Jump to bottom

Pm job management #3899

Merged
merged 14 commits into from
Nov 15, 2024
Merged

Pm job management #3899

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
12ac653
add buttons to the top and bottom of job info
ashton22305 Oct 4, 2024
ae7d979
working delete button
ashton22305 Oct 18, 2024
8dcbb56
redirect after delete and only enable delete button for completed jobs
ashton22305 Oct 18, 2024
dc09e18
better delete button formatting
ashton22305 Oct 18, 2024
866909a
button formatting and create routes to hold and stop jobs
ashton22305 Oct 18, 2024
90acf35
remove hold buttons for now and make stop button functional
ashton22305 Oct 24, 2024
ef811af
handle case where job is already completed but not updated on page
ashton22305 Oct 25, 2024
77f4355
change PATCH to POST
ashton22305 Oct 25, 2024
6a3cb34
add flash message and render instead of redirecting
ashton22305 Oct 28, 2024
815fbb9
pass project instead of using safe navigation
ashton22305 Oct 28, 2024
8ed04a7
add notices and alerts when stopping or deleting jobs
ashton22305 Nov 8, 2024
bee998d
remove_logged_job returns boolean
ashton22305 Nov 8, 2024
152333a
add data confirm to destructive buttons
ashton22305 Nov 14, 2024
1797a92
Merge branch 'master' into pm-job-management
ashton22305 Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion apps/dashboard/app/controllers/projects_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,45 @@ def job_details

hpc_job = project.job(job_details_params[:jobid].to_s, cluster_str)

render(partial: 'job_details', locals: { job: hpc_job })
@project = project

render(partial: 'job_details', locals: { job: hpc_job, project: @project })
end

# DELETE /projects/:project_id/jobs/:cluster/:jobid
def delete_job
@project = Project.find(job_details_params[:project_id])

cluster_str = job_details_params[:cluster].to_s

@project.remove_logged_job(job_details_params[:jobid].to_s, cluster_str)

@valid_project = Launcher.clusters?
@valid_scripts = Launcher.scripts?(@project.directory)
@scripts = Launcher.all(@project.directory)

render :show
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should follow a pattern here where remove_logged_job returns true or false based on whether the operation was successful. Then the controller toggles based off of that.

Like say how update, destroy, create or works in this same controller. I think redirecting here is fine (so that we don't have to create/recreate those attributes), it just seems like the toggle is redirect with a notice when successful or an alert for a failure.

end

# POST /projects/:project_id/jobs/:cluster/:jobid/stop
def stop_job
@project = Project.find(job_details_params[:project_id])
cluster_str = job_details_params[:cluster].to_s
cluster = OodAppkit.clusters[cluster_str.to_sym]

hpc_job = @project.job(job_details_params[:jobid].to_s, cluster_str)

begin
cluster.job_adapter.delete(job_details_params[:jobid].to_s) unless hpc_job.status.to_s == 'completed'
rescue StandardError => e
flash.now[:alert] = I18n.t('dashboard.jobs_project_generic_error', error: e.message.to_s)
end

@valid_project = Launcher.clusters?
@valid_scripts = Launcher.scripts?(@project.directory)
@scripts = Launcher.all(@project.directory)

render :show
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment to above, I think we should redirect instead of creating all these objects all over again.

Also seems like you need a notice in the case that it was successful.

end

private
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/app/helpers/application_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def status_text(status)
'Running'
when 'queued'
'Queued'
when 'qued_held'
when 'queued_held'
'Hold'
when 'suspended'
'Suspend'
Expand Down
19 changes: 18 additions & 1 deletion apps/dashboard/app/helpers/projects_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,21 @@ def render_readme(readme_location)
simple_format(file_content)
end
end
end

def job_details_buttons(status, job, project)
locals = { project_id: project.id, id: job.id, cluster: job.cluster }
button_partial = button_category(status)
render(partial: "projects/buttons/#{button_category(status)}_buttons", locals: locals) unless button_partial.nil?
end

def button_category(status)
case status
when 'queued_held'
'held'
when 'suspended'
'held'
else
status
end
end
end
13 changes: 13 additions & 0 deletions apps/dashboard/app/models/concerns/job_logger.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,19 @@ def upsert_job!(directory, job)
JobLoggerHelper.write_log(directory, new_jobs)
end

def delete_job!(directory, job)
existing_jobs = jobs(directory)
stored_job = existing_jobs.detect { |j| j.id == job.id && j.cluster == job.cluster }

return if stored_job.nil?

new_jobs = existing_jobs.map(&:to_h)
idx = existing_jobs.index(stored_job)
new_jobs.delete_at(idx)

JobLoggerHelper.write_log(directory, new_jobs)
end

# def write_job_log!(directory, jobs)
# JobLoggerHelper.write_log!(directory, jobs)
# endd
Expand Down
7 changes: 6 additions & 1 deletion apps/dashboard/app/models/project.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,11 @@ def job(job_id, cluster)
job
end

def remove_logged_job(job_id, cluster)
old_job = jobs.detect { |j| j.id == job_id && j.cluster == cluster }
Project.delete_job!(directory, old_job)
end

def adapter(cluster_id)
cluster = OodAppkit.clusters[cluster_id] || raise(StandardError, "Job specifies nonexistent '#{cluster_id}' cluster id.")
cluster.job_adapter
Expand All @@ -230,7 +235,7 @@ def readme_path
end

private

def update_attrs(attributes)
[:name, :description, :icon].each do |attribute|
instance_variable_set("@#{attribute}".to_sym, attributes.fetch(attribute, ''))
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/app/views/projects/_job_details.turbo_stream.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@

<turbo-stream action="replace" target="<%= id %>" data-job-status="<%= job.status %>">
<template>
<%= render(partial: 'job_details_content', locals: { job: job }, :formats=>[:html]) %>
<%= render(partial: 'job_details_content', locals: { job: job, project: project }, :formats=>[:html]) %>
</template>
</turbo-stream>
3 changes: 3 additions & 0 deletions apps/dashboard/app/views/projects/_job_details_content.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
</tr>
<% end %>
</table>
<div class="d-flex flex-row-reverse mt-2 mx-2">
<%= job_details_buttons(job.status.to_s, job, project) %>
</div>
</div>
</div>
</div>
6 changes: 6 additions & 0 deletions apps/dashboard/app/views/projects/buttons/_completed_buttons.html.erb
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these destructive buttons need an alert confirmation.

All you need to do is add a data-confirm attribute with some string. Here I just pulled a random confirm localized string. You could make a new one or use this, I'm sort of ambivalent on that.

diff --git a/apps/dashboard/app/views/projects/buttons/_completed_buttons.html.erb b/apps/dashboard/app/views/projects/buttons/_completed_buttons.html.erb
index 7d09e06fe..0df5fa971 100644
--- a/apps/dashboard/app/views/projects/buttons/_completed_buttons.html.erb
+++ b/apps/dashboard/app/views/projects/buttons/_completed_buttons.html.erb
@@ -2,5 +2,6 @@
   'Delete',
   project_delete_job_path(project_id: project_id, cluster: cluster, jobid: id),
   method: :delete,
+  data: { confirm: t('dashboard.batch_connect_sessions_delete_confirm') },
   class: 'btn btn-danger'
 ) %>

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<%= button_to(
'Delete',
project_delete_job_path(project_id: project_id, cluster: cluster, jobid: id),
method: :delete,
class: 'btn btn-danger'
) %>
6 changes: 6 additions & 0 deletions apps/dashboard/app/views/projects/buttons/_queued_buttons.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<%= button_to(
'Stop',
project_stop_job_path(project_id: project_id, cluster: cluster, jobid: id),
method: :post,
class: 'btn btn-danger'
) %>
6 changes: 6 additions & 0 deletions apps/dashboard/app/views/projects/buttons/_running_buttons.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<%= button_to(
'Stop',
project_stop_job_path(project_id: project_id, cluster: cluster, jobid: id),
method: :post,
class: 'btn btn-danger'
) %>
4 changes: 2 additions & 2 deletions apps/dashboard/app/views/projects/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,15 @@
<div class="col-md-8">
<div class="row">
<h2 class="h3 d-flex justify-content-center">Active Jobs</h2>
<%= render(partial: 'job_details', collection: @project.active_jobs, as: :job) %>
<%= render(partial: 'job_details', collection: @project.active_jobs, as: :job, locals: { project: @project }) %>
</div>

<div class="row">
<h2 class="h3 d-flex justify-content-center">Completed Jobs</h2>
<div id="completed_jobs" class="row">
<%- @project.completed_jobs.each do |job| -%>
<div class="col-md-4" id="<%= "job_#{job.cluster}_#{job.id}" %>">
<%= render(partial: 'job_details_content', locals: { job: job }) %>
<%= render(partial: 'job_details_content', locals: { job: job, project: @project }) %>
</div>
<%- end -%>
</div>
Expand Down
2 changes: 2 additions & 0 deletions apps/dashboard/config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
resources :projects do
root 'projects#index'
get '/jobs/:cluster/:jobid' => 'projects#job_details', :defaults => { :format => 'turbo_stream' }, :as => 'job_details'
delete '/jobs/:cluster/:jobid' => 'projects#delete_job', :as => 'delete_job'
post '/jobs/:cluster/:jobid/stop' => 'projects#stop_job', :as => 'stop_job'

resources :launchers do
post 'submit', on: :member
Expand Down
Loading