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

Enable per-destination container_resolver_config_file #15884

Conversation

bernt-matthias
Copy link
Contributor

@bernt-matthias bernt-matthias commented Mar 31, 2023

  • per destination container_resolvers per container_resolvers_config_file did not work because ContainerFinder did not check if this key is set in the destination_info (but only container_resolvers) this is crucial for job configuration using XML
  • per destination container_resolvers did not work if a global config was given with container_resolvers and per destination config used container_resolvers_config_file

Why this is needed / how I stumbled over this: Finally had an idea for migrating to containers on my site: I will define a

  • per destination config using cached_explicit_singularity, cached_mulled_singularity, i.e. this will only pick up containers that have been cached. tools that do not have a container installed will fall back to conda
  • global config using cached_explicit_singularity, cached_mulled_singularity, mulled_singularity which allows me to pull containers via AdminUI / API

Unfortunately my job config is still XML, so I need container_resolvers_config_file for the per destination config.

If someone wants to have this backported then I can do. For me it's fine to carry the single commit in my deployment branch for 1 release.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

- per destination container_resolvers per `container_resolvers_config_file`
  did not work because `ContainerFinder` did not check if this key is set
  in the `destination_info` (but only `container_resolvers`)
  this is crucial for job configuration using XML
- per destination container_resolvers did not work if a global config
  was given with `container_resolvers` and per destination config
  used `container_resolvers_config_file`

and there was one f missing in an f-string
@bernt-matthias bernt-matthias force-pushed the fix/per-destination-container-resolver-file branch 2 times, most recently from b04babe to 89cc0f1 Compare April 1, 2023 08:06
@bernt-matthias bernt-matthias changed the title Fix: per destination container resolver file Fix: per destination container_resolver_config_file Apr 1, 2023
@bernt-matthias bernt-matthias force-pushed the fix/per-destination-container-resolver-file branch from ad9f57c to ac91975 Compare April 2, 2023 08:37
from a file (with global container config configured at the same time)
@mvdbeek
Copy link
Member

mvdbeek commented Apr 3, 2023

This doesn't seem to be a fix ? I don't think it was meant to work this way. Otherwise I guess this is fine.

@mvdbeek mvdbeek changed the title Fix: per destination container_resolver_config_file Enable per-destination container_resolver_config_file Apr 3, 2023
@bernt-matthias
Copy link
Contributor Author

Thanks for having a look :)

@bernt-matthias bernt-matthias merged commit 967f08b into galaxyproject:dev Apr 3, 2023
40 checks passed
@bernt-matthias bernt-matthias deleted the fix/per-destination-container-resolver-file branch April 3, 2023 10:40
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

This PR was merged without a "kind/" label, please correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants