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

feat: add cubi-tk sodar update-samplesheet #240

Merged
merged 12 commits into from
Jan 14, 2025

Conversation

Nicolai-vKuegelgen
Copy link
Contributor

New function to allow flexible updates of ISA samplesheets. Also introduce a base class to implement SodarAPI features, as the currently used sodar_cli package is heavily outdated.
Features:

  • can read ped files and/or command line defined sample specific (meta)data
  • mapping of all ped/sampledata fields to ISA columns can be speciefied. Dynamic/derived columns are also possible
  • default settings for the ISA germline-DNA-sequencing sheet or the new Modellvorhaben project
  • global metadata can be defined for all samples at once
  • both adding new entries and updating entries is possible (--overwrite required for updating non-empty values)
  • Values are mapped to the ISA table based on column name matching, parsing/validation of the ISA graph structure is NOT done by this function (but errors will be returned by the Sodar API anyway)

TBD:

  • tsv or similar input for collected sampledata in another file format than ped

Note: this is a potential replacement/side-grade of cubi-tk sodar add-ped

Add sodar/update-samplesheet and tests
- update-samplesheet now uses new MV default (old sheet layout with Analysis-ID as sample-ID, other IDs as metadata)
- several fixes
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Please format your code with black: make black.

@Nicolai-vKuegelgen
Copy link
Contributor Author

Missing so far:

  • tests for the new SodarAPI class
  • smoke test for the while update-samplesheet workflow (execute)

@tedil tedil changed the title fead: add cubi-tk sodar update-samplesheet feat: add cubi-tk sodar update-samplesheet Jan 10, 2025
Copy link
Member

@tedil tedil left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with ISA, so most of my comments are more from a coding perspective and some general remarks.
I really like all the sanity checks and validations that you do here.

cubi_tk/sodar/update_samplehseet.py Outdated Show resolved Hide resolved
cubi_tk/sodar/update_samplehseet.py Outdated Show resolved Hide resolved
cubi_tk/sodar/update_samplehseet.py Outdated Show resolved Hide resolved
cubi_tk/sodar/update_samplehseet.py Outdated Show resolved Hide resolved
cubi_tk/sodar/update_samplehseet.py Outdated Show resolved Hide resolved
cubi_tk/sodar/update_samplehseet.py Outdated Show resolved Hide resolved
cubi_tk/sodar/update_samplehseet.py Outdated Show resolved Hide resolved
cubi_tk/sodar/update_samplehseet.py Outdated Show resolved Hide resolved
cubi_tk/sodar_api.py Outdated Show resolved Hide resolved
cubi_tk/sodar_api.py Outdated Show resolved Hide resolved
@Nicolai-vKuegelgen Nicolai-vKuegelgen marked this pull request as ready for review January 14, 2025 10:37
Copy link
Member

@tedil tedil left a comment

Choose a reason for hiding this comment

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

LGTM, the only thing I'd like to see as a little bonus is an example in the documentation/manual or so.

Comment on lines +198 to +201
"Dummy-ID=Source Name",
"Sample Name",
"Extract Name",
"barcode=Barcode sequence",
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this might behave unexpectedly depending on shell/quoting?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, nvm, arg_list is forwarded as is to argparse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sry, didn't fully read your comment, I can add something to the documentation though

@Nicolai-vKuegelgen Nicolai-vKuegelgen merged commit 581e650 into main Jan 14, 2025
4 checks passed
@Nicolai-vKuegelgen Nicolai-vKuegelgen deleted the sodar-update-samplesheet branch January 14, 2025 10:45
This was referenced Jan 14, 2025
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.

2 participants