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: add service info endpoint #5

Open
wants to merge 2 commits into
base: base-sha/9e84a5b0225fccc065cb01c811b8b8bd1c655a07
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
28 changes: 28 additions & 0 deletions deployment/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,34 @@ api:
swagger_ui: True
serve_spec: True

# Custom configuration
# Cf. https://foca.readthedocs.io/en/latest/modules/foca.models.html#foca.models.config.CustomConfig
custom:
# ServiceInfo configuration based on ServiceInfo model
service_info:
id: org.ga4gh.myservice
name: My project
type:
group: org.ga4gh
artifact: tes
version: 1.0.0
description: This service provides...
organization:
name: My organization
url: https://example.com
contactUrl: mailto:[email protected]

Choose a reason for hiding this comment

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

suggestion: Consider using a more generic contact URL

Using a mailto link directly in the configuration might limit flexibility. Consider using a more generic contact URL that can be updated without changing the configuration.

Suggested change
contactUrl: mailto:support@example.com
contactUrl: https://example.com/contact

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

What type of LLM test could this comment become?

  • 👍 - this comment is really good/important and we should always make it
  • 👎 - this comment is really bad and we should never make it
  • no reaction - don't turn this comment into an LLM test

documentationUrl: https://docs.myservice.example.com
createdAt: '2019-06-04T12:58:19Z'
updatedAt: '2019-06-04T12:58:19Z'
environment: test
version: 1.0.0
storage:
- file:///path/to/local/funnel-storage
- s3://ohsu-compbio-funnel/storage
tesResources_backend_parameters:
- VmSize
- ParamToRecogniseDataComingFromConfig

# Logging configuration
# Cf. https://foca.readthedocs.io/en/latest/modules/foca.models.html#foca.models.config.LogConfig
log:
Expand Down
14 changes: 6 additions & 8 deletions tesk/api/ga4gh/tes/controllers.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
# from connexion import request # type: ignore
from foca.utils.logging import log_traffic # type: ignore

from tesk.api.ga4gh.tes.service_info.service_info import ServiceInfo

# Get logger instance
logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -36,14 +38,10 @@ def CreateTask(*args, **kwargs) -> dict: # type: ignore

# GET /tasks/service-info
@log_traffic
def GetServiceInfo(*args, **kwargs) -> dict: # type: ignore
"""Get service info.

Args:
*args: Variable length argument list.
**kwargs: Arbitrary keyword arguments.
"""
pass
def GetServiceInfo() -> dict: # type: ignore

Choose a reason for hiding this comment

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

suggestion: Consider handling potential exceptions

The GetServiceInfo function does not handle any potential exceptions that might be raised by ServiceInfo().response(). Consider adding exception handling to provide more robust error handling.

Suggested change
def GetServiceInfo() -> dict: # type: ignore
def GetServiceInfo() -> dict: # type: ignore
try:
service_info = ServiceInfo()
except Exception as e:
return {"error": str(e)}

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

What type of LLM test could this comment become?

  • 👍 - this comment is really good/important and we should always make it
  • 👎 - this comment is really bad and we should never make it
  • no reaction - don't turn this comment into an LLM test

"""Get service info."""
service_info = ServiceInfo()
return service_info.response()


# GET /tasks
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 @@ -353,7 +353,7 @@ class Service(BaseModel):
organization: Organization = Field(
..., description="Organization providing the service"
)
contactUrl: Optional[AnyUrl] = Field(
contactUrl: Optional[str] = Field(

Choose a reason for hiding this comment

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

issue (bug_risk): Revert type change for contactUrl

Changing the type of contactUrl from AnyUrl to str might lead to invalid URLs being accepted. Consider reverting this change to ensure URL validation.

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

What type of LLM test could this comment become?

  • 👍 - this comment is really good/important and we should always make it
  • 👎 - this comment is really bad and we should never make it
  • no reaction - don't turn this comment into an LLM test

default=None,
description="URL of the contact for the provider of this service, e.g. a link to a "
"contact form (RFC 3986 format), or an email (RFC 2368 format).",
Expand Down
1 change: 1 addition & 0 deletions tesk/api/ga4gh/tes/service_info/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
"""Package for the TES Service Info endpoint."""
87 changes: 87 additions & 0 deletions tesk/api/ga4gh/tes/service_info/service_info.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
"""Service info for TES API."""

import json
from typing import Any, Optional

from pydantic import AnyUrl

from tesk.api.ga4gh.tes.models import (
Artifact,
Organization,
TesServiceInfo,
TesServiceType,
)
from tesk.exceptions import ConfigNotFoundError
from tesk.tesk_app import TeskApp


class ServiceInfo:
"""Service info for TES API."""

def __init__(self) -> None:
"""Initializes the BaseServiceInfoRequest class."""
self.service_info: Optional[TesServiceInfo] = None
self.config = TeskApp().conf

Choose a reason for hiding this comment

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

suggestion: Consider dependency injection for configuration

Directly instantiating TeskApp within the ServiceInfo class makes it harder to test and less flexible. Consider using dependency injection to pass the configuration.

Suggested change
self.config = TeskApp().conf
class BaseServiceInfoRequest:
def __init__(self, config=None) -> None:
"""Initializes the BaseServiceInfoRequest class."""
self.service_info: Optional[TesServiceInfo] = None
self.config = config if config is not None else TeskApp().conf

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

What type of LLM test could this comment become?

  • 👍 - this comment is really good/important and we should always make it
  • 👎 - this comment is really bad and we should never make it
  • no reaction - don't turn this comment into an LLM test


def get_default_service_info(self) -> TesServiceInfo:
"""Get the default service info.

if service info is not provided in the config,
this method will return the default service info.

Returns:
TesServiceInfo: Default service info.
"""
return TesServiceInfo(
id="org.ga4gh.tes",
name="TES",
type=TesServiceType(
group="org.ga4gh",
artifact=Artifact.tes,
version="1.1.0",
),
organization=Organization(
name="my_organization",
url=AnyUrl("https://example.com"),
),
version="1.1.0",
)

def get_service_info_from_config(self) -> TesServiceInfo:
"""Returns service info from config.

Returns:
TesServiceInfo: Service info from config.

Raises:
ConfigNotFoundError: If custom config or part of it is not found
in the config file.
"""
if not self.config.custom:

Choose a reason for hiding this comment

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

suggestion (bug_risk): Check for None explicitly

Using if not self.config.custom: can be ambiguous if self.config.custom can be an empty dictionary. Consider explicitly checking for None.

Suggested change
if not self.config.custom:
if self.config.custom is None:

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

What type of LLM test could this comment become?

  • 👍 - this comment is really good/important and we should always make it
  • 👎 - this comment is really bad and we should never make it
  • no reaction - don't turn this comment into an LLM test

raise ConfigNotFoundError("Custom configuration not found.")

service_info_data = self.config.custom.get("service_info")
if not service_info_data:
raise ConfigNotFoundError("Service info not found in custom configuration.")

try:
service_info = TesServiceInfo(**service_info_data)
except TypeError as e:

Choose a reason for hiding this comment

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

suggestion: Consider logging the exception

Logging the exception can provide more context when debugging issues related to invalid service info format.

Suggested change
except TypeError as e:
except TypeError as e:
logging.error(f"Invalid service info format: {e}")

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

What type of LLM test could this comment become?

  • 👍 - this comment is really good/important and we should always make it
  • 👎 - this comment is really bad and we should never make it
  • no reaction - don't turn this comment into an LLM test

raise ConfigNotFoundError(f"Invalid service info format: {e}") from None

return service_info

def response(self) -> dict[str, Any]:
"""Returns serialized response for /service-info.

Returns:
dict: Serialized response.
"""
if self.service_info is None:
try:
self.service_info = self.get_service_info_from_config()
except ConfigNotFoundError:
self.service_info = self.get_default_service_info()

# return self.service_info.dict()
return dict(json.loads(self.service_info.json()))

Choose a reason for hiding this comment

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

suggestion: Simplify JSON serialization

The line return dict(json.loads(self.service_info.json())) can be simplified to return self.service_info.dict() to avoid unnecessary JSON serialization and deserialization.

Suggested change
return dict(json.loads(self.service_info.json()))
return self.service_info.dict()

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

What type of LLM test could this comment become?

  • 👍 - this comment is really good/important and we should always make it
  • 👎 - this comment is really bad and we should never make it
  • no reaction - don't turn this comment into an LLM test