ugeneunipro / ugene

UGENE is free open-source cross-platform bioinformatics software
http://ugene.net
GNU General Public License v2.0
207 stars 60 forks source link

UGENE-7747 Update samtools source code #1619

Closed DmitriiSukhomlinov closed 3 months ago

DmitriiSukhomlinov commented 4 months ago

Для удобства просмотра PR разбит на два коммита - исходный код samtools и изменения, которые потребовалось сделать непосредственно в UGENE. Советую смотреть второй коммит отдельно, т.к., из-за объема исходников samtools, веб-интерфейс гитхаба подвисает, когда они открыты в режиме просмотра.

Это не весь исходный код samtools, а только минимум, который был необходим, чтобы требуемые в UGENE функции работали. В нее входит библиотека htslib (почти полностью, около 90 процентов), сам samtools (процентов 30-40 от всей библиотеки) и ряд POSIX-библиотек, отсутствующих на Windows (pcre, pthread, time, getopt, unistd, strings, опять же, большая часть из них содержит только необходимый в нашем случае минимум и собираются они все только на Windows). Самая неудобная библиотека - это pthread. Она нужна для многопоточности, но мы не работаем с многопоточностью на уровне встроенных в код инструментов, только на уровне самого UGENE, поэтому, многопоточность в samtools не используется (просто передается единица там, где можно настроить количество потоков). Однако, она очень глубоко вшита в samtools и вырезать ее без риска повредить логику работы самого инструмента у меня не получилось, поэтому её все равно придется оставить.

Все места, которые мне показались острыми, я пометил комментариями с пояснениями. Каждый удаленный или потребовавший изменения тест так же содержит комментарий "почему это потребовалось". Добавлен ряд юнит тестов на класс статических функций, содержащий основные изменения, и один гуи тест.

DmitriiSukhomlinov commented 4 months ago

@yalgaer на всякий случай дополнительно напишу, что ревью можно смотреть

DmitriiSukhomlinov commented 3 months ago

Начала тестировать. Файл _common_data/bam/scerevisiae.bam3.sam в мастере открывается в твоей ветке нет. Возможно регрессия.

Я полагаю, что ты не обновила тестовый репозиторий. Это стоит сделать, т.к. мне пришлось обновить некоторые файлы, т.к. они отказывались обрабатываться новой версией samtools из-за того, что не соответствовали спецификации формата.

Конкретно - SAM/BAM файлы делятся на две части - заголовок и риды. Если ты откроешь _common_data/bam/scerevisiae.bam3.sam в текстовом редакторе, то ты это легко увидишь (зеленый - заголовок, красный - риды): image

Проблема возникала в заголовке. Каждая строка заголовка, содержит в себе несколько тегов типа ключ:значение, а сами тэги разделяются табами. В третьей строке заголовка есть команда запуска, в которой аргументы, в старой версии этого файла, тоже были разделены табами, хотя в значениях тэга табы использовать нельзя, только пробелы. Это четко описано в спецификации формата. В пункте 1.3 The header section в самом первом абзаце прописаны два регулярных выражения и каждая из строк заголовка должна соответствовать одному из них. В нашем случае нас интересует первое регулярное выражение:

/^@(HD|SQ|RG|PG)(\t[A-Za-z][A-Za-z0-9]:[ -~]+)+$/

В старой версии файла строка ему не соотвествовала. Этому можно убедиться, например, воспользовавшись сторонним сервисом проверки регулярных выражений.

Старая версия samtools проглатывала этот косяк в файле, однако, у новой исполнение прерывается с соответствующей ошибкой "Malformed key:value pair" вот в этом месте. Я считаю, что будет правильным согласится с новой версией samtools, т.к. это правильный формат, не противоречащий спецификации, а исправления в значимой части кода могут привести к непредсказуемым последствиям.

Единственное - я когда заменял табы на пробелы в этих местах сделал это только для файлов, используемых в тестировании. Если в тестовом репозитории есть файлы, не участвующие в тестировании, но содержащие подобную проблему, то они могут быть не поправлены.

