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

feat: sequential execution of middleware #160

Merged
merged 16 commits into from
Oct 20, 2023
Merged

Conversation

Ayush5120
Copy link
Contributor

@Ayush5120 Ayush5120 commented Jun 19, 2023

  • Support for sequential execution of middleware to be injected into incoming POST /tasks requests
  • Middleware classes to be executed can be specified, in fully qualified docstring notation, in the app configuration, in the desired order of execution, e.g.:
    middlewares:
      - "package.subpackage.module.MiddlewareClass1"
      - "package.subpackage.module.MiddlewareClass2"
  • It is possible to specify one or more "fallback middlewares", in case the desired middleware fails; to achieve that, list nesting by one level is supported, such that each inner list is treated as a single "middleware group", from which at most one middleware is applied - the first one that does not raise a MiddlewareException; in the following example, proTES first attempts to apply MiddlewareClass1a; if it works, it will advance to the next item in the outer list, MiddlewareClass2 and try to apply that; however, if MiddlewareClass1a raises an error, proTES will first try to apply MiddlewareClass1b before progressing to MiddlewareClass2:
    middlewares:
      - - "package.subpackage.module.MiddlewareClass1a"
        - "package.subpackage.module.MiddlewareClass1b"
      - "package.subpackage.module.MiddlewareClass2"
  • If all middlewares of any middleware group fail (including in groups of size 1, i.e, without list nesting), proTES will respond with a 500 response
  • Developers can implement their own middlewares by implementing the pro_tes.middleware.abstract_middleware.AbstractMiddlewareClass; of course, any middleware class provided via external packages needs to be made available to the application (e.g., by adding the corresponding package to requirements.txt and rebuilding the app and worker images)
  • Available middlewares for task distribution (random and distance-based) are now available in pro_tes.plugins.middlewares.task_distribution with all their dependencies
  • Task distribution middlewares are applied by default, as a single middleware group, with the distance-based distribution (pro_tes.plugins.middlewares.task_distribution.distance.TaskDistributionDistance) being attempted first, and with the random-based task-distribution (pro_tes.plugins.middlewares.task_distribution.random.TaskDistributionRandom) serving as a fallback; this behavior can be changed by modifying the middlewares app configuration section
  • Developers can optionally make use of the base class pro_tes.plugins.middlewares.task_distribution.base.TaskDistributionBaseClass for implementing other task distribution middlewares

Minor changes:

  • Replace legacy (Python <=3.7) Type hints for Lists, Dicts and Tuples with their lowercase counterparts across the entire application
  • Silence mypy complaints about missing type hints/stubs in external packages
  • Drastically simplify middleware handler and task distribution middleware code
  • Streamline POST /tasks controller code as a preparatory step for a major refactoring
  • Streamline logging
  • Temporarily disable unit tests in preparation for an effort to write consistent unit tests for the entire application

Closes #124

@Ayush5120 Ayush5120 marked this pull request as ready for review July 31, 2023 13:21
@Ayush5120 Ayush5120 changed the title Enable Sequential Execution of Configurable Middleware Functions via Config feat: enable sequential execution of configurable middleware functions via config Jul 31, 2023
@Ayush5120 Ayush5120 requested a review from uniqueg August 3, 2023 06:56
@uniqueg uniqueg changed the title feat: enable sequential execution of configurable middleware functions via config feat: sequential execution of middleware Aug 14, 2023
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

Please have a look at the comments. I think I would structure this differently to simplify the syntax and give an easy way to define middlewares without fallbacks. I think it would be semantically cleaner, too (and possibly also less code).

pro_tes/config.yaml Outdated Show resolved Hide resolved
pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

Still some work needed, but we're getting there

pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

I'm really sorry to be nagging, but I really want to get this right before it's copied into proWES :-S

pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
pro_tes/ga4gh/tes/task_runs.py Outdated Show resolved Hide resolved
pro_tes/ga4gh/tes/task_runs.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware_pipeline.py Outdated Show resolved Hide resolved
pro_tes/middleware/middleware.py Outdated Show resolved Hide resolved
pro_tes/ga4gh/tes/task_runs.py Outdated Show resolved Hide resolved
@Ayush5120
Copy link
Contributor Author

I'm really sorry to be nagging, but I really want to get this right before it's copied into proWES :-S

No Problem at all, I will fix them all in the next commit.

@uniqueg uniqueg merged commit 21a555e into dev Oct 20, 2023
5 checks passed
@uniqueg uniqueg deleted the restructure_middleware branch October 20, 2023 01:34
@JaeAeich JaeAeich mentioned this pull request Feb 7, 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

Successfully merging this pull request may close these issues.

Config param to choose task distribution logic
2 participants