-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Optional TOML configs support #2457
Conversation
Pygments doesn't supports TOML by default https://gitlab.com/gitlab-org/gitlab-pygments-rb/commit/d460ba68716937e59498f95fd34709d98990dea3
Pygments doesn't supports TOML by default https://gitlab.com/gitlab-org/gitlab-pygments-rb/commit/d460ba68716937e59498f95fd34709d98990dea3
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.
@orsinium , Thanks for your submission!
I like the idea here (enabling other styles of configuration formats), but I'm not yet sold on why we'd want other formats.
Do other configuration formats allow features that ConfigParser doesn't? If so, what are they and how are they applicable to luigi?
(I anticipated your clarification to my concern above and I've gone ahead and reviewed your code. Those comments and questions are below.)
doc/configuration.rst
Outdated
* ``/etc/luigi/luigi.toml`` | ||
* ``luigi.toml`` | ||
* ``luigi/base.toml`` | ||
* ``luigi/local.toml`` |
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.
Could you explain why base.toml
and local.toml
are proposed as valid configuration file names?
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.
Because implicit better than explicit. Now we can use explicit inheritance via luigi.cfg
and LUIGI_CONFIG_PATH=luigi_local.cfg
. Motivation of inheritance I describe in Pull Request: this is useful for configuring default settings, that can be overridden inlocal config.
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.
You mean that it uses local first and then base. Which makes sense so you can have local changes that don't conflict with the base config.
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.
I'm removing base and local configs. This need be discussed in future for better realization.
@@ -23,6 +41,17 @@ The config file is broken into sections, each controlling a different part of th | |||
[core] | |||
scheduler_host=luigi-host.mycompany.foo | |||
|
|||
Example toml config: | |||
|
|||
.. code:: python |
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.
why is python
used here for code syntax highlighting?
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.
Because Pygments doesn't have available lexerfor TOML. This is already contributed in some Pygments versions:
But Pygments which used for travis checks doesn't have this lexer yet. I choose python as alternative lexer because toml syntax very similar to hybrid of python and ini :)
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.
And a TODO then please. :)
streaming-jar = "/usr/lib/hadoop-xyz/hadoop-streaming-xyz-123.jar" | ||
|
||
[core] | ||
scheduler_host = "luigi-host.mycompany.foo" |
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.
are single quotes acceptable too?
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.
Yes, see TOML spec for more details about syntax
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.
Maybe link to the spec in the docs too?
@@ -0,0 +1,22 @@ | |||
# -*- coding: utf-8 -*- | |||
# | |||
# Copyright 2012-2015 Spotify AB |
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.
Shouldn't these dates include 2018. Probably not that important
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.
I just copied Apache license notice from original configuration.py. You can improve it, I don't now how it must be look in right way.
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.
Yea I think this is fine ...
if cls._instance is None: | ||
cls._instance = cls(*args, **kwargs) | ||
loaded = cls._instance.reload() | ||
logging.getLogger('luigi-interface').info('Loaded %r', loaded) |
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.
side note: am i correct that neither the client nor the server's logging config names are configurable? I've found that interesting - why statically set to luigi-interface
. Perhaps we don't want that changed in general.
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.
I don't change .instance()
logic, just moved it from cfg parser to base.
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.
Yeah, I was just raising an unrelated question - a topic for future possible PR (configurable logging names).
Nothing for you to do here.
return cls._instance | ||
|
||
@classmethod | ||
def add_config_path(cls, path): |
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.
Any reason for changing the order of these classmethods?
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.
What did you mean? I just moved this method from cfg parser to base without any changes.
luigi/configuration/base_parser.py
Outdated
|
||
class BaseParser: | ||
_instance = None | ||
_config_paths = [] |
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.
Shouldn't this be an abstract property that is required to be overwritten?
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.
Yeah, this can be confusable if somebody try to use different parsers in one project. I fix it. Thank you for review :)
luigi/configuration/toml_parser.py
Outdated
# limitations under the License. | ||
# | ||
import os | ||
import os.path |
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.
why import both os
and os.path
? import os.path
can be removed
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.
Nearly. import os
can be removed, not os.path
. Fixed. Thank you.
luigi/configuration/getter.py
Outdated
} | ||
|
||
|
||
def get_config(parser=os.environ.get('LUIGI_CONFIG_PARSER', 'cfg')): |
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.
Am i correct that if you want to use a config parser other than cfg
, you have to define the LUIGI_CONFIG_PARSER
environmental variable?
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.
Yes, you're right. I plan to contribute ability to set up config parser as argument for worker's run. But now Luigi supports config parser chainging only via env var. I write it into documentation.
setup.py
Outdated
@@ -39,6 +39,7 @@ def get_static_files(path): | |||
install_requires = [ | |||
'tornado>=4.0,<5', | |||
'python-daemon<3.0', | |||
'toml', |
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.
No version specification?
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.
Right, I think, we don't need specify library version. This library exposes an API familiar to users of the standard library json
, marshal
and pickle
, and authors will not change it in future.
Yes, TOML, opposite to ini, supports data types like integer, float, boolean, list, dict. Ini supports only strings. In future, I plan to contribute pyproject.toml support, when tools like pip and setuptools will migrate on it. Also you can read in this article other benefits of migration from ini to toml. Thank you for good question :) |
By the way, thank you for useful library, guys. Luigi is save many hours for our team :) |
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.
Looks like a nice contribution. How long have you used this in prod?
|
||
in increasing order of preference. The order only matters in case of key conflicts (see docs for ConfigParser.read_). These files are meant for both the client and ``luigid``. If you decide to specify your own configuration you should make sure that both the client and ``luigid`` load it properly. | ||
You can choose right parser via ``LUIGI_CONFIG_PARSER`` environment variable. For example, ``LUIGI_CONFIG_PARSER=toml``. |
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.
Can we instead require that the file must end in .toml
? It's very little magic for more convenience.
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.
I think about it, but this is very explicit while we have many possible places for configs. What if we have luigi.cfg
and luigi.toml
? We can't combine it into one config.
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.
I think we can add an improvement down the road to define a hierarchical level of config extensions. I.e. (.cfg, .toml, .yml, <etc.>). I'm fine with requiring an ENV variable for now
doc/configuration.rst
Outdated
* ``/etc/luigi/luigi.toml`` | ||
* ``luigi.toml`` | ||
* ``luigi/base.toml`` | ||
* ``luigi/local.toml`` |
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.
You mean that it uses local first and then base. Which makes sense so you can have local changes that don't conflict with the base config.
@@ -23,6 +41,17 @@ The config file is broken into sections, each controlling a different part of th | |||
[core] | |||
scheduler_host=luigi-host.mycompany.foo | |||
|
|||
Example toml config: | |||
|
|||
.. code:: python |
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.
And a TODO then please. :)
streaming-jar = "/usr/lib/hadoop-xyz/hadoop-streaming-xyz-123.jar" | ||
|
||
[core] | ||
scheduler_host = "luigi-host.mycompany.foo" |
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.
Maybe link to the spec in the docs too?
@@ -0,0 +1,22 @@ | |||
# -*- coding: utf-8 -*- | |||
# | |||
# Copyright 2012-2015 Spotify AB |
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.
Yea I think this is fine ...
luigi/configuration/base_parser.py
Outdated
import logging | ||
|
||
|
||
class BaseParser: |
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.
inherit from object?
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.
No-no-no, don't do it! In Python2 cfg parser fails if BaseParser inherit from object. This is strange, yes. I will add comment for it
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.
Added
@@ -38,8 +37,10 @@ | |||
except ImportError: | |||
from configparser import ConfigParser, NoOptionError, NoSectionError | |||
|
|||
from .base_parser import BaseParser |
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.
I recall relative imports are discouraged, no?
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.
Why? We already have from .tools import range
in project, and it's works. Relative imports little faster and better recognized some tools like isort.
setup.py
Outdated
@@ -39,6 +39,7 @@ def get_static_files(path): | |||
install_requires = [ | |||
'tornado>=4.0,<5', | |||
'python-daemon<3.0', | |||
'toml', |
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.
Can this be excluded? Eventually we want to populate this with dependencies needed but it's important that you can run luigi without toml installed.
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.
Fixed
luigi/configuration/getter.py
Outdated
@@ -0,0 +1,34 @@ | |||
# -*- coding: utf-8 -*- |
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.
Is there a better name than getter.py
?
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.
fixed
Good question, thank you :) I used it only one day on the dev. I make this feature for our team and send this Pull Request immediately. I can give here feedback after 1-2 weeks of active usage. I think, this is a good idea, thank you. |
Improved some things:
Also you can run example project as proof-of-concept. For example, I am getting |
|
Hey @orsinium. Thanks for providing a new summary of updates. I'll try to take another look this work week! Sorry for the delay! (and yeah, don't worry so much about codecov) |
I'm fine. Codecov good and fair again :) |
Today I'll try to run our new pipeline with this improvements. I'll give you feedback soon. |
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.
Let us know the results of your pipeline execution as it relates to this PR :)
doc/configuration.rst
Outdated
* ``luigi.toml`` | ||
* ``LUIGI_CONFIG_PATH`` environment variable | ||
|
||
Both config lists reversordered by priotity (from low to high). The order only matters in case of key conflicts (see docs for ConfigParser.read_). These files are meant for both the client and ``luigid``. If you decide to specify your own configuration you should make sure that both the client and ``luigid`` load it properly. |
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.
reversordered
is not a word. Perhaps we reword something to the effect of the below:
Both config lists increase in priority (from low to high).
luigi/configuration/base_parser.py
Outdated
|
||
# IMPORTANT: don't inherit from `object`! | ||
# ConfigParser have some troubles in this case. | ||
class BaseParser: |
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.
The issue is explained better on SO
if cls._instance is None: | ||
cls._instance = cls(*args, **kwargs) | ||
loaded = cls._instance.reload() | ||
logging.getLogger('luigi-interface').info('Loaded %r', loaded) |
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.
Yeah, I was just raising an unrelated question - a topic for future possible PR (configurable logging names).
Nothing for you to do here.
All is fine, our pipeline works right with new configs. I'm planning to commit dlstadther's improvements tomorrow. Future improvements for other pull requests: 1. Local configsI want to have file for local settings for storing some config in env 2. Configure logging via main configConfig example: [logging]
version = 1
disable_existing_loggers = true
[logging.formatters.simple]
format = "{levelname:8} {asctime} {module}:{lineno} {message}"
style = "{"
datefmt = "%Y-%m-%d %H:%M:%S"
[logging.handlers.console]
level = "DEBUG"
class = "logging.StreamHandler"
formatter = "simple"
[logging.loggers.parser]
handlers = ["console"]
level = "DEBUG"
disabled = false
propagate = false How we use it in our projects: import os.path
import logging
from logging.config import dictConfig
import toml
path = os.path.dirname(os.path.abspath(__file__))
CONFIG = toml.load(os.path.join(path, 'config.toml'))
dictConfig(CONFIG['logging'])
logger = logging.getLogger('parser') |
Added all actual @dlstadther 's improvements and merged spotify/master |
@ulzha, sorry for mentioning. This is from my branch actualization |
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.
Just a couple comments and clarifying questions
|
||
in increasing order of preference. The order only matters in case of key conflicts (see docs for ConfigParser.read_). These files are meant for both the client and ``luigid``. If you decide to specify your own configuration you should make sure that both the client and ``luigid`` load it properly. | ||
You can choose right parser via ``LUIGI_CONFIG_PARSER`` environment variable. For example, ``LUIGI_CONFIG_PARSER=toml``. |
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.
I think we can add an improvement down the road to define a hierarchical level of config extensions. I.e. (.cfg, .toml, .yml, <etc.>). I'm fine with requiring an ENV variable for now
@@ -486,7 +486,7 @@ def _get_s3_config(self, key=None): | |||
defaults = dict(configuration.get_config().defaults()) | |||
try: | |||
config = dict(configuration.get_config().items('s3')) | |||
except NoSectionError: | |||
except (NoSectionError, KeyError): |
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.
Am i correct to assume that ConfigParser returns the NoSectionError
and toml returns the KeyError
when a config section heading is missing?
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.
Yes. I think, this is incorrect raise from toml parser any exceptions from ConfigParser. Maybe, we can make custom common exception:
class SectionError(NoSectionError, KeyError):
pass
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.
I'm fine with it as it is currently written; I was just verifying that they raised different errors for the same problem.
setup.py
Outdated
@@ -75,6 +76,9 @@ def get_static_files(path): | |||
] | |||
}, | |||
install_requires=install_requires, | |||
extras_require={ | |||
'toml': ['toml'], |
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.
What are your thoughts on limiting this to the current milestone version? i.e. 'toml<1.0'
(since the current is 0.9.4) I just don't want version 1.0.0 to break this. I'd rather tread with caution, even with an optional configurable feature.
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.
Python toml package version related to toml spec version. TOML spec v1.0 will be backward compatible. Some quotes:
1.0.0 will be the first version to support TOML 1.0.0. TOML still has yet to hit TOML 1.0.0 so its release is postponed indefinitely.
As of version 0.5.0, TOML should be considered extremely stable. The goal is for version 1.0.0 to be backwards compatible (as much as humanly possible) with version 0.5.0. All implementations are strongly encouraged to become 0.5.0 compatible so that the transition to 1.0.0 will be simple when that happens.
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.
In that case, why don't we put 'toml<2.0'
?
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.
Ok. This don't break anything. This fix added. Thank you for review :)
tox.ini
Outdated
@@ -35,6 +35,7 @@ deps= | |||
hypothesis[datetime] | |||
selenium==3.0.2 | |||
pymongo==3.4.0 | |||
toml |
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.
If we agree to a version requirement in setup.py
, it'll need to be reflected here.
I think, this PR is ready to merge. What do you think? :) |
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.
I'm good with this now
Good then! Let's see if this gets well used by othres. 🙌 |
Thank you! We've done great work :) |
See the added docs for usage.
See the added docs for usage.
* upstream-master: (82 commits) S3 client refactor (spotify#2482) Rename to rpc_log_retries, and make it apply to all the logging involved Factor log_exceptions into a configuration parameter Fix attribute forwarding for tasks with dynamic dependencies (spotify#2478) Add a visiblity level for luigi.Parameters (spotify#2278) Add support for multiple requires and inherits arguments (spotify#2475) Add metadata columns to the RDBMS contrib (spotify#2440) Fix race condition in luigi.lock.acquire_for (spotify#2357) (spotify#2477) tests: Use RunOnceTask where possible (spotify#2476) Optional TOML configs support (spotify#2457) Added default port behaviour for Redshift (spotify#2474) Add codeowners file with default and specific example (spotify#2465) Add Data Revenue to the `blogged` list (spotify#2472) Fix Scheduler.add_task to overwrite accepts_messages attribute. (spotify#2469) Use task_id comparison in Task.__eq__. (spotify#2462) Add stale config Move github templates to .github dir Fix transfer config import (spotify#2458) Additions to provide support for the Load Sharing Facility (LSF) job scheduler (spotify#2373) Version 2.7.6 ...
* upstream-master: S3 client refactor (spotify#2482) Rename to rpc_log_retries, and make it apply to all the logging involved Factor log_exceptions into a configuration parameter Fix attribute forwarding for tasks with dynamic dependencies (spotify#2478) Add a visiblity level for luigi.Parameters (spotify#2278) Add support for multiple requires and inherits arguments (spotify#2475) Add metadata columns to the RDBMS contrib (spotify#2440) Fix race condition in luigi.lock.acquire_for (spotify#2357) (spotify#2477) tests: Use RunOnceTask where possible (spotify#2476) Optional TOML configs support (spotify#2457) Added default port behaviour for Redshift (spotify#2474) Add codeowners file with default and specific example (spotify#2465) Add Data Revenue to the `blogged` list (spotify#2472)
* upstream-master: Remove long-deprecated scheduler config variable alternatives (spotify#2491) Bump tornado milestone version (spotify#2490) Update moto to 1.x milestone version (spotify#2471) Use passed password when create a redis connection (spotify#2489) S3 client refactor (spotify#2482) Rename to rpc_log_retries, and make it apply to all the logging involved Factor log_exceptions into a configuration parameter Fix attribute forwarding for tasks with dynamic dependencies (spotify#2478) Add a visiblity level for luigi.Parameters (spotify#2278) Add support for multiple requires and inherits arguments (spotify#2475) Add metadata columns to the RDBMS contrib (spotify#2440) Fix race condition in luigi.lock.acquire_for (spotify#2357) (spotify#2477) tests: Use RunOnceTask where possible (spotify#2476) Optional TOML configs support (spotify#2457) Added default port behaviour for Redshift (spotify#2474) Add codeowners file with default and specific example (spotify#2465) Add Data Revenue to the `blogged` list (spotify#2472) Fix Scheduler.add_task to overwrite accepts_messages attribute. (spotify#2469) Use task_id comparison in Task.__eq__. (spotify#2462)
try: | ||
import toml | ||
except ImportError: | ||
toml = False |
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.
@orsinium
This will end up following error if you have toml
library not installed...
Works great after toml
installation!
File "/Users/*****/.pyenv/versions/miniconda-latest/envs/ddhr-rjb/lib/python3.6/site-packages/luigi/configuration/toml_parser.py", line 53, in read
self.data = self._update_data(self.data, toml.load(path))
AttributeError: 'bool' object has no attribute 'load'
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.
Oh noes, @orsinium can you fix this asap?
@adamist521, please, can you provide full traceback? |
@Tarrasch, don't panic. This is unusual usage or something like this, because get_config function checks reader's Hmmmm... I need to add info about toml support in installation section :) |
This PR effectively fails |
Yes. Please, import NoSectionError from configparser from stdlib:
|
Description
LUIGI_CONFIG_PARSER
env var.Motivation and Context
Have you tested this? If so, how?