-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[App] Introduce Commands #13602
[App] Introduce Commands #13602
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…lightning into lightning_commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unblock
@@ -155,7 +156,7 @@ jobs: | |||
shell: bash | |||
run: | | |||
mkdir -p ${VIDEO_LOCATION} | |||
HEADLESS=1 python -m pytest tests/tests_app_examples/test_${{ matrix.app_name }}.py::test_${{ matrix.app_name }}_example_cloud --timeout=900 --capture=no -v --color=yes | |||
HEADLESS=1 PACKAGE_LIGHTNING=1 python -m pytest tests/tests_app_examples/test_${{ matrix.app_name }}.py::test_${{ matrix.app_name }}_example_cloud --timeout=900 --capture=no -v --color=yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, are this env. var described somewhere?
while not self._exit_event.is_set(): | ||
self.run_once() | ||
try: | ||
while not self._exit_event.is_set(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this accidentally turn into an infinite loop?
data = await request.json() | ||
command_name = data.get("command_name", None) | ||
if not command_name: | ||
raise Exception("The provided command name is empty.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
def makedirs(path: str): | ||
r"""Recursive directory creation function.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not use r in these simple strings :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tchaton, mind adding some more docstrings to newly added classes? 🐰
What does this PR do?
This PR introduces Lightning App Commands.
Commands can be configured from the app by overriding the
configure_commands
hook on the flow and returning a list of dictionaries mapping a command_name to a flow method.Once the app is running, someone can remotely execute those methods from the CLI by providing the command_name.
Here is how it looks:
Once the app is running with
Trigger one of its commands as follows
lightning run command flow_trigger_command --args name=my_name
To make it work in the cloud, add
id
.lightning run command flow_trigger_command --args name=my_name --id=01g7q3kg9kvtsdx4ak0cdmrbe8
Fixed #13637
Alternatives API:
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃
cc @Borda