Allow DAGrunner to propagate an IGNORE_EVENT with only one input argument

[ryan-cocking-mo]

Parent ticket
Child ticket
epp_workflows PR that requires these changes

Allow DAGrunner to propagate an IGNORE_EVENT with only one input argument

Previously: Allow DAGrunner to handle an empty set of arguments passed to Load / EPPT-3522 FSI fails without monow

These changes are part of a fix to the EPP fire severity index (FSI) workflow to produce outputs when MONOW input data is unavailable.

• Allow nodes with only one input to return an ignore event. This allows chains of nodes with single inputs to be correctly ignored.
  • For example, before this change, when a standardise immediately followed a load in the FSI workflow, if the load node (51) was ignored, the standardise node (52) would throw an error because it received no cubes.

• Update events documentation.
• New unit tests.

[Katherine Tomkins (katherinetomkins-mo)]

Tested as part of the EPP PR and related test tickets. Behaviour for EPP is as expected. I will leave Carwyn Pelley (@cpelley) to comment on style etc for dagrunner

[Carwyn Pelley (cpelley)]

looking at this now

[Carwyn Pelley (cpelley)]

Allow an empty set of arguments to Load to trigger the return of an event or error, depending on the value of on_missing. This allows DAGrunner to handle the situation where EPP input_latest plugin returns None when it cannot find a file.

I'm not convinced that having Load determine the correct behaviour when no file is provided is desirable, as this may end up masking a genuine issue.

In this scenario, the responsibility lies with the input_latest plugin you reference returning an IGNORE_EVENT, signalling to the plugin executor that Load should not be called. Instead, the ignore event should be propagated through the graph.

This is a fundamental aspect of the event-handling model: plugins can control graph execution by returning events, which are then propagated and filtered by the plugin executor.

Allow nodes with only one input to return an ignore event. This allows chains of nodes with single inputs to be correctly ignored.

This looks like a genuine issue that needs addressing. The changes you've made here resolve it nicely 👍
Some simplification would be good as per parameterisation.

---

There also appears to be some misunderstanding about the significance of title names and their relationship to module paths when objects are referenced within docstrings. Given that, I'd suggest considering the documentation changes in events.py as out of scope for this PR.

I already have a task planned to make more substantial corrections and improvements to the module documentation. If you're happy with that approach, I'd be glad to incorporate some of your documentation changes into that future PR and involve you in the review process.

[ryan-cocking-mo]

Allow an empty set of arguments to Load to trigger the return of an event or error, depending on the value of on_missing. This allows DAGrunner to handle the situation where EPP input_latest plugin returns None when it cannot find a file.

I'm not convinced that having Load determine the correct behaviour when no file is provided is desirable, as this may end up masking a genuine issue.

In this scenario, the responsibility lies with the input_latest plugin you reference returning an IGNORE_EVENT, signalling to the plugin executor that Load should not be called. Instead, the ignore event should be propagated through the graph.

This is a fundamental aspect of the event-handling model: plugins can control graph execution by returning events, which are then propagated and filtered by the plugin executor.

Allow nodes with only one input to return an ignore event. This allows chains of nodes with single inputs to be correctly ignored.

This looks like a genuine issue that needs addressing. The changes you've made here resolve it nicely 👍 Some simplification would be good as per parameterisation.

There also appears to be some misunderstanding about the significance of title names and their relationship to module paths when objects are referenced within docstrings. Given that, I'd suggest considering the documentation changes in events.py as out of scope for this PR.

I already have a task planned to make more substantial corrections and improvements to the module documentation. If you're happy with that approach, I'd be glad to incorporate some of your documentation changes into that future PR and involve you in the review process.

Cheers for the review Carwyn. Your explanation regarding responsibility of plugins makes sense.

I suggest we raise an error near the top of the Load call:

if not args:
  raise ValueError("Empty input arguments")

because this still takes care of an edge case (if args is empty, missing_files will also be empty and DAGrunner will continue execution) whilst providing a clear message.

I will look into an alternate solution from the EPP side, then remove the 'None' event handling and documentation changes from this PR.

Happy to assist with a review on the docs.

[Carwyn Pelley (cpelley)]

Cheers for the review Carwyn. Your explanation regarding responsibility of plugins makes sense.

No problem. I have a few thoughts on the input_latest application based on what you've discussed and would be happy to follow up offline.

I suggest we raise an error near the top of the Load call:
...because this still takes care of an edge case (if args is empty, missing_files will also be empty and DAGrunner will continue execution) whilst providing a clear message.

This behaviour was intentional. The purpose of the Load abstract class is to provide standardised capabilities beyond what most loader functions normally offer (which is why a user might not call iris.load directly). For example, it stages files from remote hosts so that remote files appear local by the time the load abstract method is invoked. This is also why continued execution was the intended behaviour.

The load abstract method is not responsible for validating arguments; that responsibility lies with the underlying loader selected by the user (e.g. iris.load, json.load, etc.). Therefore, if a user invokes load without providing the required arguments, I would expect the underlying loader to raise an exception in the same way it normally would.

Similarly, on_missing is intended to handle missing files, not incorrectly invoked loaders. Failing to provide a filepath is not a missing-file scenario; it is invalid usage and should result in an exception from the loader.

>>> import iris
iris.load(>>> iris.load()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: load() missing 1 required positional argument: 'uris'

That said, I think you've highlighted a genuine issue here. For the specific case where an application is provided with no input arguments, I think it's reasonable for the abstract class to raise an exception rather than continue execution, even if the underlying loader would eventually do so. A user explicitly providing no inputs is likely a mistake, and failing fast would provide clearer feedback. It would also protect against loaders that do not adequately validate their inputs.

On reflection, I don't think this consideration is limited to the Load abstract class. The same argument could apply more broadly to all applications deriving from the Plugin abstract base class.

Separately, it has been on my to-do list for some time to remove missing-file handling from the Load abstract class entirely and move it into its own dedicated application. This would provide a more consistent approach to missing-file handling across pipelines and avoid conflating that responsibility with the core role of the Load abstraction.

[ryan-cocking-mo]

Suggestions implemented. This PR now only addresses the issue in which a node could not propagate an IGNORE_EVENT if it only had one input argument. This includes tests and some minor edits to documentation.

[Carwyn Pelley (cpelley)]

Sorry I didn't pickup on this before now.