-
Notifications
You must be signed in to change notification settings - Fork 7
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
Convert experiment not found log to a warning #2
Conversation
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.
thanks! just a couple notes
reddit_experiments/__init__.py
Outdated
@@ -23,6 +24,7 @@ | |||
|
|||
|
|||
logger = logging.getLogger(__name__) | |||
logging.captureWarnings(True) |
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.
This is done by baseplate-serve
: https://github.com/reddit/baseplate.py/blob/develop/baseplate/server/__init__.py#L114
Generally, we shouldn't be setting global logging configuration from libraries -- applications should be the ones doing that. (baseplate-serve being an application though it's in the codebase of a library!)
reddit_experiments/__init__.py
Outdated
@@ -132,7 +134,7 @@ def _get_experiment(self, name: str) -> Optional[Experiment]: | |||
self._cfg_data = self._get_config() | |||
|
|||
if name not in self._cfg_data: | |||
logger.info("Experiment <%r> not found in experiment config", name) | |||
warnings.warn(f"Experiment <{repr(name)}> not found in experiment config", RuntimeWarning) |
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 docs say RuntimeWarning
is "Base category for warnings about dubious runtime features." I think that sounds like it should be for interpreter-level stuff, not our use. It'd probably be fine to leave this un-specified (which defaults toUserWarning
) or subclass our own ExperimentsWarning if we really want to be fancy.
* holeeeeshititworks * Cargo fixes * added comment breadcrumb trail
This should alleviate the problem in #1, I believe.