-
Notifications
You must be signed in to change notification settings - Fork 653
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
FEAT-#4371: Add logging to Modin #4372
FEAT-#4371: Add logging to Modin #4372
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4372 +/- ##
==========================================
+ Coverage 86.82% 89.32% +2.49%
==========================================
Files 222 226 +4
Lines 18041 18297 +256
==========================================
+ Hits 15664 16343 +679
+ Misses 2377 1954 -423
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
18caeb3
to
146de4d
Compare
This pull request introduces 1 alert and fixes 1 when merging 10990052a0569eec8e9fc2b71924c92c089bc8b3 into e073c4f - view on LGTM.com new alerts:
fixed alerts:
|
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.
@naren-ponder thank you for your work on this! I have a lot of comments because there's a lot of new code.
Here's one batch of comments. I reviewed a few files completely and made it partway through modin/logging/config.py
. I'll finish this round of review later.
modin/core/execution/ray/implementations/pandas_on_ray/partitioning/partition.py
Outdated
Show resolved
Hide resolved
modin/core/execution/ray/implementations/pandas_on_ray/partitioning/partition.py
Outdated
Show resolved
Hide resolved
This pull request introduces 1 alert and fixes 1 when merging b8d9a1d622d8e97a0637e2e416803cb763b87c49 into e073c4f - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 1 when merging 3eb582ed5cca65034070a765c21e5590bc6e4640 into e073c4f - view on LGTM.com new alerts:
fixed alerts:
|
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've gone through all the code changes now.
modin/core/execution/ray/implementations/pandas_on_ray/partitioning/partition.py
Outdated
Show resolved
Hide resolved
This pull request introduces 1 alert and fixes 1 when merging 84708c277a02cf132a22e921c048cf3877249dc6 into e073c4f - view on LGTM.com new alerts:
fixed alerts:
|
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.
Looks good so far! I think there may be more design considerations to go over, so I will add another review.
modin/core/execution/ray/implementations/pandas_on_ray/partitioning/partition.py
Outdated
Show resolved
Hide resolved
modin/core/execution/ray/implementations/pandas_on_ray/partitioning/partition.py
Outdated
Show resolved
Hide resolved
modin/core/execution/ray/implementations/pandas_on_ray/partitioning/partition.py
Show resolved
Hide resolved
nit: with this PR checked out and pip installed, I am unable to run I'm 90% sure it's because JupyterLab initializes with its own logger, which is conflicting with the logging module introduced in this PR. (something like https://stackoverflow.com/a/60677039/19027728). I then locally renamed the logging dir from |
This pull request introduces 1 alert and fixes 1 when merging 5719db40ad00d2a410157b72fdd1b13908f79ab4 into 270fd69 - view on LGTM.com new alerts:
fixed alerts:
|
modin/core/execution/ray/implementations/pandas_on_ray/partitioning/partition.py
Outdated
Show resolved
Hide resolved
This pull request introduces 1 alert and fixes 1 when merging 20e753ebe15401778e11ea85440db74427b2e7be into 270fd69 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 1 when merging a0e76b534d1d87b6861536301b2ee168e91614f8 into 270fd69 - view on LGTM.com new alerts:
fixed alerts:
|
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Co-authored-by: Yaroslav Igoshev <Poolliver868@mail.ru>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com>
Signed-off-by: Naren Krishna <naren@ponder.io>
Co-authored-by: Yaroslav Igoshev <Poolliver868@mail.ru>
39c15be
to
061ef36
Compare
This pull request fixes 3 alerts when merging 061ef36 into 60518ca - view on LGTM.com fixed alerts:
|
Signed-off-by: Naren Krishna <naren@ponder.io>
This pull request fixes 3 alerts when merging f9a504b into 60518ca - view on LGTM.com fixed alerts:
|
Signed-off-by: Naren Krishna <naren@ponder.io>
This pull request fixes 3 alerts when merging 8a5638a into 60518ca - view on LGTM.com fixed alerts:
|
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.
Thanks @naren-ponder LGTM
@@ -207,6 +233,7 @@ def mask(self, row_labels, col_labels): | |||
new_obj._width_cache = compute_sliced_len.remote( | |||
col_labels, self._width_cache | |||
) | |||
logger.debug(f"EXIT::Partition.mask::{self._identity}") |
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.
In the interest of merging, I think we can leave this in.
What do these changes do?
Add logging to Modin to trace API function calls.
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
git commit -s
docs/development/architecture.rst
is up-to-date