Skip to content

ENH: Add callback exception deduplication filter#39

Merged
ZLLentz merged 15 commits into
pcdshub:masterfrom
ZLLentz:callback-dedup
Nov 8, 2021
Merged

ENH: Add callback exception deduplication filter#39
ZLLentz merged 15 commits into
pcdshub:masterfrom
ZLLentz:callback-dedup

Conversation

@ZLLentz
Copy link
Copy Markdown
Member

@ZLLentz ZLLentz commented Nov 5, 2021

Description

Motivation and Context

  • The log message this targets is very likely to be a source of spam
  • The configuration options give us choices for later when we apply these to hutch-python
  • Ideas for using the counter: how many log messages were skipped during the scan? display the number of hidden messages in the prompt?
  • Might be redundant with ENH: record and optionally filter noisy ophyd loggers hutch-python#298 but I think it's important to catch these messages after 1 or 0 showings instead of waiting to trip the circuit breaker

How Has This Been Tested?

Added unit tests

Where Has This Been Documented?

Just docstrings for now

@ZLLentz ZLLentz requested review from ZryletTC and klauer November 5, 2021 23:57
@ZLLentz
Copy link
Copy Markdown
Member Author

ZLLentz commented Nov 5, 2021

I requested a review but you should wait till next week because it's 5pm on a Friday

Copy link
Copy Markdown
Contributor

@klauer klauer left a comment

Choose a reason for hiding this comment

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

I like this approach and the resulting implementation much more than the stuff I've put together for hutch-python. Perhaps we need to revisit that PR in light of this one.

On that note - we should configure logging.yml in hutch-python to instantiate this directly.

Only pending question is "what do we do about Python versions?"

Comment thread pcdsutils/log.py Outdated
Comment thread pcdsutils/tests/test_logging.py Outdated
assert len(target_records) == 1, f"Too many records! {cnt=}"
record = target_records[0]
if not filtered or cnt == 0:
assert record.levelno == logging.ERROR, f"{filtered=}, {cnt=}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having seen the syntaxerror that flake8 was raising on this file in our discussion last week, it gave a bit of extra time for me to think about the implications here.

While in the test suite, pcdsutils.tests is still part of pcdsutils. typhos (a non PCDS-specific package) relies on pcdsutils, and some other facilities are or may start to use it.

Given that, I think it'd be wise to backpedal a bit on 3.9+-only; pcdsdevices and hutch-python are fine, but less so our core libraries.

It's possible that downstream users just wouldn't care - or it's not such a big deal - but I want to raise this now before we get much further using 3.9+ features. Thoughts?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh that's a good point, I didn't even consider the typhos/dependencies angle. How does a follow-up PR to backpedal to 3.7+ before any tagging sound?

Copy link
Copy Markdown
Member Author

@ZLLentz ZLLentz Nov 8, 2021

Choose a reason for hiding this comment

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

numpy drop schedule for reference:
On next release, drop support for Python 3.5 (initially released on Sep 13, 2015)
On Jan 07, 2020 drop support for NumPy 1.14 (initially released on Jan 06, 2018)
On Jun 23, 2020 drop support for Python 3.6 (initially released on Dec 23, 2016)
On Jul 23, 2020 drop support for NumPy 1.15 (initially released on Jul 23, 2018)
On Jan 13, 2021 drop support for NumPy 1.16 (initially released on Jan 13, 2019)
On Jul 26, 2021 drop support for NumPy 1.17 (initially released on Jul 26, 2019)
On Dec 22, 2021 drop support for NumPy 1.18 (initially released on Dec 22, 2019)
On Dec 26, 2021 drop support for Python 3.7 (initially released on Jun 27, 2018)
On Jun 21, 2022 drop support for NumPy 1.19 (initially released on Jun 20, 2020)
On Apr 14, 2023 drop support for Python 3.8 (initially released on Oct 14, 2019)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2023 for 3.8 - looks to be a while ☹️

I think merging and addressing with a follow-up PR would be just fine.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

uh, this turned out to be really easy to pull back to 3.7- I'll scrap my new branch and just drop the commits into this one

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the future annotations import must be doing some magic

@klauer
Copy link
Copy Markdown
Contributor

klauer commented Nov 8, 2021

Awesome. Merge at will!

@ZLLentz ZLLentz merged commit 73407c1 into pcdshub:master Nov 8, 2021
@ZLLentz ZLLentz deleted the callback-dedup branch November 8, 2021 22:19
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