galaxyproject / tools-iuc

Tool Shed repositories maintained by the Intergalactic Utilities Commission
https://galaxyproject.org/iuc
MIT License
160 stars 423 forks source link

Process for handling tools that fail travis tests or cannot be automatically tested #1285

Open gregvonkuster opened 7 years ago

gregvonkuster commented 7 years ago

@galaxyproject/iuc What is the process for handling tools like MultiGPS (https://github.com/galaxyproject/tools-iuc/pull/1227) that pass the standard Galaxy functional tests outside of the travis environment but fail within travis? This particular tool seems to be failing with travis for 2 reasons:

  1. The Galaxy environment running within travis for testing does not include chromosome length files
  2. planemo serve does not seem to be locating the multigps package installed by conda - this may be related to one of the issues discussed in the last 2 comments here: https://github.com/galaxyproject/tools-iuc/pull/1203

In addition, I have several tools that cannot be automatically tested using the current features of the test framework provided by planemo and Galaxy. I'm currently hosting these tools here: https://github.com/gregvonkuster/galaxy_tools/tree/master/tools/plant_tribes. Several of these tools cannot be automatically tested because the current test framework does not cover the following:

  1. Tools that take an input dataset with an associated extra_files_path or produce an output dataset with an associated files_path.
  2. Tools that take as an input very large datasets (e.g. larger than most genome indexes) that are deep hierarchies of directories and files.

There has been some recent discussion around these topics (e.g., create additional IUC related tool repositories in addition to this one to house tools that cannot currently be automatically tested), so I'm hoping the IUC can reach consensus on these issues.

The PI I am working with on the PlantTribes tools is close to having a publication on the PlantTribes pipeline, including the Galaxy environment for it. Our next task in this process is to create training material for this pipeline to be added here https://github.com/galaxyproject/training-material. This means that I will be creating a docker image for these tools and the workflows that will be associated with them.

This is considered phase 1 of this pipeline, with plans to add additional tools in the future. All of these tools are currently available in the TTS owned by greg.

Before I build the docker image and start in on the training material, it would be great if we can decide whether these tools should continue to be owned by greg with this repo https://github.com/gregvonkuster/galaxy_tools/tree/master/tools/plant_tribes being the ongoing storage repo for them or whether they can be owned by iuc and housed here or in another repo long term.

Thanks for your help on this! ;)

mvdbeek commented 7 years ago

I would say there is no hurry in transferring tools to the iuc. We have taken over maintenance of third party tools in the past, and we can continue to do this.

planemo serve does not seem to be locating the multigps package installed by conda - this may be related to one of the issues discussed in the last 2 comments here: #1203

It's hard to pick out the 2 issues you are talking about from this PR. I doubt though that with a functional conda package planemo would error out just on gemini and multigps.

The Galaxy environment running within travis for testing does not include chromosome length files

Did you try what I suggested here: https://github.com/galaxyproject/tools-iuc/pull/1206#issuecomment-289575663 ? It may not work for len files, but it does work for other data that is referenced in .loc files. If we can have feedback on this we can see what to do on the galaxy side about this.

Tools that take an input dataset with an associated extra_files_path

Again, I may be wrong on this, but couldn't this be done within the datatype? If you can upload such a dataset (perhaps as a sniffable archive?), you should be able to test it. Similarly, couldn't the primary dataset reflect a summary statistic of the underlying directory structure ? In that way you can verify that the structure exists and corresponds to what you would expect.

My stance on this is that we should not accept new tools that are not testable currently. If the testing framework can be enhanced to test these tools we can always adopt them later on. I think that there is value in tool authors maintaining their own tools. I don't think that every galaxy tool should be in the IUC.

gregvonkuster commented 7 years ago

I would say there is no hurry in transferring tools to the iuc. We have taken over maintenance of third party tools in the past, and we can continue to do this.

Ok, so this is one vote for keeping the tools owned by greg in their current repo ongoing.

planemo serve does not seem to be locating the multigps package installed by conda - this may be related to one of the issues discussed in the last 2 comments here: #1203

It's hard to pick out the 2 issues you are talking about from this PR. I doubt though that with a functional conda package planemo would error out just on gemini and multigps.

To clarify, planemo test works just fine if referencing a Galaxy instance that includes chromosome length files like this:

$ planemo test --galaxy_root /home/greg/work/git_workspace/galaxy --job_output_files /home/greg/work/tools-iuc/multigps_multi2/tools-iuc/tools/multigps/test-data/output --no_cleanup --conda_dependency_resolution --conda_auto_install --conda_ensure_channels gregvonkuster,iuc,bioconda,r,defaults,conda-forge --conda_prefix /home/greg/miniconda3 --conda_exec /home/greg/miniconda3/bin/conda --conda_debug --test_data /home/greg/work/tools-iuc/multigps_multi2/tools-iuc/tools/multigps/test-data .

