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

Fix braker3 breaking TPV changes #1096

Conversation

sanjaysrikakulam
Copy link
Member

@sanjaysrikakulam sanjaysrikakulam commented Feb 2, 2024

Due to various TPV issues, I am restructuring (partially reverting it to match what was there) the changes introduced via the below PRs.

  1. Update Braker3  #1091
  2. braker3/tpv: match complete tool versions #1094

I have rolled out a version of this PR locally after several trial-and-error runs (thanks to @abueg for testing and reporting the issues).

@rlibouba: Since the EU does not yet have the 3.0.7 version installed, I have removed it from the current TPV scheduling and updated the container images for both versions 3.0.3 and 3.0.6. I had to remove it because I am unsure about the GENEMARK_PATH and the other env to be added. Please feel free to update once it is installed. To add a new version of this tool the change should go to this file https://github.com/usegalaxy-eu/usegalaxy-eu-tools/blob/9bdcca26aefcfece5f261b5c189104acf858e9b8/genome-annotation.yaml.lock#L253-L259 if I am not mistaken (@bgruening will be able to clarify on that).

@sanjaysrikakulam sanjaysrikakulam marked this pull request as draft February 2, 2024 15:30
@sanjaysrikakulam
Copy link
Member Author

I am changing the PR to draft because @abueg just informed me that the job finished in an error state after running for some time.

The error is:

ERROR in file /opt/BRAKER/scripts/braker.pl at line 5893
Failed to create new species with new_species.pl, check write permissions in /usr/share/augustus/config//species directory! Command was /usr/bin/perl /usr/share/augustus/scripts/new_species.pl --species=Propanagrolaimus --AUGUSTUS_CONFIG_PATH=/usr/share/augustus/config/ 1> /dev/null 2>/data/jwd05e/main/066/810/66810392/working/braker/errors/new_species.stderr

This error is arising from the container. Previously, we did not use containers for the 3.0.6 version. Is anyone familiar with this error?

@sanjaysrikakulam sanjaysrikakulam marked this pull request as ready for review February 2, 2024 15:48
@sanjaysrikakulam sanjaysrikakulam marked this pull request as draft February 2, 2024 15:48
@sanjaysrikakulam
Copy link
Member Author

@abueg confirmed that the tool was working before the update; therefore, I am reverting all the changes made to braker3 since the training needs to continue.

@sanjaysrikakulam sanjaysrikakulam mentioned this pull request Feb 2, 2024
@bgruening
Copy link
Member

I guess the new version gets installed automatically over the weekend. Should I prevent this?

@sanjaysrikakulam
Copy link
Member Author

I think that would be the best for now. We need a solution for the error; I am unsure whether we can simply pass in /tmp/ as a directory for the AUGUSTUS_CONFIG_PATH parameter.

@sanjaysrikakulam
Copy link
Member Author

The revert is working fine; I see the job run without errors. Upon looking into the running jobs, I see that the value for AUGUSTUS_CONFIG_PATH is set to the path /usr/local/tools/_conda/envs/[email protected]/config/ as default (probably handled by braker3 tool internally to fallback to the bin dir) when the tool runs. That is probably why we never saw this error before. Perhaps the container sets the value to what we saw in the error above and complains that it is not writable.

@sanjaysrikakulam
Copy link
Member Author

@abretaud, any ideas?

@abretaud
Copy link
Contributor

I think a line like this would be enough: https://github.com/galaxyproject/tools-iuc/blob/669dd4d406e86aee7d83e7420dced981c180d805/tools/augustus/augustus_training.xml#L16

But I dont understand why I've never encountered it while testing locally or on .fr 🤔 Need to check that

@sanjaysrikakulam
Copy link
Member Author

sanjaysrikakulam commented Feb 12, 2024

Thank you! Don't we have to copy the contents of the config directory to the set path like mentioned in their readme here

"""
⚠️ Users have reported that you need to manually copy the AUGUSTUS_CONFIG_PATH contents to a writable location before running our containers from Nextflow. Afterwards, you need to specify the writable AUGUSTUS_CONFIG_PATH as command line argument to BRAKER in Nextflow.
"""

We can ignore Nextflow 's specificity. ' Based on the error, I'd assume it's the same for everyone using their container.

@abretaud
Copy link
Contributor

Yeah, probably copying is better, like we do here: https://github.com/galaxyproject/tools-iuc/blob/669dd4d406e86aee7d83e7420dced981c180d805/tools/busco/busco.xml#L20

I can open a PR on the tool to fix it

@sanjaysrikakulam
Copy link
Member Author

Thank you! That would be great.

Yes, copying is better; however, we cannot perform that action via TPV. Hence, I am unable to configure the tool to use containers which was intended via the initial PR.

Once the tool is patched, I think all I have to do via TPV would be to change them to container versions and exclusively add an env AUGUSTUS_CONFIG_PATH: pwd/augustus_dir/ like we do for the GENEMARK_PATH: "/usr/local/tools/genemark/etp.for_braker/bin/gmes/" and the tool can use the set AUGUSTUS_CONFIG_PATH and copy the contents to that location.

@sanjaysrikakulam
Copy link
Member Author

Sorry, misunderstood. So, if the tool is patched like shared above in Busco, I think we do not have to set this path exclusively on TPV. Please correct me if I am wrong.

@abretaud
Copy link
Contributor

Just opened genouest/galaxy-tools#45

With this patch you shouldn't need to change any env var in TPV

@sanjaysrikakulam
Copy link
Member Author

Cool! Thank you very much! Appreciate it! :)

@sanjaysrikakulam
Copy link
Member Author

Solved via #1102

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.

3 participants