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.

Sign up for GitHub

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

Jump to bottom

FIX-#4582: Inherit custom log layer #4583

Merged
merged 2 commits into from
Jun 27, 2022
Merged

FIX-#4582: Inherit custom log layer #4583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7113296
FIX-#4582: Inherit custom log layer
vnlitvinov Jun 17, 2022
3961336
Merge branch 'master' into fix-log-level-inheritance
vnlitvinov Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/release_notes/release_notes-0.16.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Key Features and Updates
* FIX-#4589: Pin protobuf<4.0.0 to fix ray (#4590)
* FIX-#4577: Set attribute of Modin dataframe to updated value (#4588)
* FIX-#4411: Fix binary_op between datetime64 Series and pandas timedelta (#4592)
* FIX-#4582: Inherit custom log layer (#4583)
* Performance enhancements
* PERF-#4182: Add cell-wise execution for binary ops, fix bin ops for empty dataframes (#4391)
* Benchmarking enhancements
Expand Down
6 changes: 5 additions & 1 deletion modin/logging/class_logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


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

@naren-ponder naren-ponder Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@devin-petersohn Makes sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

class_name: Optional[str] = None,
log_level: Optional[str] = "info",
**kwargs,
Expand All @@ -53,5 +55,7 @@ def __init_subclass__(
The log level (INFO, DEBUG, WARNING, etc.).
**kwargs : dict
"""
modin_layer = modin_layer or cls._modin_logging_layer
super().__init_subclass__(**kwargs)
enable_logging(modin_layer, class_name, log_level)(cls)
cls._modin_logging_layer = modin_layer
Copy link

Choose a reason for hiding this comment

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

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"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

  1. Take the layer from declaration, but...
  2. 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)
  3. 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
  4. Then, at the end we assign a new value in case someone has actually overridden the layer

4 changes: 2 additions & 2 deletions modin/test/test_logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,6 @@ def method2(self):
"STOP::CUSTOM::Foo.method1",
"START::CUSTOM::Foo.method1",
"STOP::CUSTOM::Foo.method1",
"START::PANDAS-API::Bar.method2",
"STOP::PANDAS-API::Bar.method2",
"START::CUSTOM::Bar.method2",
"STOP::CUSTOM::Bar.method2",
]