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

Tes compliance: support multiple executors #427

Open
uniqueg opened this issue Sep 29, 2023 · 4 comments · May be fixed by #790
Open

Tes compliance: support multiple executors #427

uniqueg opened this issue Sep 29, 2023 · 4 comments · May be fixed by #790
Labels
bug Something isn't working enhancement New feature or request Size: small TES Priority: P1 Groomed to a Priority 1 issue Usability Enable TES is easy to use for end users
Milestone

Comments

@uniqueg
Copy link

uniqueg commented Sep 29, 2023

Problem:

As per the current TES specification, multiple executors can be specified in a single request:

components:
  ...
  schemas:
    ...
    tesTask:
      ...
      properties:
        ...
        executors:
          type: array
          description: |-
            An array of executors to be run. Each of the executors will run one
            at a time sequentially. Each executor is a different command that
            will be run, and each can utilize a different docker image. But each of
            the executors will see the same mapped inputs and volumes that are declared
            in the parent CreateTask message.

            Execution stops on the first error.
          items:
            $ref: '#/components/schemas/tesExecutor'

Since TES v1.1, the behavior for the case in which one of multiple executors fails can further be modified with the ignore_error property of the tesExecutor schema:

components:
  ...
  schemas:
    ...
    tesExecutor:
      ...
      properties:
        ...
        ignore_error:
          type: boolean
          description: |-
            Default behavior of running an array of executors is that execution
            stops on the first error. If `ignore_error` is `True`, then the
            runner will record error exit codes, but will continue on to the next
            tesExecutor.

As discussed with @MattMcL4475, multiple executors are currently not supported in TES on Azure.

However, some client implementations such as workflow engines may rely on this feature, e.g., for setup and teardown steps or for scheduling two or more closely related tasks with similar requirements to minimize overheads.

Solution:

  1. Implement support for multiple executors as described.
  2. Implement the ignore_error property of the tesExecutor schema as described.

Describe alternatives you've considered
N/A

Code dependencies
Not sure.

Additional context
Appropriate tests for this behavior are currently unavailable (see ga4gh/compliance-tests-ga4gh-tes#2).

@ngambani ngambani added bug Something isn't working enhancement New feature or request Usability Enable TES is easy to use for end users tobegroomed Add this label while creating new issues to get issues prioritized on the backlog Size: small labels Oct 11, 2023
@MattMcL4475 MattMcL4475 added the TES Priority: P3 Groomed to a Priority 3 issue label Dec 11, 2023
@BMurri BMurri removed the tobegroomed Add this label while creating new issues to get issues prioritized on the backlog label Apr 20, 2024
@MattMcL4475
Copy link
Collaborator

@adamnovak would implementing this issue be a significant help to implementing TES with Toil? We are doing a round of issue prioritization and your input on this would be helpful, thanks!

@MattMcL4475 MattMcL4475 added TES Priority: P1 Groomed to a Priority 1 issue and removed TES Priority: P3 Groomed to a Priority 3 issue labels Sep 27, 2024
@adamnovak
Copy link

I think that having access to multiple executors would be a big help for allowing Toil to efficiently implement some parts of WDL, such as the glob() function. I think if we could run some Toil code against the same volumes as the user container, and use TES's ability to set up a wildcard match for outputs like output_files/*/*, it would be possible to fully implement WDL without uploading any unnecessary data to shared storage. We might still be able to achieve this without multiple executors, but it would involve either implementing a lot of WDL in Bash or else somehow injecting enough Python to run Toil into the user container.

Without being able to run Toil code against the same volumes, for WDL tasks where the output files need to be dynamically determined, we'd need to upload everything that potentially could be an output to shared storage and then go through it to identify the actual outputs.

@BMurri BMurri linked a pull request Sep 28, 2024 that will close this issue
@uniqueg
Copy link
Author

uniqueg commented Sep 28, 2024

@MattMcL4475: Please also note that, as part of the core TES specs, TES compliant clients will rely on multiple executors to be supported. While so far we are making use of multiple executors to support individual server-side solutions (and so are not affected by TES-on-Azure's non-compliance), there is at least one project that we are working on (supporting Crypt4GH files in TES) that uses a middleware approach relying on multiple executors. But since we believe that the middleware-executor pattern is powerful for supercharging any TES implementation with individual functionalities, I think there will be more of those use cases that TES-on-Azure, without support for multiple executors, would miss out on.

@MattMcL4475
Copy link
Collaborator

Thank you @adamnovak and @uniqueg for the feedback, we've made this a top priority and we'll have it implemented and released in 5.4.4 this week. We also noticed that with this change, the repo will pass 100% of the TES 1.1 compliance tests (currently it passes 100% of TES 1.0)! So it will be great to get this done.

Please keep the feedback coming if you see any high-priority features/bugs, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request Size: small TES Priority: P1 Groomed to a Priority 1 issue Usability Enable TES is easy to use for end users
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants