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

Enable extension to other carbon intensity forecast providers #37

Merged
merged 4 commits into from
May 26, 2023

Conversation

tlestang
Copy link
Collaborator

It is currently assumed that carbon intensity data comes from carbonintensity.org.uk. While it is likely to remain the case for a time (until we find data sources for other countries, see #22), it didn't seem a lot of effort -- and changes -- to abstract away the API-specific bits, namely:

  • Getting a URL from a postcode and job runtime duration.
  • Parsing the api response into a format usable by cats -- currently a list of tuples.

This PR add two new arguments request_url_fun and parse_response_fun to the parsedata.get_tuple:

def get_tuple(
    postcode: str,
    request_url_fun: Callable[[datetime, str], str],
    parse_response_fun: Callable[[dict], list[tuple[datetime, int]]],
) -> list[list[tuple[datetime, int]]]:
    """
    get carbon intensity from a web API
    # ...
   """

These two arguments are functions and are specified at runtime.

The list of currently supported API interfaces (i.e. the list of avilable implementations of both functions) is provided by a module api_interface. This module makes available a dictionary of APIInterface instances, which are nothing but namedtuple instances specifying both functions for a given web service.

from cats.apt_interface import API_interfaces

api_interface = API_interfaces["carbonintensity.org.uk"]
tuples = get_tuple(
    postcode,
    api_interface.get_request_url,
    api_interface.parse_reponse_data,
)

Isolating the API-specific logic will also help should the carbonintensity.org.uk change its interface in the future. For instance the endpoint syntax or response structure.

Copy link
Collaborator

@andreww andreww left a comment

Choose a reason for hiding this comment

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

This looks like a really nice refactor / clean up / preparation for future extension. Let's merge this.

@@ -7,15 +7,20 @@
import sys

from .timeseries_conversion import cat_converter # noqa: F401
from .api_query import get_tuple # noqa: F401
from .api_query import get_tuple
Copy link
Collaborator

Choose a reason for hiding this comment

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

At some point I expect we'll want to rename get_tuple to something else. But okay for now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed - I've been trying to resist the urge of making too many changes at once.

@tlestang tlestang merged commit cc17c1b into main May 26, 2023
3 checks passed
@tlestang tlestang deleted the api_module branch May 26, 2023 21:28
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.

None yet

2 participants