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

Do not overwrite default warning filter or formatter #3514

Merged
merged 1 commit into from Feb 20, 2024
Merged

Conversation

connortann
Copy link
Collaborator

@connortann connortann commented Feb 19, 2024

Overview

Closes #3287

  • Removes modifications of the default warning formatter
  • Removes the global supression of numba deprecation warnings

Discussion

As per the issue above, the unexpected import-time side-effects can be surprising and cause nefarious bugs. As a general rule, importable libraries should avoid modifying or monkeypatching other libraries.

One consequence of this change is that the warnings produced by SHAP will be the default verbose formatting, rather than the prettier "message-only" format:

image

However, whilst I admittedly do prefer the concise formatting, I don't think this preference for formatting has anything to do with shap specifically, so it should be out-of-scope for the shap package to fiddle with generic python behaviour like this. Users are still able to modify the handling of warnings as they see fit.

@connortann connortann marked this pull request as ready for review February 19, 2024 17:20
Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b3573bd) 59.20% compared to head (d870e34) 59.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3514      +/-   ##
==========================================
+ Coverage   59.20%   59.23%   +0.03%     
==========================================
  Files          90       90              
  Lines       12731    12727       -4     
==========================================
+ Hits         7537     7539       +2     
+ Misses       5194     5188       -6     

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

@connortann connortann added this to the 0.45.0 milestone Feb 19, 2024
@connortann
Copy link
Collaborator Author

connortann commented Feb 20, 2024

As an alternative, we could preserve the current warning formatting with a function like this that tracks and restores the original global state. This would involve replacing all instances of warnings.warn.

def pretty_warn(msg: str) -> None:
    """Raise a UserWarning with a custom formatter that just shows the message"""
    previous_formatter = warnings.formatwarning
    warnings.formatwarning = lambda msg, *args, **kwargs: str(msg) + '\n'
    warnings.warn(msg)
    # Restore previous global state
    warnings.formatwarning = previous_formatter

I'm actually inclined to just leave the default warning formatter in place for simplicity. Open to suggestions. Whatever we do, it seems like we should not permanently override the global state.

Copy link
Collaborator

@CloseChoice CloseChoice left a comment

Choose a reason for hiding this comment

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

very nice! LGTM

@connortann
Copy link
Collaborator Author

Just to clarify, @CloseChoice do you have any preferences regarding removing the formatting tweak entirely vs implementing the "pretty_warn" func?

@CloseChoice
Copy link
Collaborator

I think removing it entirely as you did is totally fine. I wouldn't spend time to find a solution that keeps the old formatting AND doesn't overwrite the global warnings. If you have a quick solution please go ahead, but I would not spend even 15 minutes on that, tbh. If the formatting disturbs us in any way we can still open another issue and find a solution for that.

@connortann connortann merged commit 584fb9c into master Feb 20, 2024
16 checks passed
@connortann connortann deleted the fix/3287 branch February 20, 2024 11:50
@connortann connortann added the enhancement Indicates new feature requests label Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Do not overwrite global warning formatter
2 participants