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

Enable RMM Debug Logging via Python #1339

Merged
merged 22 commits into from
Sep 26, 2023

Conversation

harrism
Copy link
Member

@harrism harrism commented Sep 12, 2023

Description

Enables RMM debug logging in Python and exposes controls for the level of logging. Fixes #1336.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@harrism harrism requested review from a team as code owners September 12, 2023 07:46
@harrism harrism self-assigned this Sep 12, 2023
@github-actions github-actions bot added CMake Python Related to RMM Python API labels Sep 12, 2023
@harrism harrism added feature request New feature or request non-breaking Non-breaking change and removed CMake Python Related to RMM Python API labels Sep 12, 2023
@github-actions github-actions bot added CMake Python Related to RMM Python API labels Sep 12, 2023
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Some minor possible cleanup (mostly documentation), looks good to me overall

python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/CMakeLists.txt Show resolved Hide resolved
python/rmm/__init__.py Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/tests/test_rmm.py Show resolved Hide resolved
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Thanks @harrism ! Looks really good. A bunch of very minor polish/nitpick comments, but nothing to hold things up (other than the flake8 failure in linting on line 111 of logger.pyx).

compile_commands.json Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Show resolved Hide resolved
python/rmm/_lib/logger.pyx Show resolved Hide resolved
python/rmm/tests/test_rmm.py Outdated Show resolved Hide resolved
python/rmm/tests/test_rmm.py Outdated Show resolved Hide resolved
python/rmm/tests/test_rmm.py Outdated Show resolved Hide resolved
@wence-
Copy link
Contributor

wence- commented Sep 19, 2023

other than the flake8 failure in linting on line 111 of logger.pyx

I committed the fix for that one.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I started a review but I'd like to wait until some of @wence-'s comments can be resolved before diving into this at a closer level.

For the moment, I'm wondering whether should_log should be a private function, or maybe replaced by something like a private member (or method) _compiled_log_level that can be used for direct determination of the compiled setting.

compile_commands.json Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I just commented on the threads where I was tagged but can provide a more comprehensive review once more of the open questions have been addressed.

@harrism
Copy link
Member Author

harrism commented Sep 20, 2023

My latest changes expose the C++ enum directly, renamed to rmm.logging_level and with its names capitalized to be more Pythonic. The functions that take a level now only accept instances of rmm.logging_level, not ints or strings. This simplifies the logic and testing a lot. It also warns if you try to set a logging level that will not be logged due to compilation level.

Regarding should_log: we could make it private, but I thought it might come in handy for anyone trying to detect or understand why any custom log messages are not being logged. It directly mirrors a C++ function in spdlog.

I could expose logging functions to enable adding custom messages to the log from Python code as well. Separate PR.

Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Minor fixes for link syntax

python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
The URL must be surrounded by < > characters.
@wence-
Copy link
Contributor

wence- commented Sep 20, 2023

From my perspective this looks great as is. I (hopefully) fixed the doc-build failures 🤞 which were due to bad links. Not sure if @bdice will still have comments

@harrism
Copy link
Member Author

harrism commented Sep 20, 2023

Thanks for fixing my sphinx links @wence-, and for all your suggestions.

@harrism harrism requested review from bdice and vyasr September 20, 2023 21:33
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Following @wence-'s lead, I'm trying out the semantic review comments! Overall this PR is looking really good now, just a few things that I'd like to see resolved before we can merge.

FYI I would guess that the issue you were seeing where any integral value was accepted for the enum is closely related to cython/cython#5610.

python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Outdated Show resolved Hide resolved
python/rmm/_lib/logger.pyx Show resolved Hide resolved
python/rmm/_lib/logger.pyx Show resolved Hide resolved
python/rmm/tests/test_rmm.py Show resolved Hide resolved
python/rmm/tests/test_rmm.py Show resolved Hide resolved
python/rmm/tests/test_rmm.py Outdated Show resolved Hide resolved
harrism and others added 2 commits September 21, 2023 12:30
Co-authored-by: Vyas Ramasubramani <vyas.ramasubramani@gmail.com>
@harrism harrism requested a review from vyasr September 25, 2023 22:20
python/rmm/_lib/logger.pyx Show resolved Hide resolved
python/rmm/tests/test_rmm.py Show resolved Hide resolved
@harrism
Copy link
Member Author

harrism commented Sep 26, 2023

/merge

@rapids-bot rapids-bot bot merged commit 9f10543 into rapidsai:branch-23.10 Sep 26, 2023
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake feature request New feature or request non-breaking Non-breaking change Python Related to RMM Python API
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG] No way to set RMM_LOGGING_LEVEL when building Python module
4 participants