FEAT-#4524: Split Modin API and Memory log files #4526
Merged
Conversation
This file contains 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
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Codecov Report
@@ Coverage Diff @@
## master #4526 +/- ##
==========================================
+ Coverage 86.36% 88.57% +2.21%
==========================================
Files 228 229 +1
Lines 18413 18913 +500
==========================================
+ Hits 15902 16753 +851
+ Misses 2511 2160 -351
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Signed-off-by: Naren Krishna <naren@ponder.io>
naren-ponder
commented
Jun 4, 2022
|
mvashishtha
reviewed
Jun 6, 2022
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
mvashishtha
previously approved these changes
Jun 7, 2022
YarShev
reviewed
Jun 7, 2022
Co-authored-by: Yaroslav Igoshev <Poolliver868@mail.ru>
RehanSD
requested changes
Jun 8, 2022
There was a problem hiding this comment.
Looks good to me - left some minor comments. Couple of overall comments -
- I believe you need to use `` when writing in-line code in
.rstfiles rather than just ` by itself. (e.g. ``code formatted text``) - The tense of the docs change from referring to a user and telling them what to expect to speaking in first person - would be good to unify that
- I noticed that the file size for both traces and memory logs are the same - is that by design, or have we considered allowing different file sizes for each? I imagine it could be possible for a user to want more of one than the other e.g., but this may also be over engineering.
|
The memory and API log sizes being the same is mostly a coincidence (was not designed that way), but I think making that configurable is not worth it. |
Signed-off-by: Naren Krishna <naren@ponder.io>
RehanSD
requested changes
Jun 8, 2022
| Usage example | ||
| ------------- | ||
| **Developer Warning:** In some cases, running services like JupyterLab in the ``modin/modin`` directory may result in circular dependency issues. | ||
| This is as a result of a name conflict between the ``modin/logging`` directory and the Python ``logging`` module, which may be used as a default in |
There was a problem hiding this comment.
Suggested change
| This is as a result of a name conflict between the ``modin/logging`` directory and the Python ``logging`` module, which may be used as a default in | |
| This is due to a naming conflict between the ``modin/logging`` directory and the Python ``logging`` module, which may be used as a default in |
Signed-off-by: Naren Krishna <naren@ponder.io>
RehanSD
reviewed
Jun 8, 2022
Signed-off-by: Naren Krishna <naren@ponder.io>
YarShev
approved these changes
Jun 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Signed-off-by: Naren Krishna naren@ponder.io
What do these changes do?
In the current approach to Modin logging, all the logs are logged to a single file. In this updated approach, we separate out the memory logs from the Modin API logs.
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -sdocs/development/architecture.rstis up-to-date