yalgaer commented 3 months ago

Есть другая большая проблема с патчем: чтобы поддержать регулярные выражения на windows ты включил в наш исходный код большую бибилиотеку, которая превышает размер всех samtools и содержит полноценный JIT компилятор (сравни: старый samtools ~10k строк, новый ~200k - различие в 20 раз!).

Это перебор, так как QT уже поддерживает perl-compatible regular expressions да и сам samtools использует их очень ограниченно в выделенном месте.

Нужно убрать эту гигансткую зависимость из нашего будущего и попробовать использовать регулярные выражения из QT.

Наследние подобных гигантских зависимостей крайне нежелательно для UGENE: их придется поддерживать и решать их проблемы. Сам же samtools не имеет этой зависимости - это наша врезка в его код.

Возможно в рамках нашего использования samtools нам нужно очень ограниченное использование reg-exp: на уровне одной или двух функций работы со строками.

DmitriiSukhomlinov commented 3 months ago

Есть другая большая проблема с патчем: чтобы поддержать регулярные выражения на windows ты включил в наш исходный код большую бибилиотеку, которая превышает размер всех samtools и содержит полноценный JIT компилятор (сравни: старый samtools ~10k строк, новый ~200k - различие в 20 раз!).

Это перебор, так как QT уже поддерживает perl-compatible regular expressions да и сам samtools использует их очень ограниченно в выделенном месте.

Нужно убрать эту гигансткую зависимость из нашего будущего и попробовать использовать регулярные выражения из QT.

Наследние подобных гигантских зависимостей крайне нежелательно для UGENE: их придется поддерживать и решать их проблемы. Сам же samtools не имеет этой зависимости - это наша врезка в его код.

Возможно в рамках нашего использования samtools нам нужно очень ограниченное использование reg-exp: на уровне одной или двух функций работы со строками.

Я не уверен, что сделать samtools, который написан на C, зависимым от Qt, который написан на C++, в принципе возможно, но это, в целом, не так уж и важно в нашем случае. Я поисследовал код и пришел к выводу, что регулярные выражения в samtools используются только для проверки того, что те или иных данные входного рида соответствуют определенному регулярному выражению. Мы не используем этот функционал и, однозначно, не планируем, поэтому я аккуратно вырезал регулярные выражения и все с ними связанное.

yalgaer commented 3 months ago

Хорошо, патч в итоге сократился в 2 раза. Еще пару вопросов/замечаний:

1) Предлагаю не смешивать код htslib и samtools. htslib может лежать в подпапке и повторять оригинальный набор файлов - будет сразу видно что и откуда. Код для этого менять не придется - достаточно расширить INCDIR

2) Код все еще в 10 раз больше того, что был. Можно ли сохранив оригинальную структуру проектов samtools убрать все ненужные файлы/зависимости?

DmitriiSukhomlinov commented 3 months ago

Хорошо, патч в итоге сократился в 2 раза. Еще пару вопросов/замечаний:

  1. Предлагаю не смешивать код htslib и samtools. htslib может лежать в подпапке и повторять оригинальный набор файлов - будет сразу видно что и откуда. Код для этого менять не придется - достаточно расширить INCDIR

ОК

  1. Код все еще в 10 раз больше того, что был. Можно ли сохранив оригинальную структуру проектов samtools убрать все ненужные файлы/зависимости?

Можно попробовать выкинуть pthread, но это потребует существенных изменений в исходном коде samtools, Я не даю гарантий, что не добавлю скрытых багов, делая это.

DmitriiSukhomlinov commented 3 months ago

Тесты на винде попадали из-за того, что там вылез диалог "Указать время для рестарта для обновления"

yalgaer commented 3 months ago

Давай перезапустим винду и как будет зеленая я дам апрув

DmitriiSukhomlinov commented 3 months ago

@yalgaer все тесты пройдены