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

Picrust2 #3904

Merged
merged 42 commits into from
Mar 4, 2023
Merged

Picrust2 #3904

merged 42 commits into from
Mar 4, 2023

Conversation

EngyNasr
Copy link
Contributor

The 6 wrappers of Picrust2 https://github.com/picrust/picrust2/wiki are all added:

  • 4 Main (metagenome_pipeline, pathway_pipeline, hsp, place_seqs)
  • 2 Others (add_description, shuffle_predictions)

Issues:

  • 2 Main:
  1. in place_seqs, the --placement_tool when epa-ng is selected, I got errors even in the tools command line (May be because of the test-data available is not suitable)
  2. in pathway_pipeline, the --no_regroup option when set to true, I got errors even in the tools command line (May be because of the test-data available is not suitable)

(Temp solutions removing this options for now, create an issue on picrust2 github)

  • 1 not important:
    shuffle prediction, the tool itself doesn't output the files in the output directory specified in the --outdir argument, so nothing to present on usegalaxy

Extras:

  • Add more tests (I have tested all options manually, so I will just add them in the wrappers tests)

Thanks a lot,
Engy

@bernt-matthias
Copy link
Contributor

The .shed.yml file seems missing

@bernt-matthias
Copy link
Contributor

create an issue on picrust2 github

sounds like a good idea. maybe the developers have nice and small test data that you can use.

@bernt-matthias
Copy link
Contributor

Cool. Only one error: For picrust2_place_seqs you will need to add zip as requirement.

Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good. Added a few comments.

tools/picrust2/place_seqs.xml Outdated Show resolved Hide resolved
tools/picrust2/add_descriptions.xml Outdated Show resolved Hide resolved
tools/picrust2/hsp.xml Outdated Show resolved Hide resolved
tools/picrust2/shuffle_predictions.xml Outdated Show resolved Hide resolved
tools/picrust2/shuffle_predictions.xml Outdated Show resolved Hide resolved
tools/picrust2/add_descriptions.xml Outdated Show resolved Hide resolved
@bgruening
Copy link
Member

@EngyNasr what is the status here? Do you need help?

@EngyNasr
Copy link
Contributor Author

@bgruening, Back to picrust2 🎉, I have created an issue to picrust2 main github page for the issues above, however we can have the tool as it is (after removing the 2 above arguments that are causing issues), then update the tool later based on their response (add them back)

@EngyNasr
Copy link
Contributor Author

EngyNasr commented Nov 5, 2021

Sooo :)

The first two issues where totally solved, where the only problem where the data sets.

The third regarding the output of shuffled prediction, this is a tool problem, however thanks to Pietro he taught me how to play around to reach the temp path of the input file where the output is also saved and now I can present the output on Galaxy

Now all issues are solved, just testing last time and will commit all changes 🎉 🎉 🎉

tools/picrust2/hsp.xml Outdated Show resolved Hide resolved
tools/picrust2/hsp.xml Outdated Show resolved Hide resolved
tools/picrust2/hsp.xml Outdated Show resolved Hide resolved
@EngyNasr
Copy link
Contributor Author

EngyNasr commented Nov 7, 2021

I have changed as per comments but still have a problem with the test data size will solve that and commit changes

@EngyNasr
Copy link
Contributor Author

Step 3 ;) ?

Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @EngyNasr . Few more comments from me

tools/picrust2/add_descriptions.xml Outdated Show resolved Hide resolved
tools/picrust2/add_descriptions.xml Outdated Show resolved Hide resolved
tools/picrust2/add_descriptions.xml Outdated Show resolved Hide resolved
tools/picrust2/hsp.xml Outdated Show resolved Hide resolved
tools/picrust2/hsp.xml Outdated Show resolved Hide resolved
tools/picrust2/metagenome_pipeline.xml Outdated Show resolved Hide resolved
tools/picrust2/metagenome_pipeline.xml Outdated Show resolved Hide resolved
tools/picrust2/pathway_pipeline.xml Outdated Show resolved Hide resolved
tools/picrust2/pathway_pipeline.xml Outdated Show resolved Hide resolved
tools/picrust2/shuffle_predictions.xml Outdated Show resolved Hide resolved
@bgruening
Copy link
Member

Your last commit is cheating ;) You do not test the output and it's actually a valid error.

/tmp/tmpuv7474tj/job_working_directory/000/6/tool_script.sh: line 22: zip: command not found

You are missing zip or gunzip, probably both as a dependency.

Copy link
Member

@bebatut bebatut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @EngyNasr for this work!

In addition to my almost 100 (😱) inline comments, 2 general comments/questions:

  • I did not see any wrapper for picrust2_pipeline.py. Do you plan to wrap it?
  • I have mixed feeling about the intermediate files. I am not sure if it is really useful to output them as zip file. Either we expose them individually (specially if they are always the same), or we hide them. For the first version of the wrapper, I would hide them (so comment everything related to that).

tools/picrust2/macros.xml Outdated Show resolved Hide resolved
tools/picrust2/.shed.yml Outdated Show resolved Hide resolved
tools/picrust2/.shed.yml Outdated Show resolved Hide resolved
tools/picrust2/.shed.yml Outdated Show resolved Hide resolved
tools/picrust2/.shed.yml Outdated Show resolved Hide resolved
tools/picrust2/place_seqs.xml Outdated Show resolved Hide resolved
tools/picrust2/shuffle_predictions.xml Outdated Show resolved Hide resolved
tools/picrust2/place_seqs.xml Outdated Show resolved Hide resolved
tools/picrust2/shuffle_predictions.xml Outdated Show resolved Hide resolved
tools/picrust2/shuffle_predictions.xml Outdated Show resolved Hide resolved
@EngyNasr
Copy link
Contributor Author

the full pipeline is all arguments together in one wrapper, is it really important to have ?

@bgruening
Copy link
Member

I don't think so, we have workflows for that. At least in the first version we can skip this imho.

@bebatut
Copy link
Member

bebatut commented Nov 29, 2021

the full pipeline is all arguments together in one wrapper, is it really important to have ?

I expected that people knowing Picrust may use this tool first (as they may not know the different steps). So I would say yes

I don't think so, we have workflows for that. At least in the first version we can skip this imho.

@bgruening it is the script that Picrust developers put first (https://github.com/picrust/picrust2/wiki/Full-pipeline-script). So I think it may be the most important tool to have there. The other ones are for running the steps one by one.
But I may be wrong there

@EngyNasr
Copy link
Contributor Author

its not very urgent to have now, so I will currently close the PR

@EngyNasr EngyNasr closed this Jun 15, 2022
@EngyNasr EngyNasr reopened this Jun 15, 2022
@EngyNasr
Copy link
Contributor Author

EngyNasr commented Jun 15, 2022

The tool needs a follow up on this Issue

@bernt-matthias
Copy link
Contributor

bernt-matthias commented Sep 20, 2022

Hey all. Marked add comments as resolved where I thought that they are. Also did some first edits. Hope that I can continue the next days. I get more and more request by my users for this tool. So I'm happy to jump in.

TODO:

  • test run time
  • some parameters could be put in macros
    • hsp
  • allow tabular.gz in and output
    • leave it for later

bernt-matthias and others added 2 commits February 13, 2023 13:44
Co-authored-by: Cristóbal Gallardo <gallardoalbac@gmail.com>
This reverts commit 465ba47.

must be proper python: True
@bernt-matthias
Copy link
Contributor

Hi @bgruening good to merge?

@bgruening
Copy link
Member

Oh yes, very cool! Thanks a lot everyone!

@bgruening bgruening merged commit 972784d into galaxyproject:main Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants