Extend plugin_executor to catch warnings during msg construction and …

[robertplatt-mo]

…append the plugin filepath to the warning message.

[robertplatt-mo]

Carwyn Pelley (@cpelley) In order to get the CI to run I had to make changes to how the code is checked out for forks. I'm not expecting this to be merged I just wanted the CI to run. The change to the docs could feasibly me merged as it just ensures that when we push from a fork we skip the task that pushes the documentation.

I know I need to add my name to contributors but I figured I'd let you feedback first.

[Carwyn Pelley (cpelley)]

Apologies for this. I updated the repository to require forks without consideration of the its impact to the actions.
Let's go with your changes to the workflow for now -- I'll put on my todo to update the actions to properly work with forks 👍

[robertplatt-mo]

Hi. Sorry I've been on leave. I'm actually not sure what the request is here. Do you mean we should keep my changes to workflows/tests.yml for now? If so what are next steps?

[Carwyn Pelley (cpelley)]

I don't think this branch is currently doing what you think it is doing since the context manager isn't around the actual execution. It could do with a more integration test that demonstrates it working.

[Carwyn Pelley (cpelley)]

This context manager isn't around the execution of the application itself (only the definition of a string) so isn't actually doing anything currently.

[Carwyn Pelley (cpelley)]

Following precedence of the extra context we provide to exception messages raised by applications executed, we should provide similar context to the conditions which led to warning -- object name (application) and the parameters we provided it. This helps users determine the circumstances that resulted in this warning.

[Carwyn Pelley (cpelley)]

Should likely not live inside this same module.

[Carwyn Pelley (cpelley)]

I have addressed issues with this workflow file for running from forks now.

[Carwyn Pelley (cpelley)]

Some/all of these will be picked up as tests in the pytest test discover due to naming. Use of 'Dummy' or similar would avoid this.

[Carwyn Pelley (cpelley)]

robertplatt-mo, Joshua Wiggs (@JoshuaWiggs) — thanks for the work on this. I don’t currently see how the PR, in its present form, is able to function as intended (see comments above).
I’m also not yet clear on how this fits into DAGrunner’s overall logging and monitoring approach. While tagging warning messages with the application path does seem useful, I would have expected, consistent with the previous exception-handling behaviour, that the arguments passed to the science application which triggered the warning would also be included, for example, see

dagrunner/dagrunner/execute_graph.py

Lines 208 to 214 in 62380e3

	except Exception as err:
	msg = (
	f"Failed to execute {obj_name} with {args}, {callable_kwargs}"
	f"\nnode_properties: {node_properties}"
	f"\nnode_id: {node_id}"
	)
	raise RuntimeError(msg) from err

Additionally, this change likely needs some consideration in the broader context of DAGrunner’s logging and monitoring framework.
As there is currently no context provided in the PR description, my understanding is that the aim is to make it easier to identify which improver/epp application was running when underlying libraries (e.g. Iris or NumPy) emit a warning in order to better differentiate between known warnings and unknown warnings. If that is the case, it may be worth considering whether a straightforward change in epp itself would better address this need in the short term. I’m very happy to discuss ideas or suggestions, but I think some further thought is needed on how best to integrate this kind of behaviour into DAGrunner.