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

Update or remove universc #289

Open
grst opened this issue Jan 18, 2024 · 8 comments
Open

Update or remove universc #289

grst opened this issue Jan 18, 2024 · 8 comments
Labels
enhancement New feature or request quality & consistency improve quality and reduce maintenance burden

Comments

@grst
Copy link
Member

grst commented Jan 18, 2024

Description of feature

Universc is currently broken (it likely was for a while, because tests were running with the -stub option).

Prerequisite to make it work would be to finalize the module update (nf-core/modules#3493).

Personally, I am unsure how much value it adds as kallisto/alevin-fry/starsolo nowadays support quite flexible read structure definitions. I am not using universc and don't have the capacity to work on this - @TomKellyGenetics @adamrtalbot are you still interested in maintaining this functionality?

@grst grst added enhancement New feature or request quality & consistency improve quality and reduce maintenance burden labels Jan 18, 2024
@adamrtalbot
Copy link
Contributor

I agree. Let's get rid of it 👍

@TomKellyGenetics
Copy link
Member

I'm quite busy at the moment but should be able to handle it. It has a very similar CLI to Cell Ranger so if that is already supported, it wouldn't be too much trouble to migrate changes over.

@adamrtalbot
Copy link
Contributor

The CLI might be the same but as I remember it was fundamentally broken. The logs have disappeared on this run where I tried to fix it so I can't see why: https://github.com/nf-core/modules/actions/runs/6402528495/job/17379314958

Either it should be fixed or removed.

@grst
Copy link
Member Author

grst commented Jan 19, 2024

Also the cellranger module has diverged since that: It's using cellranger 7, vs. universc is version 2/3, and the cellranger module uses a python script including file renaming to call cellranger. The synergies are rather limited at this point.

@TomKellyGenetics
Copy link
Member

Renaming won't be necessary for UniverSC, it checks for compatible inputs and renames before calling Cell Ranger already (i.e., the functionality of the Python script is already integrated into it). The input arguments are the same for scRNA-Seq except for the additional "technology" parameter. I had tests for both passing locally and on GitHub Actions on the same test data at the time of merging. For more details, see the testing of the module before creating the subworkflow.

It should be trivial to fix the call on the same test data. I suspect the changes are to the test job or reference index (note the same STAR version is required for indexing and alignment in either case). Given there is an open-source implementation of this, it is unclear to me why efforts have been made to maintain a licensed proprietary version instead.

@adamrtalbot
Copy link
Contributor

Given there is an open-source implementation of this, it is unclear to me why efforts have been made to maintain a licensed proprietary version instead.

The licensing for cellranger is bizarre to say the least. cellranger 2 is abandonware with an accessible license, cellranger 3 onwards is closed source. But at version 7.2.0 they released all code to Github but with a non-open source license. It's pretty clear we can't repackage v7.2.0 so we're stuck with v2.

You may not use, propagate or modify the Software, or any derivatives thereof, except as expressly provided herein. Any attempt otherwise to use, propagate or modify it is void, and will automatically terminate your rights under this license.

Although, that is not a software license I've ever seen before.

I too think we shouldn't be maintaining a cellranger module, but I would go further and remove anything without an open source license 😝 but I know how popular cellranger is.

@grst
Copy link
Member Author

grst commented Feb 14, 2024

I agree the cellranger license is pathetic, but we have the permission to distribute it as part of this pipeline and it's still a popular tool (e.g. I work in pharma and they prefer to use it because then they can blame 10x if something is wrong). And for the multiomics assays, the open source tools haven't caught up yet.

But if you want to use an open source aligner, why not go for starsolo/alevin/kallisto which all support flexible chemistry definitions now and are much faster than cellranger 2? I.e. what exactly is the niche for universc?

I'm happy to keep it if someone makes it work again and adds a nf-test testcase, but I don't have the bandwidth to do it.

@TomKellyGenetics
Copy link
Member

TomKellyGenetics commented Feb 14, 2024

Although, that is not a software license I've ever seen before.

Yeah it’s their own custom license, which their general counsel kindly reminded us of when they became aware of our open-source implementation based off an MIT licensed repository (of version 3 which was then hastily updated).

I'm happy to keep it if someone makes it work again and adds a nf-test testcase

Thanks for taking it into consideration. The main motivation is to provide the same functionality as Cell Ranger without the constraints of the license as both the EULA and hardcoded parameters do not allow other kits. I believe the interface we provide is a convenient way to interact with it.

I’m willing to set up a test job and resubmit it as a PR but I’ve moved onto another job and we have deadlines for the end of the fiscal year (on other projects using different technologies). I’d like to revisit this when I have more time for older projects.

For context, the subworkflow was not heavily tested and was merged quickly as a drop-in replacement for Cell Ranger. However, the nf-core “module” that it calls was extensively tested and discussed in a separate PR. There was some overlap in reviewers which may explain why they assumed the new module (they’d already reviewed) would function as claimed. Unfortunately as a result, I’m unsure if the test job was originally flawed or if changes to the pipeline since have rendered it incompatible. Apologies for the inconvenience but I will attempt to rectify this when I have more time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request quality & consistency improve quality and reduce maintenance burden
Projects
None yet
Development

No branches or pull requests

3 participants