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

Galaxy cleanup #48

Merged
merged 14 commits into from
Feb 27, 2024
Merged

Galaxy cleanup #48

merged 14 commits into from
Feb 27, 2024

Conversation

Oddant1
Copy link
Member

@Oddant1 Oddant1 commented Jan 24, 2024

Deal with various q2galaxy related issues most prominently #46

else:
_, attr_name = key.split("_")
if 'elements' in value:
ext = value.get('ext', '')
files_to_move.extend([
(v['data'], v['name'] + ext) for v in value['elements']])
(v['source_path'],
v['staging_path'].split('.fastqsanger.gz')[0] + ext)

Choose a reason for hiding this comment

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

The .fastqsanger.gz suffix was only added during test file upload (haven't tested this a lot .. but might be a Galaxy bug). This should usually not appear. I guess it might work all the time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that was something I was curious about, and this little split here feels bad and hacky, I'll be trying to figure out what exactly is going on there if I can.

Choose a reason for hiding this comment

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

I researched this a bit. "staging_path" will be "{element_identifier}.{ext}", where

  • element_identifier is the name of a collection element or the dataset name
    • "/" characters in identifiers are replaced by "_"
    • for collections this is prefixed by a "/" separated list of collection identifiers (for nested collections there could be more than one "/")
  • "ext" is the datatype used in Galaxy. This will always be set, i.e. I was wrong that this only happens in tool tests.
    The available datatypes used in Galaxy (https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/config/sample/datatypes_conf.xml.sample)

I'm currently working on a PR that would add "element_identifier", "ext" and "name" (which is the dataset name .. which is seldomly useful in tools). Until this is done we have to resort to hack solutions (I guess), I guess most secure would be to remove only the longest suffix matching a Galaxy datatype.

template_body = dedent('\n'.join(template_psp_lines))
return f'''<%
{template_body}
%>'''
Copy link
Member

Choose a reason for hiding this comment

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

THANK GOODNESS. RIP in peace! ❤️

@Oddant1 Oddant1 self-assigned this Jan 25, 2024
@bernt-matthias
Copy link

Using staging_path_and_source_path will become really useful with galaxyproject/galaxy#17357 (which I just implemented yesterday) which should make it to Galaxy 24.0 (expected maybe late spring). Then we do not need to parse the element identifiers from the staging paths, but have direct access to the element identifiers. This would require to increase the tool profile to 24.0.

So we might wait for this, in particular since we have a good workaround: qiime2/galaxy-tools#51. But if you prefer to implement this earlier, i.e. to parse the staging path, then I would fix the allowed datatypes in the collection inputs (which would be good anyway!), e.g.

- <param name="elements" type="data_collection" collection_type="list" help="This data should be formatted as a FastqGzFormat. See the documentation below for more information. Elements must match regex: .+_.+_R[12]_001\.fastq\.gz"/>
+ <param name="elements" type="data_collection" collection_type="list" format="fastqsanger,fastqsanger.gz" help="This data should be formatted as a FastqGzFormat. See the documentation below for more information. Elements must match regex: .+_.+_R[12]_001\.fastq\.gz"/>

Then it should be safe to remove text matching one of the allowed extensions from the suffix. Taking only the text after the last "/" as sample identifier should work as long as users do not use "/" in sample names (this would be the part I would consider a hack).

Let me know if you need further background on Galaxy (fastq) datatypes, because there are a few (fastq, fastqsanger, fastqillumina + the same with .gz).

@Oddant1
Copy link
Member Author

Oddant1 commented Jan 29, 2024

@bernt-matthias It may well be worth our while to delay these import changes to q2galaxy until Galaxy 24.0 then, but I'll need to talk to @gregcaporaso and @ebolyen about that as well

@bernt-matthias
Copy link

delay these import changes to q2galaxy until Galaxy 24.0

Definitely. Otherwise it won't work.

@gregcaporaso
Copy link
Member

I'll need to talk to @gregcaporaso and @ebolyen about that as well

Looks like we have some agenda items for tomorrow's meeting :)

@Oddant1
Copy link
Member Author

Oddant1 commented Feb 5, 2024

I'm changing this PR to use the reworked version of import that works in current galaxy releases in the interest of having this PR mergeable before the next QIIME 2 release. Getting rid of the templated in script will have to wait for now, but at least we have a viable path forwards there in the future. Thank you @bernt-matthias

@bernt-matthias
Copy link

that works in current galaxy releases

you should the use 24.0 as value for the profile attribute of the tool tag in all tools.

@Oddant1
Copy link
Member Author

Oddant1 commented Feb 9, 2024

@bernt-matthias would you be able to help me debug a new issue I'm encountering with our deblur denoise-16S tool? I am not seeing this issue on cancer.usegalaxy.org, but I am seeing it locally and on usegalaxy.eu. When I run the tool I get the following error message with near as I can tell no other context.


image


I tried googling this issue, and that error code seems to usually be accompanied by a "x command not found" message, but I'm not getting that.

cancer.usegalaxy.org is still running galaxy 22.01, so it's possible that it's related to newer versions of galaxy in some way, but I'm getting very little to go off of here.

@Oddant1
Copy link
Member Author

Oddant1 commented Feb 9, 2024

you should the use 24.0 as value for the profile attribute of the tool tag in all tools.

We are planning on merging this PR by end of next week and using it to render the galaxy-tools for the QIIME 2 2024.2 release. The changes here do not require any galaxy changes, so I don't think we actually want this for this PR.

@bernt-matthias
Copy link

I tried googling this issue, and that error code seems to usually be accompanied by a "x command not found" message, but I'm not getting that.

The job info page contains stdout and stderr - which might contain a clue.

cancer.usegalaxy.org is still running galaxy 22.01, so it's possible that it's related to newer

You may be able to test this locally by running planemo test --galaxy_branch release_22.01 tool.xml (probably also with --biocontainers --no_dependency_resolution --no_conda_auto_init )

We are planning on merging this PR by end of next week and using it to render the galaxy-tools for the QIIME 2 2024.2 release. The changes here do not require any galaxy changes, so I don't think we actually want this for this PR.

Fine. I missed that the ad58f22 has been reverted.

@Oddant1
Copy link
Member Author

Oddant1 commented Feb 12, 2024

@bernt-matthias the stderr and stdout for the failure are both empty for all 3 outputs which is what makes this so baffling. All I have on all three tool outputs is

Execution resulted in the following message:
Fatal error: Exit code 127 ()

and

Job Messages:
{ "code_desc": "", "desc": "Fatal error: Exit code 127 ()", "error_level": 3, "exit_code": 127, "type": "exit_code" }

@bernt-matthias
Copy link

Is the generated code somewhere? Then I could test it.

@Oddant1
Copy link
Member Author

Oddant1 commented Feb 12, 2024

It's the qiime2 deblur denoise-16S tool on usegalaxy.eu (I am also able to produce locally using galaxy 23.1). If you put this file from the test data through it you should get the error. Set the trim_length to 120 and sample_stats under additional options to true.

I really appreciate all of your help by the way. Our resident galaxy guy has only been working part of the day on Thursdays for the past few weeks, and I don't know nearly as much about this stuff as he does (and have been needing to bounce between this and several other things), so your help has been invaluable.

@lizgehret lizgehret linked an issue Feb 13, 2024 that may be closed by this pull request
@Oddant1 Oddant1 requested a review from ebolyen February 14, 2024 18:51
@bernt-matthias
Copy link

Seems that usegalaxy.eu has old qiime2 versions installed (Galaxy Version 2022.11.1+q2galaxy.2022.11.1.2). Could you have a look @bgruening?

Oddly usegalaxy.org has recent tool versions (Galaxy Version 2023.5.0+q2galaxy.2023.5.0.2) but I can not find qiime2 deblur denoise-16S ..

@bgruening
Copy link

I have updated all tools to the latest version in usegalaxy-eu/usegalaxy-eu-tools@5ed4a91 those will be installed over the weekend.

Thanks for the ping.

@Oddant1
Copy link
Member Author

Oddant1 commented Feb 21, 2024

@bernt-matthias it's strange that you can't find it, I am seeing it on both usegalaxy.org and usegalaxy.eu. I was out of the office for a few days, but I had more time to debug now and the local failures I was seeing while perplexing were seemingly unrelated to the issue on usegalaxy.eu, and I think I resolved the issue I was seeing there.

I am currently still seeing usegalaxy.eu as running QIIME 2 2022.11, and I am still getting Fatal error: Exit code 127 () when running the tool there. I suspect the issue is specific to that galaxy install and will probably be resolved when the tool update goes through. It's probably somehow missing the underlying deblur tool somehow because qiime2 deblur denoise-other also fails on usegalaxy.eu.

For the record, both deblur actions work on cancer.usegalaxy.org (QIIME 2 2022.11) and usegalaxy.org (QIIME 2 2023.5).

@bernt-matthias
Copy link

it's strange that you can't find it, I am seeing it on both usegalaxy.org and usegalaxy.eu.

Yeah, this just has been fixed on org: galaxyproject/usegalaxy-tools#688. Good that it works now.

@bgruening lets update this one as well usegalaxy-eu/usegalaxy-eu-tools#676 :)

Would be great if you could share the history containing the failing jobs (maybe also the successful one on cancer..... strange that it works there), then the usegalaxy.eu people could debug it.

@bgruening
Copy link

Tool is updated :)

@Oddant1
Copy link
Member Author

Oddant1 commented Feb 21, 2024

@bernt-matthias and @bgruening I am now seeing QIIME 2 2023.5 on usegalaxy.eu, but I am still getting the same failures.

Here is a usegalaxy.eu history showing me uploading the test data and running qiime2 deblur denoise-16S in QIIME 2 2023.5 with the tool failing (it failed almost instantly after submitting the job).

Here is a usegalaxy.org history showing the QIIME 2 2023.5 version of the tool working.

Here is a cancer.usegalaxy.org history showing the QIIME 2 2022.11 version of the tool working

@bernt-matthias
Copy link

Guess only the people form eu can answer this one.

@bgruening
Copy link

We are looking at this, it has something to do with the container setup. Will inform you if we find something.

Copy link
Member

@ebolyen ebolyen left a comment

Choose a reason for hiding this comment

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

This all looks good and tests pass locally

@ebolyen ebolyen merged commit 2bd7ea3 into qiime2:dev Feb 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: In Development
Development

Successfully merging this pull request may close these issues.

qiime2_core__tools__import stacktrace from usegalaxy.eu
5 participants