-
Notifications
You must be signed in to change notification settings - Fork 30
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: add base classes and docs #191
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,14 @@ | ||
tesk | ||
TESK | ||
==== | ||
|
||
.. toctree:: | ||
:maxdepth: 4 | ||
:caption: Services (filer and taskmaster) | ||
|
||
tesk | ||
tesk.services | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why have we altered this? I see the parent model is still tesk? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. segregation in the sidebar of documentation. |
||
.. toctree:: | ||
:maxdepth: 4 | ||
:caption: API | ||
|
||
tesk.api |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
tesk.api.ga4gh package | ||
====================== | ||
|
||
Subpackages | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the difference between submodules and subpackages? Any reason why we have used subpackages here and submodules at all other places? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. autogenerated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the docs are auto-generated - why do we version control them, then? Does this require you to build the docs before committing (and if so, what if you forget)? Couldn't you just auto-generate them as part of your docs deployment and leave them out of the CI? For packages I wrote and documented their APIs via Sphinx and RtD, I only kept the Admittedly, the API docs I generated with Sphinx and RtD are pretty shitty 🤣 Still, do we really need these files here? |
||
----------- | ||
|
||
.. toctree:: | ||
:maxdepth: 4 | ||
|
||
tesk.api.ga4gh.tes | ||
|
||
Module contents | ||
--------------- | ||
|
||
.. automodule:: tesk.api.ga4gh | ||
:members: | ||
:undoc-members: | ||
:show-inheritance: |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
tesk.api.ga4gh.tes.base package | ||
=============================== | ||
|
||
Submodules | ||
---------- | ||
|
||
tesk.api.ga4gh.tes.base.base\_tesk\_request module | ||
-------------------------------------------------- | ||
|
||
.. automodule:: tesk.api.ga4gh.tes.base.base_tesk_request | ||
:members: | ||
:undoc-members: | ||
:show-inheritance: | ||
|
||
Module contents | ||
--------------- | ||
|
||
.. automodule:: tesk.api.ga4gh.tes.base | ||
:members: | ||
:undoc-members: | ||
:show-inheritance: |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
tesk.api.ga4gh.tes package | ||
========================== | ||
|
||
Subpackages | ||
----------- | ||
|
||
.. toctree:: | ||
:maxdepth: 4 | ||
|
||
tesk.api.ga4gh.tes.base | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. autogenerated |
||
|
||
Submodules | ||
---------- | ||
|
||
tesk.api.ga4gh.tes.controllers module | ||
------------------------------------- | ||
|
||
.. automodule:: tesk.api.ga4gh.tes.controllers | ||
JaeAeich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
:members: | ||
:undoc-members: | ||
:show-inheritance: | ||
|
||
Module contents | ||
--------------- | ||
|
||
.. automodule:: tesk.api.ga4gh.tes | ||
:members: | ||
:undoc-members: | ||
:show-inheritance: |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
tesk.api package | ||
================ | ||
|
||
Subpackages | ||
----------- | ||
|
||
.. toctree:: | ||
:maxdepth: 4 | ||
|
||
tesk.api.ga4gh | ||
|
||
Module contents | ||
--------------- | ||
|
||
.. automodule:: tesk.api | ||
:members: | ||
:undoc-members: | ||
:show-inheritance: |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
"""Package for base class for TESK API request.""" |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose you are aware that Connexion validates requests and responses based on the OpenAPI schema without us having to write models? It's basically the main reason we use Connexion. Admittedly, this predates the time of Pydantic - and I do like a good Pydantic model. However, this all may really not be necessary. On another note, apart from your doc strings, I don't see anything TES-specific in here... |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
"""Base classes for the TES API request.""" | ||
JaeAeich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
from abc import ABC, abstractmethod | ||
from typing import Any, final | ||
|
||
from pydantic import BaseModel | ||
|
||
from tesk.tesk_app import TeskApp | ||
|
||
|
||
class BaseTeskRequest(ABC, TeskApp): | ||
"""Base class for the TES API. | ||
|
||
This class is an abstract class that defines the common properties and | ||
methods needed by all of the TES API endpoint business logic. | ||
""" | ||
|
||
def __init__(self) -> None: | ||
"""Initializes the BaseTeskRequest class.""" | ||
super().__init__() | ||
|
||
@abstractmethod | ||
def api_response(self) -> BaseModel: | ||
"""Returns the response as Pydantic model. | ||
|
||
Should be implemented by the child class as final | ||
business logic for the specific endpoint. | ||
|
||
Returns: | ||
BaseModel: API response for the specific endpoint. | ||
""" | ||
pass | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider throwing an exception here. |
||
|
||
@final | ||
def response(self) -> dict[Any, Any]: | ||
JaeAeich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""Returns serialized response. | ||
|
||
Should be invoked by controller. | ||
|
||
Returns: | ||
dict: Serialized response for the specific endpoint. | ||
""" | ||
_response = self.api_response() | ||
if not isinstance(_response, BaseModel): | ||
raise TypeError('API response must be a Pydantic model.') | ||
return _response.dict() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
"""Package for base class for custom pydantic validators.""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
"""Base validator class, all custom validator must implement it.""" | ||
JaeAeich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
import logging | ||
from abc import ABC, abstractmethod | ||
from typing import Any, Generic, TypeVar | ||
|
||
from pydantic import ValidationError | ||
|
||
T = TypeVar('T') | ||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class BaseValidator(ABC, Generic[T]): | ||
"""Base custom validator class. | ||
|
||
Validators assume that the filed being validated is not optional as | ||
optional fields are handled by the Pydantic model itself and mypy type | ||
checking. | ||
""" | ||
|
||
@property | ||
@abstractmethod | ||
def error_message(self) -> str: | ||
"""Returns the error message. | ||
|
||
Returns: | ||
str: The error message to be used when validation fails. | ||
""" | ||
pass | ||
|
||
@abstractmethod | ||
def validation_logic(self, v: T) -> bool: | ||
"""Validation logic for the field. | ||
|
||
Args: | ||
v: The value being validated. | ||
|
||
Returns: | ||
bool: True if the validation is successful, False otherwise. | ||
""" | ||
pass | ||
|
||
def _raise_error(self, cls: Any, v: T) -> None: | ||
"""Raise a validation error. | ||
|
||
Args: | ||
cls: The class being validated. | ||
v: The value being validated. | ||
|
||
Raises: | ||
ValidationError: Raised when the validation fails. | ||
""" | ||
logger.error(f'Validation failed for {v} in {cls.__name__}.') | ||
raise ValidationError( | ||
self.error_message, | ||
model=cls, | ||
) | ||
|
||
def validate(self, cls: Any, v: T) -> T: | ||
"""Validate the value. | ||
|
||
If the value is None, ie the fields is optional | ||
in the model, then it is returned as is without any validation. | ||
|
||
Args: | ||
cls: The class being validated. | ||
v: The value being validated. | ||
|
||
Returns: | ||
T: The validated value (if valid). | ||
|
||
Raises: | ||
ValidationError: If the value is not valid. | ||
""" | ||
if not v: | ||
return v | ||
elif not self.validation_logic(v): | ||
self._raise_error(cls, v) | ||
return v |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,52 +1,19 @@ | ||
"""API server entry point.""" | ||
"""App server entry point.""" | ||
|
||
import logging | ||
import os | ||
from pathlib import Path | ||
|
||
from connexion import FlaskApp | ||
from foca import Foca | ||
from tesk.tesk_app import TeskApp | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
def init_app() -> FlaskApp: | ||
"""Initialize and return the FOCA app. | ||
|
||
This function initializes the FOCA app by loading the configuration | ||
from the environment variable `TESK_FOCA_CONFIG_PATH` if set, or from | ||
the default path if not. It raises a `FileNotFoundError` if the | ||
configuration file is not found. | ||
|
||
Returns: | ||
FlaskApp: A Connexion application instance. | ||
|
||
Raises: | ||
FileNotFoundError: If the configuration file is not found. | ||
""" | ||
# Determine the configuration path | ||
config_path_env = os.getenv('TESK_FOCA_CONFIG_PATH') | ||
if config_path_env: | ||
config_path = Path(config_path_env).resolve() | ||
else: | ||
config_path = ( | ||
Path(__file__).parents[1] / 'deployment' / 'config.yaml' | ||
).resolve() | ||
|
||
# Check if the configuration file exists | ||
if not config_path.exists(): | ||
raise FileNotFoundError(f'Config file not found at: {config_path}') | ||
|
||
foca = Foca( | ||
config_file=config_path, | ||
) | ||
return foca.create_app() | ||
|
||
|
||
def main() -> None: | ||
"""Run FOCA application.""" | ||
app = init_app() | ||
app.run(port=app.port) | ||
try: | ||
TeskApp().run() | ||
except Exception: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hope we have tested backward compatibility here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nah :) |
||
logger.exception('An error occurred while running the application.') | ||
raise | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be raising appropriate runtime exception here. |
||
|
||
if __name__ == '__main__': | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, I would much prefer to put this in another PR that focuses on this specific change (together with the proposed changes in |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
"""Base class for the APP used in initialization of API.""" | ||
JaeAeich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
import logging | ||
import os | ||
from pathlib import Path | ||
from typing import Optional, final | ||
|
||
from foca import Foca | ||
from foca.config.config_parser import ConfigParser | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class TeskApp(Foca): | ||
JaeAeich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"""TESK API class extending the Foca framework.""" | ||
|
||
def __init__( | ||
self, | ||
config_file: Optional[Path] = None, | ||
custom_config_model: Optional[Path] = None, | ||
) -> None: | ||
"""Initialize the TeskApp class. | ||
|
||
Args: | ||
config_file (Optional[Path]): Path to the configuration file. | ||
Defaults to None. | ||
custom_config_model (Optional[Path]): Path to the custom | ||
configuration model file. Defaults to None. | ||
""" | ||
if not config_file: | ||
self._load_config_file() | ||
else: | ||
self.config_file = config_file | ||
if not custom_config_model: | ||
self.custom_config_model = self._load_custom_config_model() | ||
else: | ||
self.custom_config_model = custom_config_model | ||
self.conf = ConfigParser( | ||
config_file=self.config_file, | ||
custom_config_model=self.custom_config_model, | ||
format_logs=True, | ||
).config | ||
self._app = self.create_app() | ||
|
||
@final | ||
def run(self) -> None: | ||
"""Run the application.""" | ||
_environment = self.conf.server.environment or 'production' | ||
logger.info(f'Running application in {_environment} environment...') | ||
_debug = self.conf.server.debug or False | ||
self._app.run( | ||
host=self.conf.server.host, port=self.conf.server.port, debug=_debug | ||
) | ||
|
||
@final | ||
def _load_config_file(self) -> None: | ||
"""Load the configuration file path from env variable or default location. | ||
|
||
Raises: | ||
FileNotFoundError: If the configuration file is not found. | ||
""" | ||
logger.info('Loading configuration path...') | ||
if config_path_env := os.getenv('TESK_FOCA_CONFIG_FILE'): | ||
self.config_file = Path(config_path_env).resolve() | ||
else: | ||
self.config_file = ( | ||
Path(__file__).parents[1] / 'deployment' / 'config.yaml' | ||
).resolve() | ||
|
||
if not self.config_file.exists(): | ||
raise FileNotFoundError( | ||
f'Config file not found at: {self.config_file}', | ||
) | ||
|
||
@final | ||
def _load_custom_config_model(self) -> None: | ||
"""Load the custom configuration model path from environment variable or None. | ||
|
||
Raises: | ||
FileNotFoundError: If the custom configuration model is specified and found. | ||
""" | ||
logger.info('Loading custom configuration model path...') | ||
if custom_config_model_env := os.getenv('TESK_FOCA_CUSTOM_CONFIG_MODEL'): | ||
self.custom_config_model = Path(custom_config_model_env).resolve() | ||
else: | ||
self.custom_config_model = None | ||
|
||
if self.custom_config_model and not self.custom_config_model.exists(): | ||
raise FileNotFoundError( | ||
f'Custom configuration model not found at: {self.custom_config_model}', | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: Include in a
style:
PR