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

Evaluation of params in destination section #135

Closed
lmfaber opened this issue Aug 16, 2024 · 2 comments
Closed

Evaluation of params in destination section #135

lmfaber opened this issue Aug 16, 2024 · 2 comments

Comments

@lmfaber
Copy link

lmfaber commented Aug 16, 2024

Hi everyone,

Short bug description
I'd like to report what I think is a bug with TPVs parsing of params. It seems that the f-string evaluation from TPV is working properly in the tools section, however not working in the destinations section.

Debugging information:

  • Galaxy 24.1.1 on a Ubuntu 22.04.4 server
  • The venv Galaxy is running on is using Python 3.12.3 and total_perspective_vortex is installed in version 2.4.0.

Detailed description
I was setting up a self-hosted Galaxy instance using Ansible, following this tutorial. At the step job destinations I had some issues with the walltime parameter. The tutorial suggests (in a shortened way) the following setup for the tpv_rules_local.yml file:

tools:
  default:
    ...
    rules:
      - id: resource_params_defined
        if: |
          param_dict = job.get_param_values(app)
          param_dict.get('__job_resource', {}).get('__job_resource__select') == 'yes'
        cores: int(job.get_param_values(app)['__job_resource']['cores'])
        params:
           walltime: "{int(job.get_param_values(app)['__job_resource']['time'])}"
   
destinations:
  local_env:
    ...
    params:
      native_specification: --nodes=1 --ntasks=1 --cpus-per-task={cores} --time={params['walltime']}:00:00

However, this only works in a limited way (at least for me). In the tools section the usage "{int(job.get_param_values(app)['__job_resource']['time'])}" works fine and the walltime is properly displayed in the job info of Galaxy. See red box in the image:

walltime

However, in the destination section the native_specification: --nodes=1 --ntasks=1 --cpus-per-task={cores} --time={params['walltime']}:00:00 will not correctly parse the walltime.

  • The errors suggest to use self.params['walltime']. Which also only works half-way.
  • If I use the above-mentioned self.params['walltime'], the resulting string will be the literal "{int(job.get_param_values(app)['__job_resource']['time'])}", which are not correctly evaluated in this case. This will lead to an error with the job submission, because the walltime will look like this: --time="{int(job.get_param_values(app)['__job_resource']['time'])}":00:00.

Workaround
I have found a workaround for this issue which I'd like to share, but I think the above-mentioned way is the way the developers from TPV intended the usage, as it is also in the docs.

Code snippet for the native specification:

native_specification: --partition=galaxy2 --nodes=1 --ntasks=1 --mem={int(mem * 1000)} --cpus-per-task={cores} --time={self.params.get('walltime') if isinstance(self.params.get('walltime'), int) else job.get_param_values(app).get('__job_resource').get('walltime')}:00:00

This code will get the self.params.get('walltime') for the case that job resources are not defined and else will get the resources from the __job_resources. job.get_param_values(app).get('__job_resource').get('walltime') Needs to be called directly here and it will be evaluated correctly:

I have attached my relevant configurations for anyone who faces the same issues.
tpv_rules_local.yml.txt
job_resource_params_conf.xml.j2.txt
job_conf.yml.txt

Summary
If this is not a bug and working as intended, the issue can be closed, I just wanted to document this for anyone facing the same issues as me. If this is indeed a bug, I think it would be nice to either

  • Fix the issue within TPV itself.
  • Update the documentation in the Galaxy Ansible setup.
  • Update the documentation of TPV. I think TPV by example would be a fitting section.

Hope this helps. Keep up the good work! Cheers!

@nuwang
Copy link
Member

nuwang commented Aug 18, 2024

@lmfaber Thanks for the very detailed bug report. There is indeed a problem with the example. Please take a look at this conversation for a solution: #120

We are hoping to add support for computed context variables, but till that comes, the best option is to use the very last example from that code (using env vars)

@lmfaber
Copy link
Author

lmfaber commented Sep 3, 2024

Thanks for the answer and linking the other post. I saw this post before writing my own, but somehow couldn't make the connection in my head.
I will close the issue, since it's so similar to the other one.

@lmfaber lmfaber closed this as completed Sep 3, 2024
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

No branches or pull requests

2 participants