From e41eeb121e39a22fce1a099a49888e1fa97876a3 Mon Sep 17 00:00:00 2001 From: bistline Date: Wed, 12 Feb 2025 16:42:31 -0500 Subject: [PATCH 01/22] simplifying parse_status logic --- app/models/ingest_job.rb | 19 +++++++------------ test/models/ingest_job_test.rb | 11 ++++------- 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/app/models/ingest_job.rb b/app/models/ingest_job.rb index 49bc9ecfe..546857cfc 100644 --- a/app/models/ingest_job.rb +++ b/app/models/ingest_job.rb @@ -382,10 +382,8 @@ def poll_for_completion(run_at: 1.minute.from_now) if done? && !failed? Rails.logger.info "IngestJob poller: #{pipeline_name} is done!" Rails.logger.info "IngestJob poller: #{pipeline_name} status: #{current_status}" - unless special_action? || (action == :ingest_anndata && study_file.is_viz_anndata?) - study_file.update(parse_status: 'parsed') - study_file.bundled_files.each { |sf| sf.update(parse_status: 'parsed') } - end + study_file.update(parse_status: 'parsed') + study_file.bundled_files.each { |sf| sf.update(parse_status: 'parsed') } study.reload # refresh cached instance of study study_file.reload # refresh cached instance of study_file # check if another process marked file for deletion, can happen if this is an AnnData file @@ -412,6 +410,7 @@ def poll_for_completion(run_at: 1.minute.from_now) end elsif done? && failed? Rails.logger.error "IngestJob poller: #{pipeline_name} has failed." + study_file.update(parse_status: 'parsed') # log errors to application log for inspection log_error_messages log_to_mixpanel # log before queuing file for deletion to preserve properties @@ -708,6 +707,7 @@ def launch_subsample_jobs submission = ApplicationController.batch_api_client.run_job( study_file:, user:, action: :ingest_subsample, params_object: subsample_params ) + study_file.update(parse_status: 'parsing') IngestJob.new( pipeline_name: submission.name, study:, study_file:, user:, action: :ingest_subsample, params_object: subsample_params, reparse:, persist_on_fail: @@ -819,6 +819,7 @@ def launch_anndata_subparse_jobs # reference AnnData uploads don't have extract parameter so exit immediately return if params_object.extract.blank? + study_file.update(parse_status: 'parsing') params_object.extract.each do |extract| case extract when 'cluster' @@ -862,14 +863,8 @@ def launch_anndata_subparse_jobs job.delay.push_remote_and_launch_ingest end - # if this was only a raw counts extraction, update parse status - if params_object.extract == %w[raw_counts] - study_file.update(parse_status: 'parsed') - launch_differential_expression_jobs - else - # unset anndata_summary flag to allow reporting summary later unless this is only a raw counts extraction - study_file.unset_anndata_summary! - end + # unset anndata_summary flag to allow reporting summary later unless this is only a raw counts extraction + study_file.unset_anndata_summary! unless params_object.extract == %w[raw_counts] end end diff --git a/test/models/ingest_job_test.rb b/test/models/ingest_job_test.rb index a7131a851..a25fe8c55 100644 --- a/test/models/ingest_job_test.rb +++ b/test/models/ingest_job_test.rb @@ -731,13 +731,6 @@ class IngestJobTest < ActiveSupport::TestCase pipeline_name = SecureRandom.uuid job = IngestJob.new(pipeline_name:, study:, study_file: cluster_file, user: @user, action: :ingest_subsample) - metadata = { - pipeline: { - actions: [ - { commands: %w[foo bar bing baz] } - ] - } - }.with_indifferent_access error_log = "parse_logs/#{cluster_file.id}/user_log.txt" mock = Minitest::Mock.new @@ -889,6 +882,7 @@ class IngestJobTest < ActiveSupport::TestCase annotation_file:, cluster_file:, cluster_name: 'umap', annotation_name: 'disease', annotation_scope: 'study' ) pipeline_name = SecureRandom.uuid + study_file.update(parse_status: 'parsing') job = IngestJob.new( pipeline_name:, study:, study_file:, user: @user, action: :differential_expression, params_object: ) @@ -925,6 +919,9 @@ class IngestJobTest < ActiveSupport::TestCase ApplicationController.stub :batch_api_client, mock do SingleCellMailer.stub :notify_admin_parse_fail, email_mock do job.poll_for_completion + # ensure that parse_status flag is reset after failure + study_file.reload + assert study_file.parsed? mock.verify email_mock.verify end From 527d961c30ff5bcadfdf2a1afef819ad3c43327a Mon Sep 17 00:00:00 2001 From: bistline Date: Wed, 26 Feb 2025 10:55:52 -0500 Subject: [PATCH 02/22] Renaming test --- test/models/ingest_job_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/models/ingest_job_test.rb b/test/models/ingest_job_test.rb index a25fe8c55..4f66eeb24 100644 --- a/test/models/ingest_job_test.rb +++ b/test/models/ingest_job_test.rb @@ -855,7 +855,7 @@ class IngestJobTest < ActiveSupport::TestCase end end - test 'should ensure email delivery on special action failures' do + test 'should ensure email delivery and parse_status reset on special action failures' do study = FactoryBot.create(:detached_study, name_prefix: 'Special Action Email', user: @user, From 2d681a59ef1f1b0d8128d659fbc4101c6131965a Mon Sep 17 00:00:00 2001 From: bistline Date: Wed, 26 Feb 2025 13:52:35 -0500 Subject: [PATCH 03/22] Removing all explicit Terra status checks --- .../analysis_configurations_controller.rb | 13 ----- app/controllers/api/v1/studies_controller.rb | 2 - .../api/v1/study_files_controller.rb | 2 - .../api/v1/study_shares_controller.rb | 2 - app/controllers/profiles_controller.rb | 24 ++++----- app/controllers/site_controller.rb | 39 ++------------ app/controllers/studies_controller.rb | 2 +- app/views/profiles/show.html.erb | 53 ++++++++----------- app/views/site/_study_download_data.html.erb | 16 ++---- app/views/site/_study_tabs_nav.html.erb | 16 ++---- app/views/site/study.html.erb | 2 +- app/views/studies/show.html.erb | 4 +- 12 files changed, 45 insertions(+), 130 deletions(-) diff --git a/app/controllers/analysis_configurations_controller.rb b/app/controllers/analysis_configurations_controller.rb index a07092985..e37ca2b98 100644 --- a/app/controllers/analysis_configurations_controller.rb +++ b/app/controllers/analysis_configurations_controller.rb @@ -3,7 +3,6 @@ class AnalysisConfigurationsController < ApplicationController :submission_preview, :load_study_for_submission_preview, :update_analysis_parameter] before_action :set_analysis_parameter, only: [:update_analysis_parameter] - before_action :check_firecloud_status, only: [:new, :create] before_action do authenticate_user! authenticate_admin @@ -177,16 +176,4 @@ def analysis_parameter_params :association_method, :association_data_type, :_destroy]) end - - def check_firecloud_status - unless ApplicationController.firecloud_client.services_available?(FireCloudClient::RAWLS_SERVICE) - alert = 'The Methods Repository is temporarily unavailable, so we cannot complete your request. Please try again later.' - respond_to do |format| - format.js {render js: "$('.modal').modal('hide'); alert('#{alert}')" and return} - format.html {redirect_to merge_default_redirect_params(studies_path, scpbr: params[:scpbr]), - alert: alert and return} - format.json {head 503} - end - end - end end diff --git a/app/controllers/api/v1/studies_controller.rb b/app/controllers/api/v1/studies_controller.rb index bb3cdea7e..04bc39ca6 100644 --- a/app/controllers/api/v1/studies_controller.rb +++ b/app/controllers/api/v1/studies_controller.rb @@ -1,8 +1,6 @@ module Api module V1 class StudiesController < ApiBaseController - include Concerns::FireCloudStatus - def firecloud_independent_methods # add file_info is essentially a more extensive 'show' method [:index, :show, :file_info] diff --git a/app/controllers/api/v1/study_files_controller.rb b/app/controllers/api/v1/study_files_controller.rb index 027567b08..3035c91f6 100644 --- a/app/controllers/api/v1/study_files_controller.rb +++ b/app/controllers/api/v1/study_files_controller.rb @@ -1,8 +1,6 @@ module Api module V1 class StudyFilesController < ApiBaseController - include Concerns::FireCloudStatus - before_action :authenticate_api_user! before_action :set_study before_action :check_study_edit_permission diff --git a/app/controllers/api/v1/study_shares_controller.rb b/app/controllers/api/v1/study_shares_controller.rb index f4ea294dc..7d5c5cd00 100644 --- a/app/controllers/api/v1/study_shares_controller.rb +++ b/app/controllers/api/v1/study_shares_controller.rb @@ -1,8 +1,6 @@ module Api module V1 class StudySharesController < ApiBaseController - include Concerns::FireCloudStatus - before_action :authenticate_api_user! before_action :set_study before_action :check_study_permission diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index 31cdaee42..b36666a0a 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -19,22 +19,18 @@ def show @study_shares = StudyShare.where(email: @user.email) @studies = Study.where(user_id: @user.id) @fire_cloud_profile = FireCloudProfile.new - # check if profile services are available - @profiles_available = ApplicationController.firecloud_client.services_available?(FireCloudClient::THURLOE_SERVICE) - if @profiles_available - begin - user_client = FireCloudClient.new(user: current_user, project: FireCloudClient::PORTAL_NAMESPACE) - profile = user_client.get_profile - profile['keyValuePairs'].each do |attribute| - if @fire_cloud_profile.respond_to?("#{attribute['key']}=") - @fire_cloud_profile.send("#{attribute['key']}=", attribute['value']) - end + begin + user_client = FireCloudClient.new(user: current_user, project: FireCloudClient::PORTAL_NAMESPACE) + profile = user_client.get_profile + profile['keyValuePairs'].each do |attribute| + if @fire_cloud_profile.respond_to?("#{attribute['key']}=") + @fire_cloud_profile.send("#{attribute['key']}=", attribute['value']) end - rescue => e - ErrorTracker.report_exception(e, current_user, params) - MetricsService.report_error(e, request, current_user) - logger.info "#{Time.zone.now}: unable to retrieve FireCloud profile for #{current_user.email}: #{e.message}" end + rescue => e + ErrorTracker.report_exception(e, current_user, params) + MetricsService.report_error(e, request, current_user) + logger.info "#{Time.zone.now}: unable to retrieve FireCloud profile for #{current_user.email}: #{e.message}" end end diff --git a/app/controllers/site_controller.rb b/app/controllers/site_controller.rb index 450d92554..e44638c4b 100644 --- a/app/controllers/site_controller.rb +++ b/app/controllers/site_controller.rb @@ -118,12 +118,7 @@ def update_study_settings @cluster_annotations = ClusterVizService.load_cluster_group_annotations(@study, @cluster, current_user) set_selected_annotation end - - # double check on download availability: first, check if administrator has disabled downloads - # then check if FireCloud is available and disable download links if either is true - @allow_downloads = ApplicationController.firecloud_client.services_available?(FireCloudClient::BUCKETS_SERVICE) end - set_firecloud_permissions(@study.detached?) set_study_permissions(@study.detached?) set_study_default_options set_study_download_options @@ -156,7 +151,6 @@ def study # double check on download availability: first, check if administrator has disabled downloads # then check individual statuses to see what to enable/disable # if the study is 'detached', then everything is set to false by default - set_firecloud_permissions(@study.detached?) set_study_permissions(@study.detached?) set_study_default_options set_study_download_options @@ -652,46 +646,19 @@ def set_workspace_samples end end - # check various firecloud statuses/permissions, but only if a study is not 'detached' - def set_firecloud_permissions(study_detached) - @allow_firecloud_access = false - @allow_downloads = false - @allow_edits = false - return if study_detached - begin - @allow_firecloud_access = AdminConfiguration.firecloud_access_enabled? - api_status = ApplicationController.firecloud_client.api_status - # reuse status object because firecloud_client.services_available? each makes a separate status call - # calling Hash#dig will gracefully handle any key lookup errors in case of a larger outage - if api_status.is_a?(Hash) - system_status = api_status['systems'] - sam_ok = system_status.dig(FireCloudClient::SAM_SERVICE, 'ok') == true # do equality check in case 'ok' node isn't present - rawls_ok = system_status.dig(FireCloudClient::RAWLS_SERVICE, 'ok') == true - buckets_ok = system_status.dig(FireCloudClient::BUCKETS_SERVICE, 'ok') == true - @allow_downloads = buckets_ok - @allow_edits = sam_ok && rawls_ok - end - rescue => e - logger.error "Error checking FireCloud API status: #{e.class.name} -- #{e.message}" - ErrorTracker.report_exception(e, current_user, @study, { firecloud_status: api_status}) - MetricsService.report_error(e, request, current_user, @study) - end - end - # set various study permissions based on the results of the above FC permissions def set_study_permissions(study_detached) @user_can_edit = false @user_can_compute = false @user_can_download = false @user_embargoed = false + @allow_firecloud_access = AdminConfiguration.firecloud_access_enabled? return if study_detached || !@allow_firecloud_access begin @user_can_edit = @study.can_edit?(current_user) - if @allow_downloads - @user_can_download = @user_can_edit ? true : @study.can_download?(current_user) - @user_embargoed = @user_can_edit ? false : @study.embargoed?(current_user) - end + @user_can_download = @user_can_edit ? true : @study.can_download?(current_user) + @user_embargoed = @user_can_edit ? false : @study.embargoed?(current_user) rescue => e logger.error "Error setting study permissions: #{e.class.name} -- #{e.message}" ErrorTracker.report_exception(e, current_user, @study) diff --git a/app/controllers/studies_controller.rb b/app/controllers/studies_controller.rb index 89399f7dd..41bdf72b0 100644 --- a/app/controllers/studies_controller.rb +++ b/app/controllers/studies_controller.rb @@ -46,7 +46,7 @@ def show @directories = @study.directory_listings.are_synced @primary_data = @study.directory_listings.primary_data @other_data = @study.directory_listings.non_primary_data - @allow_downloads = ApplicationController.firecloud_client.services_available?(FireCloudClient::BUCKETS_SERVICE) && !@study.detached + @allow_downloads = !@study.detached @analysis_metadata = @study.analysis_metadata.to_a # load study default options set_study_default_options diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index fcbc701a0..56a4d7912 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -7,16 +7,9 @@ - <% if @profiles_available %> - - <% else %> - - <% end %> +
@@ -144,28 +137,26 @@
- <% if @profiles_available %> -
- <% if current_user.registered_for_firecloud %> - <%= render partial: 'user_firecloud_profile' %> - <% else %> -
-
-

Please complete your Terra registration

-

- You may not update your Terra profile until you have registered with Terra and accepted the terms of service. - Please <%= link_to 'visit Terra', 'https://app.terra.bio', target: :_blank, rel: 'noopener noreferrer' %>, - select 'Sign in with Google' from the top-lefthand nav menu, and complete the sign in and registration process. -

-

- <%= link_to "Complete Registration Now ".html_safe, 'https://app.terra.bio', - target: :_blank, rel: 'noopener noreferrer', class: 'btn btn-lg btn-default' %> -

-
+
+ <% if current_user.registered_for_firecloud %> + <%= render partial: 'user_firecloud_profile' %> + <% else %> +
+
+

Please complete your Terra registration

+

+ You may not update your Terra profile until you have registered with Terra and accepted the terms of service. + Please <%= link_to 'visit Terra', 'https://app.terra.bio', target: :_blank, rel: 'noopener noreferrer' %>, + select 'Sign in with Google' from the top-lefthand nav menu, and complete the sign in and registration process. +

+

+ <%= link_to "Complete Registration Now ".html_safe, 'https://app.terra.bio', + target: :_blank, rel: 'noopener noreferrer', class: 'btn btn-lg btn-default' %> +

- <% end %> -
- <% end %> +
+ <% end %> +
diff --git a/app/views/site/_study_download_data.html.erb b/app/views/site/_study_download_data.html.erb index 02bc17346..7d072daf1 100644 --- a/app/views/site/_study_download_data.html.erb +++ b/app/views/site/_study_download_data.html.erb @@ -4,10 +4,9 @@

Study Files <%= @study_files.count %> - <%= link_to " Bulk download".html_safe, '#/', class: "btn btn-default pull-right #{!@allow_downloads ? 'disabled' : nil}", id: 'download-help' %> + <%= link_to " Bulk download".html_safe, '#/', class: "btn btn-default pull-right", id: 'download-help' %>

- <% if @allow_downloads %>