Skip to content
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

[CLI] Fix cluster logs with over 5000 entries #14458

Merged
merged 1 commit into from
Sep 15, 2022

Conversation

nmiculinic
Copy link
Contributor

@nmiculinic nmiculinic commented Aug 30, 2022

What does this PR do?

Fixes https://linear.app/lightning-ai/issue/GRID-10410/very-large-limits-say-10000-dont-work-error-400 partially.

Today if you try to fetch gridv1.Cluster logs in the CLI and want more than 5000 lines it'll brake due to internal limitations. This PR implements batching, thus we're fetching 5000 by 5000 log lines from the API server.

Does your PR introduce any breaking changes? If yes, please list them.

No

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

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:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Limitations

  • if 2 log lines are on the same microsecond and on the batch boundary, the second one will be lost. This is rare to happen, thus it's going in. This is limitation for python datetime, since it works with microsecond and not nanosecond precision for timedelta.

Test plan

  • CI
  • Locally tested to fetch newly created n-020 cluster logs

Did you have fun?

Make sure you had fun coding 🙃

cc @Borda

@nmiculinic nmiculinic self-assigned this Aug 30, 2022
@nmiculinic nmiculinic changed the title Grid 10410 very large limits say 10000 dont work [CLI] Fix cluster logs with over 5000 entries Aug 30, 2022
@nmiculinic nmiculinic marked this pull request as ready for review August 30, 2022 18:07
@nmiculinic nmiculinic force-pushed the grid-10410-very-large-limits-say-10000-dont-work branch from 5c908f8 to ecf89e5 Compare August 30, 2022 18:07
@Borda Borda added bug Something isn't working app (removed) Generic label for Lightning App package labels Aug 30, 2022
@Borda Borda added this to the app:0.6 milestone Aug 30, 2022
src/lightning_app/cli/lightning_cli.py Outdated Show resolved Hide resolved
src/lightning_app/utilities/cluster_logs.py Outdated Show resolved Hide resolved
@nmiculinic nmiculinic force-pushed the grid-10410-very-large-limits-say-10000-dont-work branch from f133fe6 to e3ce0d9 Compare August 31, 2022 15:25
@nmiculinic nmiculinic requested a review from dmitsf August 31, 2022 16:24
src/lightning_app/utilities/cluster_logs.py Show resolved Hide resolved
src/lightning_app/utilities/cluster_logs.py Outdated Show resolved Hide resolved
src/lightning_app/cli/lightning_cli.py Show resolved Hide resolved
src/lightning_app/utilities/cluster_logs.py Outdated Show resolved Hide resolved
@Borda Borda requested a review from tchaton August 31, 2022 19:17
@nmiculinic nmiculinic force-pushed the grid-10410-very-large-limits-say-10000-dont-work branch from 4569a8e to 841df78 Compare September 1, 2022 12:07
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM once last comments are addressed.

@mergify mergify bot added the ready PRs ready to be merged label Sep 4, 2022
@carmocca carmocca modified the milestones: app:0.6, app:0.6.x Sep 7, 2022
@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Sep 12, 2022
@nmiculinic
Copy link
Contributor Author

This is blocked on prod release which support nanosecond precision timestamps for logs

@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Sep 12, 2022
@codecov
Copy link

codecov bot commented Sep 12, 2022

Codecov Report

Merging #14458 (088e0ab) into master (692f0f3) will increase coverage by 1%.
The diff coverage is 33%.

❗ Current head 088e0ab differs from pull request most recent head bbfea2f. Consider uploading reports for the commit bbfea2f to get more accurate results

@@            Coverage Diff            @@
##           master   #14458     +/-   ##
=========================================
+ Coverage      84%      85%     +1%     
=========================================
  Files         391      327     -64     
  Lines       28306    25860   -2446     
=========================================
- Hits        23764    21945   -1819     
+ Misses       4542     3915    -627     

@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Sep 13, 2022
@nmiculinic nmiculinic force-pushed the grid-10410-very-large-limits-say-10000-dont-work branch from c2c249c to 0620283 Compare September 14, 2022 11:53
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Sep 14, 2022
@nmiculinic nmiculinic force-pushed the grid-10410-very-large-limits-say-10000-dont-work branch 2 times, most recently from 7da69e0 to cc2fbc7 Compare September 14, 2022 13:03
@nmiculinic nmiculinic force-pushed the grid-10410-very-large-limits-say-10000-dont-work branch from 9865aea to ae9803f Compare September 14, 2022 15:10
Copy link
Member

@dmitsf dmitsf left a comment

Choose a reason for hiding this comment

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

🚀

src/lightning_app/utilities/cluster_logs.py Show resolved Hide resolved
@nmiculinic nmiculinic force-pushed the grid-10410-very-large-limits-say-10000-dont-work branch from 088e0ab to bbfea2f Compare September 15, 2022 10:26
@Borda Borda enabled auto-merge (squash) September 15, 2022 10:44
@Borda Borda merged commit 75e6c91 into master Sep 15, 2022
@Borda Borda deleted the grid-10410-very-large-limits-say-10000-dont-work branch September 15, 2022 11:05
@krshrimali krshrimali mentioned this pull request Sep 16, 2022
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app (removed) Generic label for Lightning App package bug Something isn't working ready PRs ready to be merged
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants