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

feature(streamline mutant delivery and upload) #3916

Merged
merged 96 commits into from
Dec 18, 2024
Merged

Conversation

eliottBo
Copy link
Contributor

@eliottBo eliottBo commented Nov 5, 2024

Description

This PR is a feature branch that will implement the automation of the mutant file delivery as well as a partial rework of what files we store and upload in the FOHM upload api.

Added

  • MutantFileFormatter to rename correctly the files for the upload

How to prepare for test

  • Ssh to relevant server (depending on type of change)
  • Use stage: us
  • Paxa the environment: paxa
  • Install on stage (example for Hasta):
    bash /home/proj/production/servers/resources/hasta.scilifelab.se/update-tool-stage.sh -e S_cg -t cg -b mutant-file-formatter -a

Requires

Merges of:
Clinical-Genomics/MUTANT#178
https://github.com/Clinical-Genomics/servers/pull/1520
https://github.com/Clinical-Genomics/clinical-genomics-ui/pull/620

How to test

Expected test outcome

  • Check that ...
  • Take a screenshot and attach or copy/paste the output.

Review

  • Tests executed by
  • "Merge and deploy" approved by
    Thanks for filling in who performed the code review and the test!

This version is a

  • MAJOR - when you make incompatible API changes
  • MINOR - when you add functionality in a backwards compatible manner
  • PATCH - when you make backwards compatible bug fixes or documentation/instructions

Implementation Plan

  • Document in ...
  • Deploy this branch on ...
  • Inform to ...

@eliottBo
Copy link
Contributor Author

eliottBo commented Nov 6, 2024

Test in stage:
Files have the correct name after running cg deliver case -c case_internal_id -d fastq --dry-run
Screenshot 2024-11-06 at 13 35 49

@eliottBo eliottBo marked this pull request as ready for review November 6, 2024 13:16
@eliottBo eliottBo requested a review from a team as a code owner November 6, 2024 13:16
@eliottBo eliottBo requested a review from Vince-janv November 6, 2024 13:17
Copy link
Contributor

@islean islean left a comment

Choose a reason for hiding this comment

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

Adding some comments. In general I am positive, but I do think we can make the code a bit clearer since it is taking me quite some time to puzzle together what the actual flow is here. I will try to map the flow out and you can correct me.

  1. So we have SampleFiles being passed to the FileFormattingService. The first layer of formatting from the SampleFileFormatter returns a list of FormattedFiles which have the original path attached, and a formatted path which prepends the sample name?
  2. These formatted files are now concatenated in the SampleFileConcatenationFormatter. This results in the formatted_path attributes becoming exchanged so that all FormattedFiles whose original_path corresponds to a file from the same sample with the same direction have the same concatenated formatted_path.
  3. We now enter the MutantFileFormatter which prepends the region and region code to the file names. In doing so, all the formatted_paths from step 2 are actually set to the original paths of all the formatted files. The formatted paths are exactly the same as the original paths but with the reqion and lab code prepended.
  4. We now filter out unique FormattedFiles. I guess this is because the change of the original_path in step 3 now makes it so that for a sample having 2 forward fastq files, we will have 2 duplicated FormattedFile entries? This indicates to me that we are using the list of FormattedFiles in a not intended way.
  5. We now rename all the FormattedFiles from their original path to the formatted path. Will this work? I guess the original paths all correspond to the concatenated paths now so they have been created in step 2? Is the only goal at this point to rename the already concatenated fastq files so that they contain the lims metadata?

Also - in the _concatenate_fastq_files in the SampleFileConcatenationFormatter we seem to have hard coded that we are removing the raw fastq files which I thought should not happen in this case. Is it intended that we remove the raw fastq files?

@ChrOertlin ChrOertlin self-assigned this Nov 25, 2024
@ChrOertlin ChrOertlin changed the title Mutant file formatter feature(streamline mutant delivery and upload) Nov 25, 2024
@ChrOertlin ChrOertlin marked this pull request as draft November 25, 2024 13:48
Copy link
Contributor

@islean islean left a comment

Choose a reason for hiding this comment

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

Looks good! Some unclarity stemming from Mutant with the multiple formatting steps but that is fine

cg/apps/lims/api.py Outdated Show resolved Hide resolved
@ChrOertlin ChrOertlin marked this pull request as ready for review December 17, 2024 12:37
Copy link
Contributor

@Vince-janv Vince-janv left a comment

Choose a reason for hiding this comment

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

Nice work! Since we've touched the delivery flow, have you tested that non-sars-cov2 deliveries still behave as we expect them?

Copy link
Contributor

@diitaz93 diitaz93 left a comment

Choose a reason for hiding this comment

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

Awesome 🚀 Lets rename the component class if possible

cg/apps/lims/api.py Outdated Show resolved Hide resolved
cg/services/deliver_files/factory.py Show resolved Hide resolved
cg/services/deliver_files/factory.py Outdated Show resolved Hide resolved
cg/services/deliver_files/factory.py Show resolved Hide resolved
cg/services/deliver_files/factory.py Outdated Show resolved Hide resolved
Sebastian review

Co-authored-by: Sebastian Diaz <[email protected]>
Copy link
Contributor

@islean islean left a comment

Choose a reason for hiding this comment

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

Super nice improvement!

cg/apps/lims/api.py Outdated Show resolved Hide resolved
cg/services/deliver_files/factory.py Outdated Show resolved Hide resolved
cg/services/deliver_files/file_fetcher/analysis_service.py Outdated Show resolved Hide resolved
cg/services/deliver_files/file_formatter/files/abstract.py Outdated Show resolved Hide resolved
cg/services/deliver_files/utils.py Outdated Show resolved Hide resolved
@ChrOertlin ChrOertlin merged commit cfa2e0a into master Dec 18, 2024
10 checks passed
@ChrOertlin ChrOertlin deleted the mutant-file-formatter branch December 18, 2024 12:02
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.

5 participants