From 357d0bf7e6b6d54841810686e0ee404cc497b6a4 Mon Sep 17 00:00:00 2001 From: bistline Date: Mon, 15 Aug 2022 12:58:33 -0400 Subject: [PATCH] removing skip_push parameter --- app/lib/differential_expression_service.rb | 4 ++-- app/lib/file_parse_service.rb | 2 +- app/models/ingest_job.rb | 17 +++++------------ .../lib/differential_expression_service_test.rb | 12 ++++++------ 4 files changed, 14 insertions(+), 21 deletions(-) diff --git a/app/lib/differential_expression_service.rb b/app/lib/differential_expression_service.rb index 9b95e48a1f..d36bde04f7 100644 --- a/app/lib/differential_expression_service.rb +++ b/app/lib/differential_expression_service.rb @@ -178,7 +178,7 @@ def self.run_differential_expression_job(cluster_file, study, user, annotation_n # launch DE job job = IngestJob.new(study: study, study_file: cluster_file, user: user, action: :differential_expression, params_object: params_object) - job.delay.push_remote_and_launch_ingest(skip_push: true) # skip push as file is already in bucket + job.delay.push_remote_and_launch_ingest true else raise ArgumentError, "job parameters failed to validate: #{params_object.errors.full_messages}" @@ -198,7 +198,7 @@ def self.run_differential_expression_job(cluster_file, study, user, annotation_n def self.validate_annotation(cluster_file, study, annotation_name, annotation_scope) cluster = study.cluster_groups.by_name(cluster_file.name) raise ArgumentError, "cannot find cluster for #{cluster_file.name}" if cluster.nil? - + result = DifferentialExpressionResult.find_by(study: study, cluster_group: cluster, annotation_name: annotation_name, diff --git a/app/lib/file_parse_service.rb b/app/lib/file_parse_service.rb index 9af510ec08..4351b578ef 100644 --- a/app/lib/file_parse_service.rb +++ b/app/lib/file_parse_service.rb @@ -71,7 +71,7 @@ def self.run_parse_job(study_file, study, user, reparse: false, persist_on_fail: bundle.study_files.update_all(parse_status: 'parsing') job = IngestJob.new(study: study, study_file: matrix, user: user, action: :ingest_expression, reparse: reparse, persist_on_fail: persist_on_fail) - job.delay.push_remote_and_launch_ingest(skip_push: true) + job.delay.push_remote_and_launch_ingest else return self.missing_bundled_file(study_file) end diff --git a/app/models/ingest_job.rb b/app/models/ingest_job.rb index 94750f755b..8afd70c46d 100644 --- a/app/models/ingest_job.rb +++ b/app/models/ingest_job.rb @@ -49,15 +49,13 @@ class IngestJob # Push a file to a workspace bucket in the background and then launch an ingest run and queue polling # Can also clear out existing data if necessary (in case of a re-parse) # - # * *params* - # - +skip_push+ (Boolean) => skip call to study.send_to_firecloud(study_file) (may be in process in different thread) # * *yields* # - (Google::Apis::GenomicsV2alpha1::Operation) => Will submit an ingest job in PAPI # - (IngestJob.new(attributes).poll_for_completion) => Will queue a Delayed::Job to poll for completion # # * *raises* # - (RuntimeError) => If file cannot be pushed to remote bucket - def push_remote_and_launch_ingest(skip_push: false) + def push_remote_and_launch_ingest begin file_identifier = "#{study_file.bucket_location}:#{study_file.id}" rails_model = MODELS_BY_ACTION[action] @@ -70,7 +68,7 @@ def push_remote_and_launch_ingest(skip_push: false) # first check if file is already in bucket (in case user is syncing) remote = ApplicationController.firecloud_client.get_workspace_file(study.bucket_id, study_file.bucket_location) if remote.nil? - is_pushed = poll_for_remote(skip_push: skip_push) + is_pushed = poll_for_remote else is_pushed = true # file is already in bucket end @@ -110,20 +108,15 @@ def push_remote_and_launch_ingest(skip_push: false) # helper method to push & poll for remote file # - # * *params* - # - +skip_push+ (Boolean) => skip call to study.send_to_firecloud(study_file) (may be in process in different thread) - # # * *returns* # - (Boolean) => Indication of whether or not file has reached bucket - def poll_for_remote(skip_push: false) + def poll_for_remote attempts = 1 is_pushed = false file_identifier = "#{study_file.bucket_location}:#{study_file.id}" while !is_pushed && attempts <= MAX_ATTEMPTS - unless skip_push - Rails.logger.info "Preparing to push #{file_identifier} to #{study.bucket_id}" - study.send_to_firecloud(study_file) - end + Rails.logger.info "Preparing to push #{file_identifier} to #{study.bucket_id}" + study.send_to_firecloud(study_file) Rails.logger.info "Polling for upload of #{file_identifier}, attempt #{attempts}" remote = ApplicationController.firecloud_client.get_workspace_file(study.bucket_id, study_file.bucket_location) if remote.present? diff --git a/test/integration/lib/differential_expression_service_test.rb b/test/integration/lib/differential_expression_service_test.rb index d9565c5c83..c1f8f2df61 100644 --- a/test/integration/lib/differential_expression_service_test.rb +++ b/test/integration/lib/differential_expression_service_test.rb @@ -77,7 +77,7 @@ class DifferentialExpressionServiceTest < ActiveSupport::TestCase # we need to mock 2 levels deep as :delay should yield the :push_remote_and_launch_ingest mock job_mock = Minitest::Mock.new - job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new, [Hash]) + job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new) mock = Minitest::Mock.new mock.expect(:delay, job_mock) IngestJob.stub :new, mock do @@ -155,7 +155,7 @@ class DifferentialExpressionServiceTest < ActiveSupport::TestCase DataArray.create(data_array_params) job_mock = Minitest::Mock.new - job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new, [Hash]) + job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new) mock = Minitest::Mock.new mock.expect(:delay, job_mock) IngestJob.stub :new, mock do @@ -178,7 +178,7 @@ class DifferentialExpressionServiceTest < ActiveSupport::TestCase @basic_study.update(default_options: { cluster: 'cluster_diffexp.txt', annotation: 'species--group--study' }) DataArray.create!(@all_cells_array_params) job_mock = Minitest::Mock.new - job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new, [Hash]) + job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new) mock = Minitest::Mock.new mock.expect(:delay, job_mock) IngestJob.stub :new, mock do @@ -205,9 +205,9 @@ class DifferentialExpressionServiceTest < ActiveSupport::TestCase test 'should run differential expression job on all annotations' do DataArray.create!(@all_cells_array_params) job_mock = Minitest::Mock.new - job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new, [Hash]) - job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new, [Hash]) - job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new, [Hash]) + job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new) + job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new) + job_mock.expect(:push_remote_and_launch_ingest, Delayed::Job.new) mock = Minitest::Mock.new mock.expect(:delay, job_mock) mock.expect(:delay, job_mock)