-
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] Application logs in CLI #13634
Conversation
for more information, see https://pre-commit.ci
is “show” the best command word here? @adam-lightning. |
@williamFalcon The idea is that Tho, for the future, I'd distinguish the "show" from "list" as one first one answers the question about the object's contents / details, and the other lists the objects we might want to interact with. Or You mean something else by the CLI evolution ? |
that makes sense. i'm just thinking about what else "show" will apply to other than logs |
@adam-lightning We should add two things:
|
|
Would be nice to update the docs in the same PR :))) up to you changelog is here: https://github.com/Lightning-AI/lightning/blob/35ec79ad1f75790767a6857637c9bbe052e47b58/src/lightning_app/CHANGELOG.md |
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.
LGTM once the comments are addressed.
Co-authored-by: thomas chaton <[email protected]>
Co-authored-by: thomas chaton <[email protected]>
Co-authored-by: thomas chaton <[email protected]>
Co-authored-by: thomas chaton <[email protected]>
for more information, see https://pre-commit.ci
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.
I like "show". For a different product, we had a CLI redesign planned (now on ice) where "show" was supposed to show information about many different types resources. It also fits well for logs.
lightning show [what] [name or id]
logs_api_client = _LightningLogsSocketAPI(client.api_client) | ||
|
||
# We will use a socket per component | ||
log_sockets = [ |
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.
This probably works well for some apps, and is a good start. I just want to point out that an app doesn't need to have a constant amount of "components". These could come and go during runtime of the app. So if you "follow" the logs, that's probably useful to know.
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.
Yes, this is definitely a skateboard version and needs to be improved upon.
What does this PR do?
This PR adds CLI command
lightning show logs <app_name> [<components>]
that prints logsDoes your PR introduce any breaking changes? If yes, please list them.
We've extended the
run_app_in_cloud
context manager to returnapp_name
as well. This is to enable easier testing, but might also affect other existing tests from different repositories.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