Release v1.1.0#56
Merged
Merged
Conversation
Tracking job completion only within the `capture_job_complete` function causes a reevaluation of all jobs to check for completion. While this is normally fine assuming the implementation provided can check multiple times for a completed job, this causes the `on_job_complete` function to be called multiple times when the respective action has completed during the workflow run whilst the host watchdog is active, and then again when the `post_run_actions` synchronizes by waiting for all tracked actions. The `on_job_complete` should be treated as a one-time function. In the case of the `PBSHost` it should do the true release of the resource requisition that was being held. The release could be checked to see if the respective named requisition exists, but ideally if it does not this _should_ be treated as an error since it can only be released once. This further reinforces the concept that `on_job_complete` should be called only once per tracked job.
Tracking job completion only within the `capture_job_complete` function causes a reevaluation of all jobs to check for completion. While this is normally fine assuming the implementation provided can check multiple times for a completed job, this causes the `on_job_complete` function to be called multiple times when the respective action has completed during the workflow run whilst the host watchdog is active, and then again when the `post_run_actions` synchronizes by waiting for all tracked actions. The `on_job_complete` should be treated as a one-time function. In the case of the `PBSHost` it should do the true release of the resource requisition that was being held. The release could be checked to see if the respective named requisition exists, but ideally if it does not this _should_ be treated as an error since it can only be released once. This further reinforces the concept that `on_job_complete` should be called only once per tracked job.
Previously, queue info could only come from the action resource dict or the host settings, giving priority to the action's queue. These changes continue to support this priority but now allow requisitions to provide their own queue information. This queue is provided as an intermediate priority such that an action-provided queue is always prioritized. Effectively, this allows actions that do not have queues designated on a multi-queue host providing multiple resource pools where the host also does not provide a queue to be auto-assigned a queue based on which queue services the resource pool of the requisition. This would reduce the need for actions or a host to precribe queues, and allow for more generalized and flexible workflow definitions. The `submit_args` function now provides the args to be formatted as well as any potential queue info. The default assignment of `queues` for resource definitions is `[None]`, and should be assumed that `queue` at any point until `launch_wrapper` assigns the value can be `None`. In the `PBSHost` during requisition determination, the `queue` is first set by the action resource `queue`, respecting the priority. If none exists, then the first `queue` that satisfies all numeric resources of the action's request is selected (which could be value `None`). The rest of the logic for determining which nodeset remains the same except that an additional check is added to make sure the nodeset is serviced (part of) by the selected queue. Because the default `queues` is `[None]`, when no action or best-fit `queue` is used because no queues are defined, the previous logic of considering all nodesets is applied. Furthermore, this implies that that if nodesets do not define `queues` but actions do request a specific queue then the requisition will fail to be resolved since no nodesets service that `queue`. This is intended behavior as queues should be used to route to specific resources and if that specific resource route cannot be honored then a failure should occur. When an action does provide a `queue` that is fulfilled by nodesets in that `queue`, the requisition `queue` should match the action `queue`. If they do not, an error is thrown. This is also possible if a nodeset services multiple queues and the wrong one is selected. This may be fixed in the future. Resource pools / nodesets are priority checked based on instantiation order (order of appearance for JSON files).
Previously, queue info could only come from the action resource dict or the host settings, giving priority to the action's queue. These changes continue to support this priority but now allow requisitions to provide their own queue information. This queue is provided as an intermediate priority such that an action-provided queue is always prioritized. Effectively, this allows actions that do not have queues designated on a multi-queue host providing multiple resource pools where the host also does not provide a queue to be auto-assigned a queue based on which queue services the resource pool of the requisition. This would reduce the need for actions or a host to precribe queues, and allow for more generalized and flexible workflow definitions. The `submit_args` function now provides the args to be formatted as well as any potential queue info. The default assignment of `queues` for resource definitions is `[None]`, and should be assumed that `queue` at any point until `launch_wrapper` assigns the value can be `None`. In the `PBSHost` during requisition determination, the `queue` is first set by the action resource `queue`, respecting the priority. If none exists, then the first `queue` that satisfies all numeric resources of the action's request is selected (which could be value `None`). The rest of the logic for determining which nodeset remains the same except that an additional check is added to make sure the nodeset is serviced (part of) by the selected queue. Because the default `queues` is `[None]`, when no action or best-fit `queue` is used because no queues are defined, the previous logic of considering all nodesets is applied. Furthermore, this implies that that if nodesets do not define `queues` but actions do request a specific queue then the requisition will fail to be resolved since no nodesets service that `queue`. This is intended behavior as queues should be used to route to specific resources and if that specific resource route cannot be honored then a failure should occur. When an action does provide a `queue` that is fulfilled by nodesets in that `queue`, the requisition `queue` should match the action `queue`. If they do not, an error is thrown. This is also possible if a nodeset services multiple queues and the wrong one is selected. This may be fixed in the future. Resource pools / nodesets are priority checked based on instantiation order (order of appearance for JSON files).
When a single subfigure is created within matplotlib it returns only one Axes element. When more than one subfigure is generated, an iterable is returned. This was assumed to be a `list`, but is in fact an `ndarray` of Axes. Conversion to some sort of `Iterable` is necessary to make the remaining code functionally identical regardless of number of subplots. The fix is to check if the returned value is `Iterable` rather than just `list`. As none of the subsequent code relies on it being an `ndarray` and there is no previous reference to `numpy` in this utility, conversion to a simple `list` is acceptable.
This breaks out the primary logic previously found in `process_patches`
to allow a single patch `dict` to be processed. The original function
still sorts and processes in priority order, but with the single patch
function available now it can be called to immediately patch the
workflow with the patch syntax.
The command line option `--patch` runs _immediately after_ the workflow
is fully loaded but before any other operations are performed. This
effectively makes it the lowest priority patch. As patch priority is
inverse to final precedent (first patches may be overwritten by later
priority patches) the command line patch always has precedence being
applied last.
Usage may look like:
```
sane_runner -p demo/ --patch '{ "actions" : { "action_000" : { "config" : { "foo" : 5 } } } }'
```
Setting the default to `[[None]]` causes a null intersection when an actual requisition exists. Instead default to an empty list, and after conversion if the `queues` list is empty then the return `queue` is `None`, otherwise try to do a set reduction.
Without the `delay=True` option, the default behavior immediately opens the new file handle before first write for every Action. Effectively, this means that 1) every action always generates a logfile even if not queued to run and 2) all action logs are cleared at workflow start such that the previous logs of completed actions not queued this time are cleared. This is undesirable behavior and unintuitive for the statefulness that is presented by SANE workflows. Providing the optional value of `delay=True` prevents file overwriting on `FileHandler` creation and instead waits for first write. This solves both 1 & 2 by only creating and updating files for Actions that in some part executed during this workflow run (calls `self.log()`)
Without the `delay=True` option, the default behavior immediately opens the new file handle before first write for every Action. Effectively, this means that 1) every action always generates a logfile even if not queued to run and 2) all action logs are cleared at workflow start such that the previous logs of completed actions not queued this time are cleared. This is undesirable behavior and unintuitive for the statefulness that is presented by SANE workflows. Providing the optional value of `delay=True` prevents file overwriting on `FileHandler` creation and instead waits for first write. This solves both 1 & 2 by only creating and updating files for Actions that in some part executed during this workflow run (calls `self.log()`)
The current regex for dereferencing only supports characters as defined
by the python re `\w` sequence. This matches alphanumeric characters as
well as the underscore (`_`) character. While the `.` is reserved for
job name construction when using an HPC host, the common delimeter
`-` could be used for instance in Action names.
By modifying the internal regexes used for dereferencing, strings
indexing into a dictionary can use `\w|-` as word constructs. This is
helpful for when users want to mix both delimeters (`_` and `-`).
For example, previously when referencing a dependency output for an
action named `build-linux_x86_gnu_debug` the dereferencing system would
silently fail. Now dereference strings like so are valid:
```
"${{ dependencies.build-linux_x86_gnu_debug.outputs.install_dir }}"
```
One should still strive for good consistent naming of action names,
outputs, and attributes.
Previously, with the order of processing the subprocess results first then checking the state, we could end up in a situation where (due to multiple actions running) the orchestrator is woken up before "action A" finished. Then during the processing of other action results, "action A" completes without the results being processed. Then when checking the state, "action A" reports being completed so is removed from the node list. Once removed it will never be checked again for results. On HPC launches, this race condition could lead to missing job IDs since a node was completed but its results not post processed. One possible solution is to decouple the processed_nodes loop from the nodes stored in the results map. However, the results and state of an Action are conceptually coupled and thus the decision for now is to process the two together. Instead, the order is flipped such that state checks are performed first with a single read value then the results processed. Next, the action subprocess is coupled by checking if it has been "processed" (removed from the `processed_nodes`) instead of `results[node].done()`. This coupling ensures that if the state satisfied completion [and a subprocess was launched] then a result should follow shortly after. Thus the result call is time limited to 10 seconds. This fix critically depends on two things: 1. Gathering results is strictly coupled to state completion 2. No action prematurely sets a complete state before effectively terminating launch routine. As users are heavily advised against modifying the `Action.launch()` function, point (2) should be satisfied as well. Any user deriving their own `launch()` function should keep this point in mind.
The current regex for dereferencing only supports characters as defined
by the python re `\w` sequence. This matches alphanumeric characters as
well as the underscore (`_`) character. While the `.` is reserved for
job name construction when using an HPC host, the common delimeter `-`
could be used for instance in Action names.
By modifying the internal regexes used for dereferencing, strings
indexing into a dictionary can use `\w|-` as word constructs. This is
helpful for when users want to mix both delimeters (`_` and `-`).
For example, previously when referencing a dependency output for an
action named `build-linux_x86_gnu_debug` the dereferencing system would
silently fail. Now dereference strings like so are valid:
```
"${{ dependencies.build-linux_x86_gnu_debug.outputs.install_dir }}"
```
One should still strive for good consistent naming of action names,
outputs, and attributes.
Previously, with the order of processing the subprocess results first then checking the state, we could end up in a situation where (due to multiple actions running) the orchestrator is woken up before "action A" finished. Then during the processing of other action results, "action A" completes without the results being processed. Then when checking the state, "action A" reports being completed so is removed from the node list. Once removed it will never be checked again for results. On HPC launches, this race condition could lead to missing job IDs since a node was completed but its results not post processed. One possible solution is to decouple the processed_nodes loop from the nodes stored in the results map. However, the results and state of an Action are conceptually coupled and thus the decision for now is to process the two together. Instead, the order is flipped such that state checks are performed first with a single read value then the results processed. Next, the action subprocess is coupled by checking if it has been "processed" (removed from the `processed_nodes`) instead of `results[node].done()`. This coupling ensures that if the state satisfied completion [and a subprocess was launched] then a result should follow shortly after. Thus the result call is time limited to 10 seconds. This fix critically depends on two things: 1. Gathering results is strictly coupled to state completion 2. No action prematurely sets a complete state before effectively terminating launch routine. As users are heavily advised against modifying the `Action.launch()` function, point (2) should be satisfied as well. Any user deriving their own `launch()` function should keep this point in mind.
Control of whether the output of `Action.execute_subprocess()` is dictated by the internal `Action` attribute `__exec_raw__`. If users need to execute some subprocess within their custom Action with wrapped logging output, they need to ensure this value is set correctly. Likewise, to ensure consistent logging, users would need to reinstate the previous value after the subprocess finishes. Rather than require temp variable and read/write of a private variable the control can now be facilitated by a push/pop set of helper functions. `pop_exec_raw()` should only be called after a corresponding `push_exec_raw()` call. Multiple nested calls are not supported at this time (e.g. multiple push before equivalent pop) as it is meant to only wrap around `execute_subprocess()` calls.
Setting the log scope at the caller level of the user defined functions ensures that the user does not need to set any scope of their own, regardless of any inheritance. Users are still free to set their own scopes within functions, but these functions will always start with a base scope corresponding to the function name.
Setting the log scope at the caller level of the user defined functions ensures that the user does not need to set any scope of their own, regardless of any inheritance. Users are still free to set their own scopes within functions, but these functions will always start with a base scope corresponding to the function name.
PR #34 revamped the logging capabilities and set a specific level for stdout annotation. The log level output from Environment functions that generate stdout log info were not updated to use this new value. This change updates the value to use the internally defined log level for stdout.
As sane_view is meant to be a post-processing script, it is often useful to view the log files of Actions. Likewise, the ability to filter for logs with reported errors and adjust path to be relative is important. To keep command function calls consistent all signatures have been updated to pass the full option set then allow the function logic to get only what it needs.
This adds a command to the `sane_view` helper to list out the logfiles for a workflow. Options to view the `.runlog` instead, errors only, and as relative paths are provided to aid in any workflow post-run evaluation. These changes also include a minor fix to the plotting of `usage` when a host has no resource consumption.
The dereference regex (both full string detection and bracket indexing)
originally has two near identical copies to account for any number of
repeating sub-references and a final fully formed attribute. If the
regex has to be modified for new syntax it must be updated in both
locations.
To fix this the regex now expects one or more fully formed attribute
expressions that are delimited by '.' character. To avoid duplication
of the regex again using lookbehind constructs the expression *does*
allow ending a reference string with '.' which is technically malformed.
The logic for processing reference strings consisted of comparing the
set of matches and their spans to see if they are roughly equivalent.
This was both inefficient, overly complicated, and lacked checking for
cycles greater than one step (A->A->A).
An improvement on this is to instead keep a history of each full set of
substitutions made in a list, and after the first pass check if the new
processed string is within the history. This method both greatly
simplifies the check condition if a string is done processing (i.e. it
did not change on the next pass therefore no more substitution can be
made) and if a complex cycle was encountered (e.g. appears in the
history that is not the latest change).
A side benefit to this is our logging info can now include the full
breakdown of substitutions per iteration by repeating that history back
until the final output, which may be useful for debugging. This info is
not controllable at the moment, but may be considered for toggling if it
begins to clutter logs.
A reference string with malformed ending ('.') is allowed and produces
a warning message, with corrections to the string made in situ during
processing.
Test for edge cases of complex cycles and malformed but accepted strings
The dereference regex (both full string detection and bracket indexing)
originally has two near identical copies to account for any number of
repeating sub-references and a final fully formed attribute. If the
regex has to be modified for new syntax it must be updated in both
locations.
To fix this the regex now expects one or more fully formed attribute
expressions that are delimited by '.' character. To avoid duplication of
the regex again using lookbehind constructs the expression *does* allow
ending a reference string with '.' which is technically malformed.
The logic for processing reference strings consisted of comparing the
set of matches and their spans to see if they are roughly equivalent.
This was both inefficient, overly complicated, and lacked checking for
cycles greater than one step (A->A->A).
An improvement on this is to instead keep a history of each full set of
substitutions made in a list, and after the first pass check if the new
processed string is within the history. This method both greatly
simplifies the check condition if a string is done processing (i.e. it
did not change on the next pass therefore no more substitution can be
made) and if a complex cycle was encountered (e.g. appears in the
history that is not the latest change).
A side benefit to this is our logging info can now include the full
breakdown of substitutions per iteration by repeating that history back
until the final output, which may be useful for debugging. This info is
not controllable at the moment, but may be considered for toggling if it
begins to clutter logs.
A reference string with malformed ending ('.') is allowed and produces a
warning message, with corrections to the string made in situ during
processing.
Tests are included to ensure that this simplification addresses previous
issues and is an overall improvement.
Add much needed documentation on custom `Action` classes, complex usage of dereference strings, and the `Logger`: * `Logger` reference API introduced * Documentation on general inheritance allowing JSON interface extending (`OptionLoader`) * Documentation on deriving from `Action` and expected practice, with notes on execution contexts * Documentation on dereference strings and their particular use in deferred runtime evaluation vs hand-written logic. * Small note on paired project https://github.com/islas/sane-workflows-action/ for use in GitHub Actions * Link back to readthedocs main documentation in top-level readme A minor fix of having `Action.outputs` persistent across execution context is also included. Documentation was slightly restructured to avoid unnecessary nesting/clicking.
To account for different execution context between the orchestrator task and the actual Action run, Actions must save+load their outputs. This is fine for a local run host where the tasks are directly executed by the main orchestrator process and thus executed in DAG-dependency order. However in HPC environments tasks are submitted into a queue in DAG order with dependency info set at that time. Execution and thus evaluation of outputs will not occur until the real host runs the job. This delay would cause dependency outputs to run with the old values set at submission time instead of the values after dependencies have finished executing. To remedy this, building off of #52, Actions now reload the dependency outputs inside the `action_launcher.py` Note that dependency outputs will still be incorrect within the main orchestrator process context, but at this stage this information should not be needed as is never stored beyond the pickle file. Should users need the dependency output information per action in this situation (e.g. post_run_actions() for the Host) users can call the internal API function `Action.reload_dependency_outputs()` manually.
Without this suppression environment loading scripts that give verbose information (e.g. intel setvars.sh) will have these lines executed by the Python `exec()` function. Best case, it is total nonsense and throws an error, worst case it executes some catastrophic code. Note that the loader still executes perceived differences in the env, so users should be cautious about loading scripts and be aware of code injection.
Ensure that dependency outputs are current at Action start To account for different execution context between the orchestrator task and the actual Action run, Actions must save+load their outputs. This is fine for a local run host where the tasks are directly executed by the main orchestrator process and thus executed in DAG-dependency order. However in HPC environments tasks are submitted into a queue in DAG order with dependency info set at that time. Execution and thus evaluation of outputs will not occur until the real host runs the job. This delay would cause dependency outputs to run with the old values set at submission time instead of the values after dependencies have finished executing. To remedy this, building off of #52, Actions now reload the dependency outputs inside the `action_launcher.py` Note that dependency outputs will still be incorrect within the main orchestrator process context, but at this stage this information should not be needed as is never stored beyond the pickle file. Should users need the dependency output information per action in this situation (e.g. post_run_actions() for the Host) users can call the internal API function `Action.reload_dependency_outputs()` manually.
The system just emulates deferred queue separate from SANE orchestration and handling very basic dependency staging. The two directories are used as trackers for queuing and completion. Job id is assigned based on order of arrival and is used to write out all tracked information. This meshes with the expectations of the `HPCHost` within SANE workflows.
Use mock HPC system to create a very rudimentary `HPCHost` class. It does not do any error checking or resource management and instead focuses on behavior of deferred execution.
#54) Without this suppression environment loading scripts that give verbose information (e.g. intel setvars.sh) will have these lines executed by the Python `exec()` function. Best case, it is total nonsense and throws an error, worst case it executes some catastrophic code. Note that the loader still executes perceived differences in the env, so users should be cautious about loading scripts and be aware of code injection.
Create a very simple "HPC" system focused on behavior of dependency resolution and separate execution context from the `Orchestrator`. The system still has enough capability to properly work with SANE and provide job IDs, state and status values, and execute Actions. It *DOES NOT* handle resource management. The test is a very simple execution of A->B actions. More tests can be developed later to test correctness of HPC-like behavior beyond dry-runs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Official release
Includes the following PRs:
sane_view#50