Commendable work on these important tools!

I'll respond to your discussion points in order:

1. I think we should keep the format conversion tools (sam <-> bam <-> cram) for usability/dicoverability reasons. Slice could be eliminated.

2. New version scheme seems good to me, assuming that we detect 1.7+g is > 2.x

3. I don't know about cram indexes, but I think we can assume in galaxy that all files of type "bam" are indexed. I say no indexing for sam files

4. Would be nice to add the other galaxy bam input types(that are not indexed) for tools like view that don't need an index:

   • qname_input_sorted.bam
   • qname_sorted.bam
   • unsorted.bam

5. I think the tool should ask about output type (.e.g. bam, sam, cram, indexed bam(implies coord sorting), indexed cram) and remove the "same" type - which, IMHO, adds complexity without much benefit. It would be tough to render a workflow from a tool using a same type. (left an inline comment to this effect)

6. Read tags support looks fine to me as a repeat

7. I think it's reasonable to require an fa even if the tool only really needs the index. fasta.gz should be supported

8. Sorry - I don't know about the change_format bits - maybe someone else will

I think we should keep the format conversion tools (sam <-> bam <-> cram) for usability/dicoverability reasons. Slice could be eliminated.

Agreed. What would be the IUC way to remove slice? Delete it? Or mark it deprecated..? How?

New version scheme seems good to me, assuming that we detect 1.7+g is > 2.x

Jep. Can someone answer if this works? Otherwise I would suggest to increase the version slowly, such that samtools version will catch up eventually.

I don't know about cram indexes, but I think we can assume in galaxy that all files of type "bam" are indexed. I say no indexing for sam files

OK.

Would be nice to add the other galaxy bam input types(that are not indexed) for tools like view that don't need an index:
    qname_input_sorted.bam
    qname_sorted.bam
    unsorted.bam

OK. Do we need to add them explicitely? Or are they implicitely allowed anyway, because of the data types class structure?

I think the tool should ask about output type (.e.g. bam, sam, cram, indexed bam(implies coord sorting), indexed cram) and remove the "same" type - which, IMHO, adds complexity without much benefit. It would be tough to render a workflow from a tool using a same type. (left an inline comment to this effect)

OK

Read tags support looks fine to me as a repeat

OK

I think it's reasonable to require an fa even if the tool only really needs the index. fasta.gz should be supported

I will check

Sorry - I don't know about the change_format bits - maybe someone else will

New version scheme seems good to me, assuming that we detect 1.7+g is > 2.x

Jep. Can someone answer if this works? Otherwise I would suggest to increase the version slowly, such that samtools version will catch up eventually.

This doesn't apply here, since this is a new tool. Any way 2.0 > 1.7+g, which can be verified with:

>>> from packaging.version import parse
>>> parse('2.0') > parse('1.7+g')
True

Somehow I have a problem with planemo on my local install (only this branch):

All tests fail immediately, I just get:

Traceback (most recent call last):
  File "/tmp/tmpH13feS/galaxy-dev/test/functional/test_toolbox.py", line 88, in test_tool
    self.do_it(tool_version=tool_version, test_index=test_index)
  File "/tmp/tmpH13feS/galaxy-dev/test/functional/test_toolbox.py", line 41, in do_it
    verify_tool(tool_id, self.galaxy_interactor, resource_parameters=resource_parameters, test_index=test_index, tool_version=tool_version, register_job_data=register_job_data)
  File "/tmp/tmpH13feS/galaxy-dev/lib/galaxy/tools/verify/interactor.py", line 653, in verify_tool
    stage_data_in_history(galaxy_interactor, tool_id, testdef.test_data(), test_history)
  File "/tmp/tmpH13feS/galaxy-dev/lib/galaxy/tools/verify/interactor.py", line 59, in stage_data_in_history
    upload_wait = galaxy_interactor.stage_data_async(test_data, history, tool_id)
  File "/tmp/tmpH13feS/galaxy-dev/lib/galaxy/tools/verify/interactor.py", line 267, in stage_data_async
    file_name = self.test_data_path(tool_id, fname)
  File "/tmp/tmpH13feS/galaxy-dev/lib/galaxy/tools/verify/interactor.py", line 232, in test_data_path
    assert response.status_code == 200
AssertionError

I also tried to reset my venv and planemo configuration directories. I'm completely stuck on this one. Any ideas?

got planemo running again (problem was that I linked to the test data of the converters). my todos are done. the remaining points are open questions.

There is a merge conflict here @bernt-matthias.

I think I would not include the different BAM datatypes here. My intuition is we should keep the tools simple in this regard and offer one sort tool that can create the other datatypes if needed.

Also please note, that Galaxy ships by default with a few converters, so BAM to CRAM and so on should be already in Galaxy.

@bgruening : I value simplicity too, but in this case I wonder whether the cost of duplicating the potentially very large data files for a format conversion is worth a bit more code complexity.

I don't think having a list of 4 output types (sam, bam, cram, count) is much more complex for users than "bam (coordinate sorted), cram (coordinate sorted), sam, unsorted.bam, qname_input_sorted.bam, qname_sorted.bam, unsorted.bam,count"

Or maybe you were referring to the input formats? I think there is even less user complexity if that's your concern.

@bwlang has a point with the data duplication, but users can decide to delete intermediate data (in workflows you can do this automatically).

agreed - also, these other bam formats are proably not typically used much...

@bernt-matthias do you mind if I rebase this against the new macros.xml file and address my last comment ?

I merged the macros.xml file using the github frontend and commited an update using nicer names for the new macros.

thanks, looks good. what do you think about #1963 (comment) ? I'm a little worried about users generating CRAM files and then being disappointed that they don't work when downloading them from Galaxy.

I'm a little worried about users generating CRAM files and then being disappointed that they don't work when downloading them from Galaxy.

You have a point with this. But this might be treated by $reffa!=None which adds --output-fmt-option no_ref (note this is the default). One could state in the help (which needs some love anyway) that in case the user wants to use CRAM outside of Galaxy without the reference available then no refernce must be specified in the tool. But I think that programs using CRAM as input should allow to specify the path to the reference (and the md5 check verifies that its the correct reference).

It seems that only the md5 is stored in CRAM.

https://www.htslib.org/workflow/#mapping_to_cram :

In CRAM format the reference sequence is linked to by the md5sum (M5 auxiliary tag) in the CRAM header (@SQ tags). This is mandatory and part of the CRAM specification.

@mvdbeek took some time to provide a somewhat useful help.

We still need a .shed.yml for samtools view.

@bernt-matthias could you add the .shed.yml file ?

Done