FIX-#4582: Inherit custom log layer #4583

vnlitvinov · 2022-06-17T10:02:02Z

Signed-off-by: Vasily Litvinov fam1ly.n4me@yandex.ru

What do these changes do?

Fix inheritance of custom log layer. It's still possible to use PANDAS-API log layer in subclasses, but to do so one has to set it explicitly.

commit message follows format outlined here
passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
signed commit with git commit -s
Resolves Modin custom logging layer is not inherited #4582
tests added and passing
module layout described at docs/development/architecture.rst is up-to-date
added (Issue Number: PR title (PR Number)) and github username to release notes for next major release

Signed-off-by: Vasily Litvinov <fam1ly.n4me@yandex.ru>

codecov · 2022-06-17T10:17:54Z

Codecov Report

Merging #4583 (3961336) into master (af7f4ed) will increase coverage by 3.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4583      +/-   ##
==========================================
+ Coverage   86.55%   89.60%   +3.04%     
==========================================
  Files         230      231       +1     
  Lines       18435    18714     +279     
==========================================
+ Hits        15957    16768     +811     
+ Misses       2478     1946     -532

Impacted Files	Coverage Δ
modin/logging/class_logger.py	`100.00% <100.00%> (ø)`
modin/experimental/xgboost/test/test_default.py	`70.00% <0.00%> (-30.00%)`	⬇️
...s/pandas_on_dask/partitioning/virtual_partition.py	`82.05% <0.00%> (-12.83%)`	⬇️
...entations/pandas_on_dask/partitioning/partition.py	`85.36% <0.00%> (-3.66%)`	⬇️
modin/pandas/__init__.py	`65.15% <0.00%> (-1.52%)`	⬇️
modin/experimental/xgboost/xgboost.py	`86.66% <0.00%> (-0.96%)`	⬇️
modin/experimental/batch/test/test_pipeline.py	`100.00% <0.00%> (ø)`
modin/pandas/base.py	`95.30% <0.00%> (+0.08%)`	⬆️
...mentations/pandas_on_ray/partitioning/partition.py	`93.57% <0.00%> (+1.83%)`	⬆️
...tations/pandas_on_python/partitioning/partition.py	`93.75% <0.00%> (+2.08%)`	⬆️
... and 16 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

naren-ponder · 2022-06-17T15:03:14Z