I'm not quite sure, but it may be an issue with planemo serve that is not finding the multigps conda package and the gemini conda package. I've not used planemo serve, so I'm not sure. The gemini issue looks to possibly be some cached older version of gemini. I've not seen this issue though using planemo test.

The Galaxy environment running within travis for testing does not include chromosome length files

Did you try what I suggested here: https://github.com/galaxyproject/tools-iuc/pull/1206#issuecomment-289575663 ? It may not work for len files, but it does work for other data that is referenced in .loc files. If we can have feedback on this we can see what to do on the galaxy side about this.

I didn't try this because the framework code that handles the $chromInfo command line value did not look in the .loc files for the path to the file in the past. Perhaps this has changed recently? I'll give this a try, but I'll have to figure out what the entry in the .loc file would look like for the $chromInfo value to be properly handled. Hmm....

Tools that take an input dataset with an associated extra_files_path

Again, I may be wrong on this, but couldn't this be done within the datatype? If you can upload such a dataset (perhaps as a sniffable archive?), you should be able to test it.

These PlantTribes data formats are not sniffable, nor upload-able since they can only be generated by a tool. The extra_files_path directory can easily contain thousands of files of various data formats. The current set of data types is defined here https://github.com/galaxyproject/galaxy/blob/dev/config/datatypes_conf.xml.sample#L604, with a few more coming shortly. I suppose I could enhance the data types to be sniffable, but in order to test, the tool's test-data directory in the repo would have to include a subdirectory of these files in order for the <test> tag set to find them. I don't think the current test framework handles this, but perhaps I'm wrong on this?

Similarly, couldn't the primary dataset reflect a summary statistic of the underlying directory structure ? In that way you can verify that the structure exists and corresponds to what you would expect.

The primary dataset does, in fact, contain information like this. So it could be used for checking in this way. However, as described above, the associated extra_files_path directory would still be needed for automated testing wouldn't it?

My stance on this is that we should not accept new tools that are not testable currently.

Yes, I understand this point, and tend to agree with it. There is a problem with this though, and the MultiGPS tool provides a good example. I had no idea that it would not pass testing until I committed it to the repo and it failed within the travis environment. I was less aware of the travis environment when I committed the tool than I am now. I never would have committed the tool if it had not passed testing in my local environment.

If the testing framework can be enhanced to test these tools we can always adopt them later on.

Ok. What would be the process for this if the tool is owned by a 3rd party?

I think that there is value in tool authors maintaining their own tools.

I totally agree. My feeling is that anyone that contributes a tool to the tools-iuc repo should maintain it.

I don't think that every galaxy tool should be in the IUC.

Ok.

yhoogstrate commented 7 years ago

I share the same view as @mvdbeek

gregvonkuster commented 7 years ago

@mvdbeek and @yhoogstrate I understand your shared perspective which is in line with the current philosophy for this repo. That philosophy has changed a bit over the years, which is both hoped for and expected. The IUC should certainly continue to change things for the better over time.

My understanding is that the following statements partially reflect the current approach for tools in this repo - please let me know if my understanding is incorrect.

  1. Existing tools that don't adhere to current best practices will be corrected over time as resources are available
  2. No new tools will be allowed into this repo unless they adhere to current best practices
  3. No new tools will be allowed into this repo unless they define functional tests which pass using the test framework that is currently available in this repo
  4. A primary goal for this repo is to be able to showcase best practice tools to tool developers outside of the IUC to provide examples of how they should develop their tools (the success of this item depends on the priority of 1)

If the above 4 statements are generally correct, they are certainly reasonable and justified. But we need to make sure that our philosophy is continually reviewed and improved as appropriate.

To a certain extent, I feel that new tool development (and all related components - e.g., data types, workflow needs, etc) should steer the priorities of the Galaxy project as a whole. So if new tools become available that benefit an area of research and the current Galaxy features do not fully support those tools, a priority should be (and is, I believe) placed on developing or enhancing those Galaxy features to support those new tools and their various components. I believe this is (at least a component of) the basis of Galaxy's ongoing development plans and priorities.

It may make sense for the IUC to take a similar approach with regard to tools in this repo, especially when it comes to accepting new tools that follow best practices but may require new features that are currently not available (e.g., cannot be tested using the current test framework / features). In the past, tools like this were just added to the blacklist and taken off when the required features were introduced.

This approach has now changed, which is certainly understandable. When the IUC first got started, one of its primary goals was to get new developers interested in building tools for Galaxy. The outcome has far exceeded the expectations for this original goal, so it is no longer as high of a priority. A general consensus now seems to be that this repo is "too popular" (perhaps not the best "label", but I hope my meaning is clear). I wonder if this problem could be improved by adding a new general rule, something like "In order for a new tool to be accepted into this repo, the contributor must agree to maintain that tool over time as much as possible". I know that this doesn't always work, but if this is one of the formal criteria, tool authors may choose to not attempt to contribute their tools here, which would be fine.

