main loop: fix log_main_loop and add log_db plugin #4674
Conversation
95df2b0 to
090d7a5
Compare
|
Add a miscellaneous fix for the conda-build workflow which appears to have broken due to it now detecting the conda activate script itself? |
|
Failed test |
| @@ -18,19 +19,27 @@ jobs: | |||
|
|
|||
There was a problem hiding this comment.
May want to use actions/setup-python?
There was a problem hiding this comment.
Don't think so, this action doesn't use Python?
0ee91d2 to
d3a326b
Compare
|
Closing and reopening to get the tests to pick up the changes to master |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Looks good but:
- I'm not clear on what the bug actually was
- Just noticed a small mistake with the logging
diff --git a/cylc/flow/main_loop/__init__.py b/cylc/flow/main_loop/__init__.py index 4f77c2fde..83da68dc1 100644 --- a/cylc/flow/main_loop/__init__.py +++ b/cylc/flow/main_loop/__init__.py @@ -350,9 +350,9 @@ def load(config, additional_plugins=None): )[(plugin_name, coro_name)] = coro plugins['timings'][(plugin_name, coro_name)] = deque(maxlen=1) LOG.debug( - 'Loaded main loop plugin "%s": %s', - plugin_name + '\n', - '\n'.join((f'* {x}' for x in log)) + 'Loaded main loop plugin "%s":\n%s', + plugin_name, + '\n'.join(f'* {x}' for x in log) ) # set the initial state of the plugin plugins['state'][plugin_name] = {}
* Plugin had become broken with changes to the way timings are recorded.
* A new main loop plugin for development purposes that logs database operations into a .sql file. * Includes a test for the development main loop plugins to ensure they can run to the point of producing output.
d3a326b to
91e7c97
Compare
(the root cause was a change in the way timings were stored)
Applied. |
I think it was this one. Needed to inspect the DB calls in order to confirm the changes were doing the right thing. |
There was a problem hiding this comment.
Nothing here which should block a merge (hence the approval), but a few comments made about spelling & semi-unrelated lack of docstrings.
I also noticed that if you run this for a while the cost of the mainloop plugins increases
Is there any housekeeping on this?
I'm not too worried if there isn't because at a rate of increase in the order of 1kb/s ~= 84mb/day which is OK if you aren't running really long running tests.
| if data: | ||
| data = _normalise(data) | ||
| _dump(data, scheduler.workflow_run_dir) | ||
| _plot(data, scheduler.workflow_run_dir) | ||
|
|
||
|
|
||
| def _normalise(data): | ||
| earliest_time = min(( |
There was a problem hiding this comment.
Would I be unreasonable in asking for docstrings for these functions?
There was a problem hiding this comment.
_normalisenormalises the data._dumpjson.dumpsthe data._plotplots the data.
There was a problem hiding this comment.
Is the meaning of "normalise" supposed to be intuitively obvious, without reading the code?
There was a problem hiding this comment.
I see the unit test does have a docstring 😁
def test_normalise(test_data):
"""Ensure we correctly normalise the timings against the earliest time."""
That is because the mainloop plugins data is holding the information that was used to generate the graph. Normally the timing data doesn't accumulate, but if you want to plot it it has to. |
Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
|
Merging with two approvals. |
Impacts developer plugins not loaded in normal circumstances so a reasonably acceptable thing to sneak into an RC. Otherwise push to 8.1.
(Fixes a bug but not a user-facing one)
Contribute a patch developed during the review of #4581 for logging database operations. This helps keep track of:
Fix an error in the
log_main_loopplugin discovered on-route and add a test so it doesn't happen again.Requirements check-list
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfgandconda-environment.yml.