Conversation
Contributor
|
""" WalkthroughThe documentation was updated to require Python ≥3.11 instead of ≥3.10. In the code, the logging filter's initialization was modified to ensure compatibility with Python versions before 3.11 by adding a fallback mechanism for resolving log levels if certain logging APIs are unavailable. Changes
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/installation.qmd(2 hunks)src/axolotl/logging_config.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: PyTest (3.11, 2.7.1)
- GitHub Check: PyTest from Source Dist (3.11, 2.5.1)
- GitHub Check: PyTest from Source Dist (3.11, 2.7.1)
- GitHub Check: PyTest (3.11, 2.6.0)
- GitHub Check: PyTest from Source Dist (3.11, 2.6.0)
- GitHub Check: pre-commit
- GitHub Check: PyTest (3.11, 2.5.1)
- GitHub Check: preview
- GitHub Check: pre-commit
🔇 Additional comments (1)
docs/installation.qmd (1)
17-17: Python ≥ 3 .11 requirement conflicts with new 3 .10 compatibility fallbackThe code change in
src/axolotl/logging_config.pypurposefully adds support for Python 3.10 by falling back whengetLevelNamesMappingis absent, yet the documentation now raises the minimum version to 3.11.
Either remove the fallback and truly drop 3.10, or keep the previous “≥ 3.10 (3.11 recommended)” wording so users aren’t mis-led.Also applies to: 156-156
Comment on lines
28
to
40
| axolotl_log_level = os.getenv("AXOLOTL_LOG_LEVEL", DEFAULT_AXOLOTL_LOG_LEVEL) | ||
| other_log_level = os.getenv("LOG_LEVEL", DEFAULT_LOG_LEVEL) | ||
|
|
||
| try: | ||
| # py311+ only | ||
| level_mapping = logging.getLevelNamesMapping() | ||
| self.axolotl_level = level_mapping[axolotl_log_level] | ||
| self.other_level = level_mapping[other_log_level] | ||
| except AttributeError: | ||
| # For py311-, use getLevelName directly | ||
| self.axolotl_level = logging.getLevelName(axolotl_log_level) | ||
| self.other_level = logging.getLevelName(other_log_level) | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden log-level resolution & avoid silent KeyError / case issues
logging.getLevelNamesMapping()will raise KeyError if the env value isn’t an exact key (e.g. “info” or custom levels).- In the fallback branch,
logging.getLevelName()returns a string like"Level foo"for unknown names, which will make the later>=comparison raiseTypeError. - Conversion should be case-insensitive.
Proposed fix:
- axolotl_log_level = os.getenv("AXOLOTL_LOG_LEVEL", DEFAULT_AXOLOTL_LOG_LEVEL)
- other_log_level = os.getenv("LOG_LEVEL", DEFAULT_LOG_LEVEL)
+ axolotl_log_level = os.getenv("AXOLOTL_LOG_LEVEL",
+ DEFAULT_AXOLOTL_LOG_LEVEL).upper()
+ other_log_level = os.getenv("LOG_LEVEL",
+ DEFAULT_LOG_LEVEL).upper()
try:
# py311+ only
level_mapping = logging.getLevelNamesMapping()
- self.axolotl_level = level_mapping[axolotl_log_level]
- self.other_level = level_mapping[other_log_level]
- except AttributeError:
- # For py311-, use getLevelName directly
- self.axolotl_level = logging.getLevelName(axolotl_log_level)
- self.other_level = logging.getLevelName(other_log_level)
+ self.axolotl_level = level_mapping.get(axolotl_log_level)
+ self.other_level = level_mapping.get(other_log_level)
+ if self.axolotl_level is None or self.other_level is None:
+ raise KeyError
+ except (AttributeError, KeyError):
+ # Fallback for py311- or unknown names
+ self.axolotl_level = logging.getLevelName(axolotl_log_level)
+ self.other_level = logging.getLevelName(other_log_level)
+ if not isinstance(self.axolotl_level, int) or not isinstance(self.other_level, int):
+ raise ValueError(
+ f"Invalid log level(s): {axolotl_log_level}, {other_log_level}"
+ )This makes the logic robust across Python versions and user input.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| axolotl_log_level = os.getenv("AXOLOTL_LOG_LEVEL", DEFAULT_AXOLOTL_LOG_LEVEL) | |
| other_log_level = os.getenv("LOG_LEVEL", DEFAULT_LOG_LEVEL) | |
| try: | |
| # py311+ only | |
| level_mapping = logging.getLevelNamesMapping() | |
| self.axolotl_level = level_mapping[axolotl_log_level] | |
| self.other_level = level_mapping[other_log_level] | |
| except AttributeError: | |
| # For py311-, use getLevelName directly | |
| self.axolotl_level = logging.getLevelName(axolotl_log_level) | |
| self.other_level = logging.getLevelName(other_log_level) | |
| axolotl_log_level = os.getenv("AXOLOTL_LOG_LEVEL", | |
| DEFAULT_AXOLOTL_LOG_LEVEL).upper() | |
| other_log_level = os.getenv("LOG_LEVEL", | |
| DEFAULT_LOG_LEVEL).upper() | |
| try: | |
| # py311+ only | |
| level_mapping = logging.getLevelNamesMapping() | |
| self.axolotl_level = level_mapping.get(axolotl_log_level) | |
| self.other_level = level_mapping.get(other_log_level) | |
| if self.axolotl_level is None or self.other_level is None: | |
| raise KeyError | |
| except (AttributeError, KeyError): | |
| # Fallback for py311- or unknown names | |
| self.axolotl_level = logging.getLevelName(axolotl_log_level) | |
| self.other_level = logging.getLevelName(other_log_level) | |
| if not isinstance(self.axolotl_level, int) or not isinstance(self.other_level, int): | |
| raise ValueError( | |
| f"Invalid log level(s): {axolotl_log_level}, {other_log_level}" | |
| ) |
🤖 Prompt for AI Agents
In src/axolotl/logging_config.py around lines 28 to 40, the current log-level
resolution can raise KeyError if environment variables have unexpected casing or
unknown values, and fallback returns strings causing TypeErrors later. To fix
this, normalize the environment variable values to uppercase before lookup, use
a safe method to get numeric log levels that returns a default or None for
unknown levels, and handle these cases explicitly to avoid exceptions. This
ensures robust, case-insensitive log level parsing across Python versions.
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
winglian
approved these changes
Jun 18, 2025
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Reported on Discord by Caitlyn and artem , the
logging.logging.getLevelNamesMappingwas added in 311 https://docs.python.org/3/library/logging.html#logging.getLevelNamesMappingThis PR adds a try catch to handle for Py310. However, we would now recommend Py311 as that's where we test
Motivation and Context
How has this been tested?
Screenshots (if appropriate)
Types of changes
Social Handles (Optional)
Summary by CodeRabbit
Documentation
Bug Fixes