-
Notifications
You must be signed in to change notification settings - Fork 173
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
Comments
I agree. Let's get rid of it 👍 |
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. |
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. |
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. |
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. |
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.
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. |
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. |
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).
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. |
Hi guys, |
Let's use the anticipated major version bump to 3.x to remove it. My reasoning:
I think it's the right decision to remove unused or rarely used functionality from the pipeline to ensure future maintainability. |
@grst Can you clarify the issue specifically? At the time of merging unit tests were working as expected. No changes to the workflow have been made since to my knowledge. The '--stub' option was only invoked to avoid timeout as the test jobs had a long runtime. The universc module has tested carefully and still works as far as I can tell. Note that @apeltzer reviewed both the module and subworkflow so was aware of prior testing. I suspect it is a matter of changes to the input data for test jobs as the code is containerised. Has there been changes to how the test data is stored or the output directory for test jobs? For example this pull request would break UniverSC if the dependency were ignored. In this case it is a possibly remediated with simple changes to the test parameters. As discussed above and in our manuscript in more detail, there are specific reasons for using this tool instead of Cell Ranger. I disagree that this merits removing the entire workflow. However, I will respect this decision if there is a consensus that it is truly too difficult to maintain. I find this difficult to believe since the test data is the same as older versions of Cell Ranger unless newer versions are no longer compatible with it. Apologies, due to family commitments and new role, I've not had time to investigate the root cause of this myself. |
while the subworkflow from nf-core modules may be tested, there is was no pipeline-level test for universc implemented. It's been a while since I tried, but last time I was not able to execute the universc branch of the pipeline. It may or may not have been easy to fix, but it has never been a priority to any of the maintainers and we unfortunately can dedicate very limited resources to this pipeline ourselves. The point is that every line of code is a maintenance burden, makes refactoring slower etc. Sometimes it's just better to get rid of rarely used features to free resources. Finally, while I can totally see why one would not want to use CellRanger, I still haven't heard a convincing argument to use universc over other open source tools such as starsolo, kallisto or alevin. |
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?
The text was updated successfully, but these errors were encountered: