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: create task #207

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 23 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions deployment/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ custom:
- ParamToRecogniseDataComingFromConfig
taskmaster:
imageName: docker.io/elixircloud/tesk-core-taskmaster
imageVersion: v0.10.2
imageVersion: v0.10.4
filerImageName: docker.io/elixircloud/tesk-core-filer
filerImageVersion: v0.10.2
filerImageVersion: v0.10.4
ftp:
# Name of the secret with FTP account credentials
secretName: account-secret
Expand Down
1 change: 1 addition & 0 deletions docs/source/pages/tesk/tesk.api.ga4gh.tes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Subpackages
:maxdepth: 4

tesk.api.ga4gh.tes.service_info
tesk.api.ga4gh.tes.task

Submodules
----------
Expand Down
29 changes: 29 additions & 0 deletions docs/source/pages/tesk/tesk.api.ga4gh.tes.task.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
tesk.api.ga4gh.tes.task package
===============================

Submodules
----------

tesk.api.ga4gh.tes.task.create\_task module
-------------------------------------------

.. automodule:: tesk.api.ga4gh.tes.task.create_task
:members:
:undoc-members:
:show-inheritance:

tesk.api.ga4gh.tes.task.task\_request module
--------------------------------------------

.. automodule:: tesk.api.ga4gh.tes.task.task_request
:members:
:undoc-members:
:show-inheritance:

Module contents
---------------

.. automodule:: tesk.api.ga4gh.tes.task
:members:
:undoc-members:
:show-inheritance:
29 changes: 29 additions & 0 deletions docs/source/pages/tesk/tesk.k8s.converter.data.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
tesk.k8s.converter.data package
===============================

Submodules
----------

tesk.k8s.converter.data.job module
----------------------------------

.. automodule:: tesk.k8s.converter.data.job
:members:
:undoc-members:
:show-inheritance:

tesk.k8s.converter.data.task module
-----------------------------------

.. automodule:: tesk.k8s.converter.data.task
:members:
:undoc-members:
:show-inheritance:

Module contents
---------------

.. automodule:: tesk.k8s.converter.data
:members:
:undoc-members:
:show-inheritance:
45 changes: 45 additions & 0 deletions docs/source/pages/tesk/tesk.k8s.converter.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
tesk.k8s.converter package
==========================

Subpackages
-----------

.. toctree::
:maxdepth: 4

tesk.k8s.converter.data

Submodules
----------

tesk.k8s.converter.converter module
-----------------------------------

.. automodule:: tesk.k8s.converter.converter
:members:
:undoc-members:
:show-inheritance:

tesk.k8s.converter.executor\_command\_wrapper module
----------------------------------------------------

.. automodule:: tesk.k8s.converter.executor_command_wrapper
:members:
:undoc-members:
:show-inheritance:

tesk.k8s.converter.template module
----------------------------------

.. automodule:: tesk.k8s.converter.template
:members:
:undoc-members:
:show-inheritance:

Module contents
---------------

.. automodule:: tesk.k8s.converter
:members:
:undoc-members:
:show-inheritance:
8 changes: 8 additions & 0 deletions docs/source/pages/tesk/tesk.k8s.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
tesk.k8s package
================

Subpackages
-----------

.. toctree::
:maxdepth: 4

tesk.k8s.converter

Submodules
----------

Expand Down
16 changes: 12 additions & 4 deletions tesk/api/ga4gh/tes/controllers.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
"""Controllers for GA4GH TES API endpoints."""

import logging
from typing import Any

# from connexion import request # type: ignore
from foca.utils.logging import log_traffic # type: ignore
from pydantic import ValidationError

from tesk.api.ga4gh.tes.models import TesTask
from tesk.api.ga4gh.tes.service_info.service_info import ServiceInfo
from tesk.api.ga4gh.tes.task.create_task import CreateTesTask
from tesk.exceptions import BadRequest

# Get logger instance
logger = logging.getLogger(__name__)
Expand All @@ -26,14 +30,18 @@ def CancelTask(id, *args, **kwargs) -> dict: # type: ignore