modin/logging/class_logger.py

    @classmethod
    def __init_subclass__(
        cls,
-        modin_layer: str = "PANDAS-API",
+        modin_layer: Optional[str] = None,


Can we change Line 39 to be modin_layer: str = _modin_logging_layer? Does that work in Python? Then just adding cls._modin_logging_layer = modin_layer will fix this issue?

If not, can we also update the function comment to reflect the Optional[str] = None?

@naren-ponder I think the Pythonic way here is to default to None, I don't think what you describe will work because cls is not in scope in the signature.

@devin-petersohn Makes sense.

This won't work, but for the different reason.

In Python, default values are computed once at function declaration, so all instances of __init_subclass__ would get the default value as "PANDAS-API", effectively not achieving what this PR aims to achieve.

devin-petersohn

LGTM, but will wait for @naren-ponder to agree.

devin-petersohn · 2022-06-19T19:16:58Z

modin/logging/class_logger.py

    @classmethod
    def __init_subclass__(
        cls,
-        modin_layer: str = "PANDAS-API",
+        modin_layer: Optional[str] = None,


@naren-ponder I think the Pythonic way here is to default to None, I don't think what you describe will work because cls is not in scope in the signature.

naren-ponder

LGTM 🚢

naren-ponder · 2022-06-19T19:34:34Z

modin/logging/class_logger.py

    @classmethod
    def __init_subclass__(
        cls,
-        modin_layer: str = "PANDAS-API",
+        modin_layer: Optional[str] = None,


@devin-petersohn Makes sense.

devin-petersohn · 2022-06-21T14:36:14Z

modin/logging/class_logger.py

@@ -31,10 +31,12 @@ class ClassLogger:
    This mixin must go first in class bases declaration to have the desired effect.
    """

+    _modin_logging_layer = "PANDAS-API"


We can move this to a constant at the top of the file and it should work.

If you mean declare the string as a constant outside of the class (and re-use it in this line to set the variable) then yes, sure, it will work. But why?..

Unless we want to re-use the same constant in the @enable_logging...

Sorry, was responding to the comment here: #4583 (comment)

I thought you were saying this didn't work as we wanted it to, my mistake. Is this ok to merge?

Unless you want me to extract this constant (which I don't see much value in, TBH), it should be good to go.

vnlitvinov · 2022-06-27T12:46:10Z

@devin-petersohn @naren-ponder I had to merge to solve the conflict, please re-approve and let's merge the thing if no objections.

jeffreykennethli · 2022-06-27T16:37:08Z

modin/logging/class_logger.py

        super().__init_subclass__(**kwargs)
        enable_logging(modin_layer, class_name, log_level)(cls)
+        cls._modin_logging_layer = modin_layer


Does this change the _modin_logging_layer for all children of ClassLogger or just the children of ClassLogger and class Foo(ClassLogger)? For example, if there are classes:

Foo(ClassLogger, modin_layer="Custom"), Bar(Foo) and Other(ClassLogger), is Other guaranteed to have modin_layer="PANDAS-API"?

Yes, it should be doing this way. The logic here is:

Take the layer from declaration, but...

If not specified, take from the class object (not instance! we don't have instances here yet, as it's the time of defining the class type)

Class object at the time of construction won't have its own _modin_layer set up, so it would take one from the parent class, effectively inheriting

Then, at the end we assign a new value in case someone has actually overridden the layer

RehanSD

LGTM!

vnlitvinov requested a review from a team as a code owner June 17, 2022 10:02

FIX-modin-project#4582: Inherit custom log layer

7113296

Signed-off-by: Vasily Litvinov <fam1ly.n4me@yandex.ru>

vnlitvinov force-pushed the fix-log-level-inheritance branch from e4d684d to 7113296 Compare June 17, 2022 10:02

vnlitvinov added the Ready for review label Jun 17, 2022

naren-ponder reviewed Jun 17, 2022

View reviewed changes

devin-petersohn previously approved these changes Jun 19, 2022

View reviewed changes

naren-ponder previously approved these changes Jun 19, 2022

View reviewed changes

devin-petersohn reviewed Jun 21, 2022

View reviewed changes

Merge branch 'master' into fix-log-level-inheritance

3961336

vnlitvinov dismissed stale reviews from naren-ponder and devin-petersohn via 3961336 June 27, 2022 12:45

naren-ponder approved these changes Jun 27, 2022

View reviewed changes

jeffreykennethli reviewed Jun 27, 2022

View reviewed changes

RehanSD approved these changes Jun 27, 2022

View reviewed changes

devin-petersohn approved these changes Jun 27, 2022

View reviewed changes

devin-petersohn merged commit 2de5c67 into modin-project:master Jun 27, 2022

vnlitvinov deleted the fix-log-level-inheritance branch June 27, 2022 18:38

jeffreykennethli mentioned this pull request Jul 1, 2022

FIX-#4527: Log Modin Ray execution #4587

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX-#4582: Inherit custom log layer #4583

FIX-#4582: Inherit custom log layer #4583

vnlitvinov commented Jun 17, 2022 •

edited

Loading

codecov bot commented Jun 17, 2022 •

edited

Loading

naren-ponder Jun 17, 2022 •

edited

Loading

devin-petersohn Jun 19, 2022

naren-ponder Jun 19, 2022

vnlitvinov Jun 21, 2022

devin-petersohn left a comment

devin-petersohn Jun 19, 2022

naren-ponder left a comment

naren-ponder Jun 19, 2022

devin-petersohn Jun 21, 2022

vnlitvinov Jun 22, 2022

devin-petersohn Jun 22, 2022

vnlitvinov Jun 27, 2022

vnlitvinov commented Jun 27, 2022

jeffreykennethli Jun 27, 2022

vnlitvinov Jun 27, 2022

RehanSD left a comment

FIX-#4582: Inherit custom log layer #4583

FIX-#4582: Inherit custom log layer #4583

Conversation

vnlitvinov commented Jun 17, 2022 • edited Loading

What do these changes do?

codecov bot commented Jun 17, 2022 • edited Loading

Codecov Report

naren-ponder Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devin-petersohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naren-ponder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnlitvinov commented Jun 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RehanSD left a comment

Choose a reason for hiding this comment

vnlitvinov commented Jun 17, 2022 •

edited

Loading

codecov bot commented Jun 17, 2022 •

edited

Loading

naren-ponder Jun 17, 2022 •

edited

Loading