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

Removing skip_push parameter for IngestJob.push_remote_and_launch_ingest (SCP-4576) #1591

Merged
merged 2 commits into from
Aug 15, 2022

Conversation

bistline
Copy link
Contributor

@bistline bistline commented Aug 15, 2022

BACKGROUND & CHANGES

The Ruby 3.1 update exposed a regression with passing parameters to methods when invoking them via delay. This is only used in IngestJob.push_remote_and_launch_ingest where we pass the skip_push parameter, which will skip attempting to push a file to a bucket if it is already there. However, inspection of the code revealed that this would never actually be exercised as we check first to see if the file has in fact been pushed to the bucket. Therefore, this update removes the parameter from the method definition.

It is unclear why delay is not passing parameters properly - we are on the latest release of DelayedJob. It is likely related to the Ruby 3.1 update and the hash/literal shorthand updates. We may need to open a support ticket with DelayedJob if we ever need to use this feature. UPDATE: It appears this is a known issue in DelayedJob and methods with keyword arguments. Thankfully, we do not require them at this time, but if we do ever need to support kwargs via delay, we will need this bug fixed. There appear to be several open PRs aimed at adding better support for Ruby 3.1, so hopefully this will be fixed soon.

This also updates sentry-raven to a more recent release to silence the multipart-post deprecation warnings in the logs.

MANUAL TESTING

  1. Pull branch, start DelayedJob with ./rails_local_setup.rb && source config/secrets/.source_env.bash && bin/rails jobs:work
  2. In a separate terminal window, open a Rails console session
  3. Re-run a DE job with the following:
result = DifferentialExpressionResult.last
study = result.study
cluster_file = result.cluster_group.study_file
annotation_name = result.annotation_name
annotation_scope = result.annotation_scope
result.destroy
DifferentialExpressionService.run_differential_expression_job(cluster_file, study, study.user, annotation_name:, annotation_scope:)
  1. In development.log, validate the job launched by looking for the PAPI parameters:
Remote found for UMAP_crush.txt:6272a06f3ea6aec6b795f2ac, launching Ingest job
Request object sent to Google Pipelines API (PAPI), excluding 'environment' parameters:
---
:actions:
  :commands:
  - python
  - ingest_pipeline.py
  - "--study-id"
  - 62729e0894ec8f9704b9981e
  - "--study-file-id"
  - 6272a06f3ea6aec6b795f2ac
  - "--user-metrics-uuid"
  - f775823b-94f7-46af-b1c7-789582efaa18
  - differential_expression
...

Copy link
Member

@eweitz eweitz left a comment

Choose a reason for hiding this comment

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

Code looks good!

@codecov
Copy link

codecov bot commented Aug 15, 2022

Codecov Report

Merging #1591 (357d0bf) into development (b15a57c) will increase coverage by 0.01%.
The diff coverage is 42.85%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1591      +/-   ##
===============================================
+ Coverage        69.70%   69.72%   +0.01%     
===============================================
  Files              282      282              
  Lines            21717    21716       -1     
  Branches          1491     1491              
===============================================
+ Hits             15138    15141       +3     
+ Misses            6519     6515       -4     
  Partials            60       60              
Impacted Files Coverage Δ
app/lib/file_parse_service.rb 60.00% <0.00%> (ø)
app/models/ingest_job.rb 28.80% <40.00%> (+0.07%) ⬆️
app/lib/differential_expression_service.rb 93.33% <100.00%> (ø)
lib/delayed_job_accessor.rb 89.47% <0.00%> (-5.27%) ⬇️
app/lib/summary_stats_utils.rb 88.18% <0.00%> (-0.91%) ⬇️
app/lib/expression_viz_service.rb 77.29% <0.00%> (+2.70%) ⬆️

@bistline
Copy link
Contributor Author

Seems this is a known issue in DelayedJob that they are working on fixing: collectiveidea/delayed_job#1130

Copy link
Contributor

@ehanna4 ehanna4 left a comment

Choose a reason for hiding this comment

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

Works as advertised!

@bistline bistline merged commit 8610bbb into development Aug 15, 2022
@github-actions github-actions bot deleted the jb-de-service-bugfix branch August 15, 2022 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants