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

Fix DeprecationWarning in pythonjsonlogger>=3.1.0 #3120

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

patrick-kidger
Copy link

@patrick-kidger patrick-kidger commented Feb 8, 2025

pythonjsonlogger.jsonlogger is deprecated as of pythonjsonlogger>=3.1.0:

https://github.com/nhairs/python-json-logger/releases/tag/v3.1.0

https://github.com/nhairs/python-json-logger/blob/db04a0f9066cc331f8d6177f828fe073c7b2a4cc/src/pythonjsonlogger/jsonlogger.py

Why are the changes needed?

This is a warning of an impending deprecation.

What changes were proposed in this pull request?

To not import a deprecated module.

How was this patch tested?

Running them in CI here. Let's find out if they pass.

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Summary by Bito

Updated import statement in flytekit/loggers.py to handle pythonjsonlogger>=3.1.0 deprecation warning. Implemented conditional import logic to check for new module path (pythonjsonlogger.json) with fallback to legacy path (pythonjsonlogger.jsonlogger). Change ensures backward compatibility across different versions of pythonjsonlogger package.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 1

Copy link

welcome bot commented Feb 8, 2025

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@flyte-bot
Copy link
Contributor

flyte-bot commented Feb 8, 2025

Code Review Agent Run #bb460b

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 9d69b1b..6fc497e
    • flytekit/loggers.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Contributor

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Bug Fix - Fix Deprecated Module Import

loggers.py - Updated pythonjsonlogger import to handle module renaming in version 3.1.0

Copy link

codecov bot commented Feb 8, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.61%. Comparing base (1eb6743) to head (6fc497e).

Files with missing lines Patch % Lines
flytekit/loggers.py 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3120      +/-   ##
==========================================
+ Coverage   79.60%   79.61%   +0.01%     
==========================================
  Files         203      203              
  Lines       21625    21628       +3     
  Branches     2788     2789       +1     
==========================================
+ Hits        17214    17220       +6     
- Misses       3629     3631       +2     
+ Partials      782      777       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@patrick-kidger
Copy link
Author

I think the failing tests seem to be unrelated to my change? I'm not sure what's going on there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants