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

Feeds #16

Merged
merged 29 commits into from
Jan 4, 2018
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ae2ef0b
First pass at feeds implementation
danpalmer Dec 21, 2017
1bea78b
We need requests for the feeds
danpalmer Dec 27, 2017
e02f519
Merge branch 'master' into feeds
danpalmer Dec 28, 2017
76b7739
Merge branch 'master' into feeds
danpalmer Jan 3, 2018
ba6a996
Linter
danpalmer Jan 3, 2018
36bbabf
Move Context, it's now not just axout execution
danpalmer Jan 3, 2018
a3352a0
Update tests for new constructor args
danpalmer Jan 3, 2018
7079d5b
This interferes with other stuff, remove for now
danpalmer Jan 3, 2018
f83cf76
We need the label to template the URL
danpalmer Jan 3, 2018
8254e54
Fix circular imports for types
danpalmer Jan 3, 2018
ee634af
Update tests for new Context constructor
danpalmer Jan 3, 2018
58d2723
Test the feed access
danpalmer Jan 3, 2018
2e5398d
Ensure we only get the data once
danpalmer Jan 3, 2018
64ecec6
Linting
danpalmer Jan 3, 2018
87b710c
Add some types
danpalmer Jan 3, 2018
6526f06
Move mypy dependency into a file
danpalmer Jan 3, 2018
8881e33
Drop types for this for now
danpalmer Jan 3, 2018
29fb2af
Make these tests more realistic
danpalmer Jan 3, 2018
8b5b93a
Add feed config
danpalmer Jan 4, 2018
12aeee9
Test that feeds must have unique names
danpalmer Jan 4, 2018
f366c6c
Implement and test setup for feeds
danpalmer Jan 4, 2018
b9f1924
Call correctly
danpalmer Jan 4, 2018
62eeacf
Improve behaviour of this util
danpalmer Jan 4, 2018
fd73449
More context tests
danpalmer Jan 4, 2018
0446c06
Support state machine name in feed URL
danpalmer Jan 4, 2018
ae33115
Remove unnecessary f-string
danpalmer Jan 4, 2018
e485628
Types
danpalmer Jan 4, 2018
606b673
Rename for clarity
danpalmer Jan 4, 2018
473539d
Should fail loudly if the feed errors
danpalmer Jan 4, 2018
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 .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ jobs:
# Run tox, caching the .tox directory
- restore_cache:
name: Restore .tox cache
key: deps-tox-{{ .Branch }}-{{ checksum "scripts/linting/requirements.txt" }}-{{ checksum "scripts/testing/requirements.txt" }}-{{ checksum "setup.py" }}
key: deps-tox-{{ .Branch }}-{{ checksum "scripts/linting/requirements.txt" }}-{{ checksum "scripts/typechecking/requirements.txt" }}-{{ checksum "scripts/testing/requirements.txt" }}-{{ checksum "setup.py" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 to adding a typechecking requirements.txt; this change (and the similar one below) should probably be cherry-picked to master though.

- run:
name: Test/Lint/Typecheck
command: |
. venv/bin/activate
tox
- save_cache:
name: Save .tox cache
key: deps-tox-{{ .Branch }}-{{ checksum "scripts/linting/requirements.txt" }}-{{ checksum "scripts/testing/requirements.txt" }}-{{ checksum "setup.py" }}
key: deps-tox-{{ .Branch }}-{{ checksum "scripts/linting/requirements.txt" }}-{{ checksum "scripts/typechecking/requirements.txt" }}-{{ checksum "scripts/testing/requirements.txt" }}-{{ checksum "setup.py" }}
paths:
- ".tox"

Expand Down
2 changes: 2 additions & 0 deletions routemaster/config/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Loading of application configuration."""

from routemaster.config.model import (
Feed,
Gate,
State,
Action,
Expand All @@ -24,6 +25,7 @@
__all__ = (
'load_config',
'load_database_config',
'Feed',
'Gate',
'State',
'Action',
Expand Down
13 changes: 13 additions & 0 deletions routemaster/config/loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import jsonschema.exceptions

from routemaster.config.model import (
Feed,
Gate,
State,
Action,
Expand Down Expand Up @@ -106,15 +107,27 @@ def _load_state_machine(
name: str,
yaml_state_machine: Yaml,
) -> StateMachine:
feeds = [_load_feed(x) for x in yaml_state_machine.get('feeds', [])]

if len(set(x.name for x in feeds)) < len(feeds):
raise ConfigError(
f"Feeds must have unique names at {'.'.join(path + ['feeds'])}",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

For even nicer errors, we could use a collections.Counter to detect exactly which feeds had non-unique names. (I'm not sure how many feeds we're expecting to exist in each config file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd expect it will be typically 1, sometimes 1<n<5. I don't think this is necessary for now.


return StateMachine(
name=name,
states=[
_load_state(path + ['states', str(idx)], yaml_state)
for idx, yaml_state in enumerate(yaml_state_machine['states'])
],
feeds=feeds,
)


def _load_feed(yaml: Yaml) -> Feed:
return Feed(name=yaml['name'], url=yaml['url'])


def _load_state(path: Path, yaml_state: Yaml) -> State:
if 'action' in yaml_state and 'gate' in yaml_state: # pragma: no branch
raise ConfigError( # pragma: no cover
Expand Down
16 changes: 14 additions & 2 deletions routemaster/config/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
NamedTuple,
)

from routemaster.exit_conditions import Context, ExitConditionProgram
from routemaster.exit_conditions import ExitConditionProgram

if False: # typing
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale behind this being a fake import -- is it circular?

I'm ok with using the if False idiom if there are conflicts on the import or it's a major performance hit, but would rather just import things directly for simpler cases as it's a bit ugly (and also makes the type annotations a bit ugly too).

(future occurrences not commented)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the rationale behind this being a fake import -- is it circular?

Yes

This is only to support typing, and in Python 3.7 we can switch to 'if TYPING'.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, could we have a comment about these being circular? (possibly as well as the need for it only being for typing).

from routemaster.context import Context # noqa


class TimeTrigger(NamedTuple):
Expand Down Expand Up @@ -69,7 +72,7 @@ class ContextNextStates(NamedTuple):
path: str
destinations: Iterable[ContextNextStatesOption]

def next_state_for_label(self, label_context: Context) -> str:
def next_state_for_label(self, label_context: 'Context') -> str:
"""Returns next state based on context value at `self.path`."""
val = label_context.lookup(self.path.split('.'))
for destination in self.destinations:
Expand Down Expand Up @@ -133,10 +136,19 @@ class Action(NamedTuple):
State = Union[Action, Gate]


class Feed(NamedTuple):
"""
The definition of a feed of dynamic data to be included in a context.
"""
name: str
url: str


class StateMachine(NamedTuple):
"""A state machine."""
name: str
states: List[State]
feeds: List[Feed]

def get_state(self, state_name: str) -> State:
"""Get the state object for a given state name."""
Expand Down
15 changes: 15 additions & 0 deletions routemaster/config/schema.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,21 @@ properties:
title: State Machine
type: object
properties:
feeds:
title: Feeds
type: array
items:
title: Feed
type: object
properties:
name:
type: string
url:
type: string
required:
- name
- url
additionalProperties: false
states:
title: States
type: array
Expand Down
14 changes: 14 additions & 0 deletions routemaster/config/tests/test_loading.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import pytest

from routemaster.config import (
Feed,
Gate,
Action,
Config,
Expand Down Expand Up @@ -45,6 +46,7 @@ def test_trivial_config():
state_machines={
'example': StateMachine(
name='example',
feeds=[],
states=[
Gate(
name='start',
Expand Down Expand Up @@ -73,6 +75,9 @@ def test_realistic_config():
state_machines={
'example': StateMachine(
name='example',
feeds=[
Feed(name='data_feed', url='http://localhost/<label>'),
],
states=[
Gate(
name='start',
Expand Down Expand Up @@ -190,6 +195,7 @@ def test_next_states_shorthand_results_in_constant_config():
state_machines={
'example': StateMachine(
name='example',
feeds=[],
states=[
Gate(
name='start',
Expand Down Expand Up @@ -224,6 +230,9 @@ def test_environment_variables_override_config_file_for_database_config():
state_machines={
'example': StateMachine(
name='example',
feeds=[
Feed(name='data_feed', url='http://localhost/<label>'),
],
states=[
Gate(
name='start',
Expand Down Expand Up @@ -302,3 +311,8 @@ def test_raises_for_unparseable_database_port_in_environment_variable():
with mock.patch.dict(os.environ, {'DB_PORT': 'not an int'}):
with assert_config_error(f"Could not parse DB_PORT as an integer: 'not an int'."):
load_config(yaml_data('realistic'))


def test_multiple_feeds_same_name_invalid():
with assert_config_error(f"Feeds must have unique names at state_machines.example.feeds"):
Copy link
Contributor

Choose a reason for hiding this comment

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

This string doesn't have any format placeholders in -- consider dropping the f prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

load_config(yaml_data('multiple_feeds_same_name_invalid'))
6 changes: 3 additions & 3 deletions routemaster/config/tests/test_next_states.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
ContextNextStates,
ContextNextStatesOption,
)
from routemaster.exit_conditions import Context
from routemaster.context import Context

UTC_NOW = datetime.datetime.now(dateutil.tz.tzutc())

Expand All @@ -36,7 +36,7 @@ def test_context_next_states():
],
)

context = Context({'foo': True}, UTC_NOW, None)
context = Context('label1', {'foo': True}, UTC_NOW, None, [])

assert next_states.all_destinations() == ['1', '2']
assert next_states.next_state_for_label(context) == '1'
Expand All @@ -51,7 +51,7 @@ def test_context_next_states_raises_for_no_valid_state():
],
)

context = Context({'foo': 'bar'}, UTC_NOW, None)
context = Context('label1', {'foo': 'bar'}, UTC_NOW, None, [])

with pytest.raises(RuntimeError):
next_states.next_state_for_label(context)
1 change: 1 addition & 0 deletions routemaster/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
TEST_STATE_MACHINES = {
'test_machine': StateMachine(
name='test_machine',
feeds=[],
states=[
Gate(
name='start',
Expand Down
76 changes: 76 additions & 0 deletions routemaster/context.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
"""Context definition for exit condition programs."""
import datetime
from typing import Any, Dict, Iterable, Sequence

from routemaster.feeds import Feed
from routemaster.utils import get_path


class Context(object):
"""Execution context for exit condition programs."""

def __init__(
self,
label: str,
Copy link
Contributor

Choose a reason for hiding this comment

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

Elsewhere (in the API at least) we call this a label_name (with label meaning a fully qualified name plus state machine), should we use that terminology here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I'm not sure about this. A label should be just the string, I think we should probably change the Label in state_machine to something like LabelRef. Won't do this in this PR though.

metadata: Dict[str, Any],
now: datetime.datetime,
feeds: Dict[str, Feed],
accessed_variables: Iterable[str],
) -> None:
"""Create an execution context."""
if now.tzinfo is None:
raise ValueError(
"Cannot evaluate exit conditions with naive datetimes",
)

self.now = now
self.metadata = metadata
self.feeds = feeds

self._pre_warm_feeds(label, accessed_variables)

def lookup(self, path: Sequence[str]) -> Any:
"""Look up a path in the execution context."""
location, *rest = path

try:
return {
'metadata': self._lookup_metadata,
'feeds': self._lookup_feed_data,
}[location](rest)
except (KeyError, ValueError):
return None

def _lookup_metadata(self, path: Sequence[str]) -> Any:
return get_path(path, self.metadata)

def _lookup_feed_data(self, path: Sequence[str]) -> Any:
feed_name, *rest = path
return self.feeds[feed_name].lookup(rest)

def property_handler(self, property_name, value, **kwargs):
"""Handle a property in execution."""
if property_name == ('passed',):
epoch = kwargs['since']
return (self.now - epoch).total_seconds() >= value
if property_name == ('defined',):
return value is not None
if property_name == () and 'in' in kwargs:
return value in kwargs['in']
raise ValueError("Unknown property {name}".format(
name='.'.join(property_name)),
)

def _pre_warm_feeds(self, label, accessed_variables):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional to omit the type signature from this method? From what I recall, mypy doesn't check inside non-annotated methods by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for accessed_variable in accessed_variables:
parts = accessed_variable.split('.')

if len(parts) < 2:
continue

if parts[0] != 'feeds':
continue

feed = self.feeds.get(parts[1])
if feed is not None:
feed.fetch(label)
2 changes: 0 additions & 2 deletions routemaster/exit_conditions/__init__.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
"""Parsing and evaluation of exit condition programs."""

from routemaster.exit_conditions.context import Context
from routemaster.exit_conditions.program import ExitConditionProgram

__all__ = (
'Context',
'ExitConditionProgram',
)
42 changes: 0 additions & 42 deletions routemaster/exit_conditions/context.py

This file was deleted.

6 changes: 4 additions & 2 deletions routemaster/exit_conditions/program.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import typing

from routemaster.exit_conditions.parser import parse
from routemaster.exit_conditions.context import Context
from routemaster.exit_conditions.analysis import find_accessed_keys
from routemaster.exit_conditions.peephole import peephole_optimise
from routemaster.exit_conditions.evaluator import evaluate
Expand All @@ -12,6 +11,9 @@
format_parse_error_message,
)

if False: # typing
from routemaster.context import Context # noqa


class ExitConditionProgram(object):
"""Compiled exit condition program."""
Expand Down Expand Up @@ -39,7 +41,7 @@ def accessed_variables(self) -> typing.Iterable[str]:
for accessed_key in find_accessed_keys(self._instructions):
yield '.'.join(accessed_key)

def run(self, context: Context) -> bool:
def run(self, context: 'Context') -> bool:
"""Evaluate this program with a given context."""
return evaluate(
self._instructions,
Expand Down
Loading