Closed awegsche closed 1 year ago
Regarding this point:
change "Retrieved 1 values for {key}" to "{key} = {value}", possibly handling the NaN at this point in an elegant way
If you pay attention to the logs, this is not from us. It is pytimber
that decided to log it at info
level and there is nothing we can do about it (apart from maybe asking them to change that). The retrieved values are also logged, see screenshot below:
Regarding the warnings
, I agree that they could be moved to a lower level. Here's a suggestion: we log these as debug
and issue a single info
level line at the end of the function stating which knobs are affected, if any.
Could be something along the lines of:
def _extract(ldb, knobs_dict: KnobsDict, knob_categories: Sequence[str], time: datetime) -> KnobsDict:
"""
Main function to gather data from the state-tracker.
Args:
ldb (pytimber.LoggingDB): The pytimber database.
knobs_dict (KnobsDict): A mapping of all knob-names to KnobEntries.
knob_categories (Sequence[str]): Knob Categories or Knob-Names to extract.
time (datetime): The time, when to extract.
Returns:
Dict[str, KnobsDict]: Contains all the extracted knobs, grouped by categories.
When extraction was not possible, the value attribute of the respective KnobEntry is still None
"""
LOGGER.info(f"---- EXTRACTING KNOBS @ {time} ----")
knobs_returned_nan_or_infinite = []
knobs = {}
for category in knob_categories:
for knob in KNOB_CATEGORIES.get(category, [category]):
try:
knobs[knob] = knobs_dict[knob]
except KeyError as e:
raise KeyError(f"Knob '{knob}' not found in the knob-definitions!") from e
# LOGGER.debug(f"Looking for {knob:<34s} ") # pytimber logs this to info anyway
knobkey = f"LhcStateTracker:{knob}:target"
knobvalue = ldb.get(knobkey, time.timestamp()) # use timestamp to preserve timezone info
if knobkey not in knobvalue:
knobs_returned_nan_or_infinite.append(knob)
LOGGER.debug(f"No value for {knob} found")
continue
timestamps, values = knobvalue[knobkey]
if len(values) == 0:
knobs_returned_nan_or_infinite.append(knob)
LOGGER.debug(f"No value for {knob} found")
continue
value = values[-1]
if not math.isfinite(value):
knobs_returned_nan_or_infinite.append(knob)
LOGGER.debug(f"Value for {knob} is not a number or infinite")
continue
LOGGER.info(f"Knob value for {knob} extracted: {value} (unscaled)")
knobs[knob].value = value
if len(knobs_returned_nan_or_infinite):
logger.info(f"The following knobs were not returned a value, or were returned a NaN/infinite value: {', '.join(knobs_returned_nan_or_infinite)})
return knobs
Slightly related: this change could be a good opportunity to fix the module docstring and specify that the functionality requires being in the GPN.
Ha! Quick check and found something:
pytimber.LoggingDB(loglevel=logging.ERROR)
should solve the logging from pytimber issue.
(the fact that a class called LoggingDB has a parameter loglevel, that is unrelated to the logging of the DB is ... very intuitive)
I implemented the suggestions
I'm not sure if we need to log it at all (apart from the debug messages), if we don't care anyway. But either way is fine for me.
Update module docstring, bump patch version and merge?
The coverage went down to 40% :unamused:
fixed by #398
Feature Description
In its current state the logging of the knob_extractor is overly verbose and unhelpful
The following points should improve:
Possible Implementation
"Retrieved 1 values for {key}"
to"{key} = {value}"
, possibly handling the NaN at this point in an elegant way