But I'm wondering if it may still make sense to allow new tools into this repo that cannot currently be tested, especially if they demonstrate the need for new Galaxy framework and test features that will be valuable to the project and community.

My tools here https://github.com/gregvonkuster/galaxy_tools/tree/master/tools/plant_tribes provide an initial set of phylogenetic tools that will eventually grow very large. They are associated with these new Galaxy datatypes https://github.com/galaxyproject/galaxy/pull/3999, which I hope are ideal, but without any review by the IUC it is more difficult to be sure. The output of this particular tool https://github.com/gregvonkuster/galaxy_tools/tree/master/tools/plant_tribes/gene_family_phylogeny_builder should be loadable by the recently introduced Phylocanvas viz plugin: https://github.com/galaxyproject/galaxy/pull/3850. I'm hoping that the new pttree Galaxy datatype that it produces will be acceptable as an input format for this new viz tool.

This new PlantTribes tools could also help steer the ongoing development of dataset collections. The tools currently take dataset.extra_files_path as input and produce dataset.files_path as output. They were not written to use dataset collections because:

  1. These directories can contains thousands of files, making the current dataset collection UI features difficult to use
  2. The files in these directories consist of multiple Galaxy datatypes, and currently dataset collections assume 1 datatype

It would be ideal for me to be able to enhance these tools to take advantage of potential upcoming enhancements to the dataset collection features, but again this is not as easy if these PlantTribes tools are not on the IUC's radar.

Thanks everyone, for your time and consideration of these thoughts. ;)

yhoogstrate commented 7 years ago

Existing tools that don't adhere to current best practices will be corrected over time as resources are available

  • Unless there is an additional obvious reason not to keep supporting them and somebody likes to work on this, I guess you're right. No new tools will be allowed into this repo unless they adhere to current best practices No new tools will be allowed into this repo unless they define functional tests which pass using the test framework that is currently available in this repo
  • This is a requirement of the best practices, so equal to above (http://galaxy-iuc-standards.readthedocs.io/en/latest/best_practices/tool_xml.html#tests) A primary goal for this repo is to be able to showcase best practice tools to tool developers outside of the IUC to provide examples of how they should develop their tools (the success of this item depends on the priority of 1)
  • I would argue there is also a second primary goal: to find the limitations in Galaxy and start discussion on these in order to get them hopefully fixed and allow corresponding tools into IUC

To a certain extent, I feel that new tool development (and all related components - e.g., data types, workflow needs, etc) should steer the priorities of the Galaxy project as a whole. So if new tools become available that benefit an area of research and the current Galaxy features do not fully support those tools, a priority should be (and is, I believe) placed on developing or enhancing those Galaxy features to support those new tools and their various components. I believe this is (at least a component of) the basis of Galaxy's ongoing development plans and priorities.

Agreed 200% - but until these work flawlessly I would like to see corresponding PRs open and under 'WIP' before we push them to the toolsheds and get issue reports

It may make sense for the IUC to take a similar approach with regard to tools in this repo, especially when it comes to accepting new tools that follow best practices but may require new features that are currently not available (e.g., cannot be tested using the current test framework / features). In the past, tools like this were just added to the blacklist and taken off when the required features were introduced.

I believe the blacklist was there for a slightly different reason. Before planemo was able to test we were all running tests independently and locally on host systems that were not clean. This resulted in tools pushed to the toolshed that were in fact broken. The current setup ensures this does not happen anymore. During the transition to planemo with bioconda we were finally able to automagically test them, but not everything was ported by then. The blacklist was an interim solution to work on those that were behind and we have almost finished all tools (https://github.com/galaxyproject/tools-iuc/issues/1262) :+1: .

But I'm wondering if it may still make sense to allow new tools into this repo that cannot currently be tested, especially if they demonstrate the need for new Galaxy framework and test features that will be valuable to the project and community.

(This has a large overlap with https://github.com/galaxyproject/tools-iuc/issues/1230). I am maybe a bit 'religous' in software engineering and I see testing as one of the holy grales. What you could do is open a PR in galaxy-iuc/standards to remove the testing requirement and to see the responses but I'm afraid this will not be accepted.

They are associated with these new Galaxy datatypes galaxyproject/galaxy#3999, which I hope are ideal, but without any review by the IUC it is more difficult to be sure.

If tests fail this does not mean you can't make pull requests or can't get reviews... I am happy to see them because that's the way to be aware of the limitations of Galaxy but personally I would like to see them 'on hold' until the framework is compatible.

What the IUC has done in the past with tools that require(d) a lot of work or hacking the Galaxy framework are hackathons / codefests. I think you taking the lead in something like that could be a way forward here?