# POST /tasks
@log_traffic
def CreateTask(*args, **kwargs) -> dict: # type: ignore
def CreateTask(**kwargs) -> dict:
"""Create task.

Args:
*args: Variable length argument list.
**kwargs: Arbitrary keyword arguments.
"""
pass
request_body: Any = kwargs.get("body")
JaeAeich marked this conversation as resolved.
Show resolved Hide resolved
try:
tes_task = TesTask(**request_body)
JaeAeich marked this conversation as resolved.
Show resolved Hide resolved
except ValidationError as e:
raise BadRequest(str(e)) from e
return CreateTesTask(tes_task).response()


# GET /tasks/service-info
Expand Down
2 changes: 1 addition & 1 deletion tesk/api/ga4gh/tes/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ class TesResources(BaseModel):
example={"VmSize": "Standard_D64_v3"},
)
backend_parameters_strict: Optional[bool] = Field(
False,
default=False,
description="If set to true, backends should fail the task if any "
"backend_parameters\nkey/values are unsupported, otherwise, backends should "
"attempt to run the task",
Expand Down
1 change: 1 addition & 0 deletions tesk/api/ga4gh/tes/task/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
"""Task API controller logic."""
93 changes: 93 additions & 0 deletions tesk/api/ga4gh/tes/task/create_task.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
"""TESK API module for creating a task."""

import logging
from http import HTTPStatus

from tesk.api.ga4gh.tes.models import TesCreateTaskResponse, TesResources, TesTask
from tesk.api.ga4gh.tes.task.task_request import TesTaskRequest
from tesk.exceptions import KubernetesError

logger = logging.getLogger(__name__)


class CreateTesTask(TesTaskRequest):
"""Create TES task.

Arguments:
TesTaskRequest: Base class for TES task request.
task: TES task to create.
"""

def __init__(
self,
task: TesTask,
):
"""Initialize the CreateTask class.

Args:
task: TES task to create.
"""
super().__init__()
self.task = task

def handle_request(self) -> TesCreateTaskResponse:
"""Create TES task.

Returns:
TesCreateTaskResponse: TES task response after creating corresponding K8s
job.
"""
attempts_no: int = 0
total_attempts_no: int = (
self.tesk_k8s_constants.job_constants.JOB_CREATE_ATTEMPTS_NO
)

while attempts_no < total_attempts_no:
try:
logger.debug(
f"Creating K8s job, attempt no: {attempts_no}/{total_attempts_no}."
)
attempts_no += 1
jemaltahir marked this conversation as resolved.
Show resolved Hide resolved
JaeAeich marked this conversation as resolved.
Show resolved Hide resolved

minimum_ram_gb = self.kubernetes_client_wrapper.minimum_ram_gb()

# Setting task resources based on the minimum RAM
if self.task.resources is None:
self.task.resources = TesResources(cpu_cores=int(minimum_ram_gb))
elif (
self.task.resources.ram_gb is None
or self.task.resources.ram_gb < minimum_ram_gb
):
self.task.resources.ram_gb = minimum_ram_gb

# Create the K8s job and config map
taskmaster_job = self.tes_kubernetes_converter.from_tes_task_to_k8s_job(
self.task,
)
taskmaster_config_map = (
self.tes_kubernetes_converter.from_tes_task_to_k8s_config_map(
self.task,
taskmaster_job,
)
)
_ = self.kubernetes_client_wrapper.create_config_map(
taskmaster_config_map
)
created_job = self.kubernetes_client_wrapper.create_job(taskmaster_job)

assert created_job.metadata is not None
assert created_job.metadata.name is not None
JaeAeich marked this conversation as resolved.
Show resolved Hide resolved

return TesCreateTaskResponse(id=created_job.metadata.name)

except KubernetesError as e:
if e.status == HTTPStatus.CONFLICT:
logger.debug(
"Conflict while creating Kubernetes job, retrying...",
)
pass

raise KubernetesError(
status=HTTPStatus.INTERNAL_SERVER_ERROR,
reason=f"Failed to create Kubernetes job after {attempts_no} attempts.",
)
45 changes: 45 additions & 0 deletions tesk/api/ga4gh/tes/task/task_request.py
Copy link
Member

Choose a reason for hiding this comment

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

Is it a base class for a TESK request (any request, including GET requests) or a task request (a POST requests to tasks/ in order to request a task being created). If the former, make sure to rename the module and class, if the latter, make sure to adapt the module-level docstring.

Copy link
Author

Choose a reason for hiding this comment

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

It is the former, can you please suggest a name, I can't think of a reason or name to do so. I name it task request so that this base class can hold all the attr that might be common among endpoint that deal with "task request" and create a common interface to interacting with api programatically.

If we extend more endpoint sooner or later (say create serviceInfo), then I would propose to even create a base class for this class named request or something, just so that programatically, business logic is forced to exit via the same method say response.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that second part is where I would see this going then. Maybe even ending up in FOCA.

I mean, if we already have 21 changed files (not including tests) for the addition of a single controller, we might as well go all the way, right? 😛

Copy link
Member

Choose a reason for hiding this comment

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

@JaeAeich: Why is it that with your PRs I often see the entire file changed, even though it's just an iteration over the last time I've viewed? I don't have this with other people, so I guess it's something with your editor or Git flow.

Please have a look at this, because it's really quite annoying. Instead of just focusing on what changed since the last time, I have to look through the entire file again - which is not only not fun, but also holds up the reviews big time, especially when they tend to end up being huge.

And even apart from that, it's also really not good practice in terms of provenance. If this were previously existing code (e.g., maybe some of the old TES code from TES-core still remains), you'd end up being listed as git blame for every line, taking credit and blame for other people's work.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I agree, but I can assure you am not going it on purpose 😭, the thing is TESK code is complex and if I try to break the code base it makes no sense and is very hard to connect the dots. Also I don't think I have any issues in my git flow, my configs seem to be sound. I am trying not to step and cover up someones code but if you notice we most of the files are completely new and not a modification of prev (well there weren't prev files to speack of as I am mostly working on new module api mostly and service isn't touched).

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
"""Base class for tesk request."""

import json
import logging
from abc import ABC, abstractmethod

from pydantic import BaseModel

from tesk.k8s.constants import tesk_k8s_constants
from tesk.k8s.converter.converter import TesKubernetesConverter
from tesk.k8s.wrapper import KubernetesClientWrapper

logger = logging.getLogger(__name__)


class TesTaskRequest(ABC):
"""Base class for tesk request ecapsulating common methods and members.

Attributes:
kubernetes_client_wrapper: kubernetes client wrapper
tes_kubernetes_converter: TES Kubernetes converter, used to convert TES requests
to Kubernetes resource
tesk_k8s_constants: TESK Kubernetes constants
"""

def __init__(self):
"""Initialise base class for tesk request."""
self.kubernetes_client_wrapper = KubernetesClientWrapper()
self.tes_kubernetes_converter = TesKubernetesConverter()
self.tesk_k8s_constants = tesk_k8s_constants

@abstractmethod
def handle_request(self) -> BaseModel:
"""Business logic for the request."""
pass

def response(self) -> dict:
"""Get response for the request."""
response: BaseModel = self.handle_request()
try:
res: dict = json.loads(json.dumps(response))
return res
except (TypeError, ValueError) as e:
logger.info(e)
Comment on lines +40 to +44
Copy link
Member

Choose a reason for hiding this comment

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

What's the significance of this? Why not going straight for the Pydantic way of marshalling the model?

Copy link
Author

Choose a reason for hiding this comment

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

I tried, for some reason I couldn't, IDK why! And this weird way only worked.

return response.dict()
5 changes: 5 additions & 0 deletions tesk/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ class KubernetesError(ApiException):
"detail": "An unexpected error occurred.",
"status": 500,
},
KubernetesError: {
"title": "Kubernetes error",
"detail": "An error occurred while interacting with Kubernetes.",
"status": 500,
},
}


Expand Down
1 change: 1 addition & 0 deletions tesk/k8s/converter/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
"""Module for converting Kubernetes objects to Task objects."""
Loading
Loading