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

Allow specifying useful classes of tools for mapping in job conf YAML/XML #12258

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

jmchilton
Copy link
Member

Various job configurations I've seen in the wild and written for testing have special mappings setup for tools that cannot be used with remote Pulsar (since uploads are local to the web server for instance) or for tool that require Galaxy lib (including hacks like setting dynamic job runners just to import the lists from galaxy.tools and then not applying version restrictions correctly and such).

This provides abstractions to map whole classes of tools at a time to circumvent these hacks. The two classes currently defined are local and requires_galaxy as documented in the advanced job config.

local is for tools that we know require job handler files that don't respect compute environment abstractions and aren't setup for remote execution (e.g. won't work with a remote Pulsar instance). This will help deployers route these jobs away from Pulsar quickly and should help cleanup a lot of expected failures in galaxyproject/pulsar#259. This also cleans up a lot of the test configurations I think.

requires_galaxy this is for jobs that use tools that require Galaxy's Python environment. It should provide a more rigorous way to access GALAXY_LIB_TOOLS_UNVERSIONED and GALAXY_LIB_TOOLS_VERSIONED`` for non-dynamic job configuration mapping.

How to test the changes?

(Select all options that apply)

  • This is a refactoring of components with existing test coverage. (Not strictly true - more like I rewrote existing tests to leverage this feature and be cleaner - still it is testing the feature through that usage.)

License

  • I agree to license these contributions under Galaxy's current license.
  • I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to [email protected].

@github-actions github-actions bot added this to the 21.09 milestone Jul 14, 2021
@jmchilton jmchilton force-pushed the job_conf_tool_classes branch from f5af6a8 to 52cbaba Compare July 14, 2021 17:47
@@ -2431,6 +2441,7 @@ def exec_before_job(self, app, inp_data, out_data, param_dict=None):
class ExpressionTool(Tool):
requires_js_runtime = True
tool_type = 'expression'
tool_type_local = True
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't have to be local ? It just needs galaxy + node, right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

In your framework tests it seems to write stuff to the job directory that Pulsar doesn't know about it - so it will fail with Pulsar. I think we could change this if we fixed Pulsar - I would imagine approaching this like switching this variable when we update the client staging logic to account for these files.

Copy link
Member

Choose a reason for hiding this comment

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

If you add a TODO here I'm happy to merge this

Copy link
Member Author

Choose a reason for hiding this comment

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

@mvdbeek mvdbeek merged commit 85d5e12 into galaxyproject:dev Jul 15, 2021
@natefoo
Copy link
Member

natefoo commented Jul 20, 2021

Can we add a warning log message to the Pulsar runner so that any "local" tool that goes to Pulsar emits something helpful for admins? I also think that any local tool not explicitly mapped to Pulsar probably ought to go to a non-Pulsar destination, but there's no good way to do that beyond adding a "default local" option alongside "default"?

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

Successfully merging this pull request may close these issues.

3 participants