Skip to content

Add support for simultaneous runs of Script helper#31937

Merged
balloob merged 5 commits intohome-assistant:devfrom
pnbruckner:script-helper
Feb 24, 2020
Merged

Add support for simultaneous runs of Script helper#31937
balloob merged 5 commits intohome-assistant:devfrom
pnbruckner:script-helper

Conversation

@pnbruckner
Copy link
Copy Markdown
Contributor

@pnbruckner pnbruckner commented Feb 18, 2020 •
edited
Loading

Proposed change

The Script helper (i.e., homeassistant.helpers.script) is used by several integrations to implement Home Assistant's Script Syntax, the most notable being automation & script. However the current behavior, when run multiple times simultaneously (e.g., via an automation that is triggered quickly), is at best unexpected, and worse, requires clumsy workarounds. (More on that below.)

This PR is the first step towards resolving those issues. It improves the Script helper to properly support multiple, simultaneous runs, with options to provide usage flexibility. However, by default, it implements the current behavior to avoid breakage. The intention is to ultimately remove the so-called "legacy" behavior.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Background

See PR #31740. It was a first attempt at resolving these issues and contains a description of the problems related to the current behavior of the Script helper, as well as YAML for an example automation that is affected by it. However it was decided to break up the changes into smaller chunks, this being the first.

Although not mentioned in the other PR, the typical workaround is to move the automation's action sequence into a script, and then in the automation's action section invoke the new script like this:

action:
- service: script.turn_off
  entity_id: script.new
- service: script.new

This is an attempt to implement the new restart behavior described below.

Also, there currently isn't an easy way to call a script completly "synchronously" if it contains a delay or wait_template. The typical workaround to effectively make a script synchronous is to use an input_boolean. It would first be turned on by the "caller", then the caller would use a wait_template to wait for it to be turned off, which would done by the last step of the called script. This is an attempt to implement the new blocking behavior described below.

New Script helper options

if_running

  • Controls what happens if Script run (i.e., async_run called) while previous run has not yet completed.
  • Values:
    • error Raise an exception
    • ignore Return without doing anything (previous run continues as-is)
    • parallel Start run in new task
    • restart Stop previous run before starting new run

run_mode

  • Controls when call to async_run will return.
  • Values:
    • background Returns immediately
    • legacy Implements previous behavior, which is to return when done, or when suspended by delay or wait_template, whichever comes first
    • blocking Returns when run has completed

Defaults

  • If neither is specified, default is run_mode=legacy (and if_running is not used.)
  • Otherwise, defaults are if_running=parallel and run_mode=blocking.

Note: If run_mode is set to legacy then if_running must be None.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@pnbruckner
Add tests for legacy Script helper behavior
e14fcdb
@pnbruckner
Add Script helper if_running and run_mode options
bd572bb 
- if_running controls what happens if Script run while previous run
  has not completed. Can be:
  - error: Raise an exception
  - ignore: Return without doing anything (previous run continues as-is)
  - parallel: Start run in new task
  - restart: Stop previous run before starting new run
- run_mode controls when call to async_run will return. Can be:
  - background: Returns immediately
  - legacy: Implements previous behavior, which is to return when done,
            or when suspended by delay or wait_template
  - blocking: Returns when run has completed
- If neither is specified, default is run_mode=legacy (and if_running
  is not used.) Otherwise, defaults are if_running=parallel and
  run_mode=background. If run_mode is set to legacy then if_running must
  be None.
- Caller may supply a logger which will be used throughout instead of
  default module logger.
- Move Script running state into new helper classes, comprised of an
  abstract base class and two concrete clases, one for legacy behavior
  and one for new behavior.
- Remove some non-async methods, as well as call_from_config which has
  only been used in tests.
- Adjust tests accordingly.
@pnbruckner pnbruckner requested a review from a team as a code owner February 18, 2020 02:07
@probot-home-assistant
Copy link
Copy Markdown

probot-home-assistant Bot commented Feb 18, 2020

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (automation) you are listed as a codeowner for? Thanks!

@probot-home-assistant
Copy link
Copy Markdown

probot-home-assistant Bot commented Feb 18, 2020

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (script) you are listed as a codeowner for? Thanks!

@pnbruckner pnbruckner requested a review from balloob February 18, 2020 02:08
@pnbruckner
Copy link
Copy Markdown
Contributor Author

pnbruckner commented Feb 18, 2020

Tests were added to help make sure the "legacy" behavior is maintained. Test for the new features (i.e., if_running & run_mode) still need to be added.

@codecov

This comment has been minimized.

Sign in to view
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/components/automation/__init__.py
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 18, 2020

This is off to a really great start! 🎉

balloob
balloob reviewed Feb 18, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
@pnbruckner
Change per review
0b151e3 
- Change run_mode default from background to blocking.
- Make sure change listener is called, even when there's an unexpected
  exception.
- Make _ScriptRun.async_stop more graceful by using an asyncio.Event for
  signaling instead of simply cancelling Task.
- Subclass _ScriptRun for background & blocking behavior.

Also:

- Fix timeouts in _ScriptRun by converting timedeltas to float seconds.
- General cleanup.
balloob
balloob reviewed Feb 20, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 20, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 20, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py
balloob
balloob reviewed Feb 20, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 20, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 20, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 20, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py
balloob
balloob reviewed Feb 20, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
@pnbruckner
Change per review 2
f0bb3c1 
- Don't propagate exceptions if call from user has already returned
  (i.e., for background runs or legacy runs that have suspended.)
- Allow user to specify if exceptions should be logged. They will still
  be logged regardless if exception is not propagated.
- Rename _start_script_delay and _start_wait_template_delay for
  clarity.
- Remove return value from Script.async_run.
- Fix missing await.
- Change call to self.is_running in Script.async_run to direct test of
  self._runs.
@pnbruckner
Copy link
Copy Markdown
Contributor Author

pnbruckner commented Feb 21, 2020

FYI I'm working on the tests now.

balloob
balloob reviewed Feb 23, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py Outdated
balloob
balloob reviewed Feb 23, 2020
View reviewed changes
Comment thread homeassistant/helpers/script.py
@pnbruckner
Change per review 3 and add tests
e9ab4c9 
- Remove Script.set_logger().
- Enhance existing tests to check all run modes.
- Add tests for new features.
- Fix a few minor bugs found by tests.
balloob
balloob approved these changes Feb 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! 🎉

Anything else you want to change before we merge it?

@pnbruckner
Copy link
Copy Markdown
Contributor Author

pnbruckner commented Feb 24, 2020

Thanks! The reviews have been great and not only helped improve the code, but I learned a lot, too. 😃

No, I think that's it. Besides the formal tests I also have a few scripts and automations I wrote specifically for this to test if the legacy behavior has been maintained, and they seem to indicate it has. So I think we're good.

Next PR will focus on using the new capabilities in the script integration. After that automations.

@balloob balloob merged commit b2d7bc4 into home-assistant:dev Feb 24, 2020
@pnbruckner pnbruckner deleted the script-helper branch February 24, 2020 22:58
@lock lock Bot locked and limited conversation to collaborators Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@balloob balloob balloob approved these changes

Assignees

No one assigned

Labels

cla-signed core has-tests integration: automation integration: script

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pnbruckner @balloob @homeassistant