Loggin config

[canismarko]

Description

There is a call to the configure_logging() function at the top-level of bits/__init__.py. This means that any import of bits automatically sets up the logging configuration.

Part of the logging configuration is to monkeypatch the logger with a bsdev() method. If configure_logging() is to be optional, module-level calls to logger.bsdev() will not work since this method will not be available.

This PR moves the configure_logging() call to the startup.py file. It also patches the module-level calls to logger.bsdev() to default to logger.debug() if bsdev is not available.

Fixes # (issue)

• Provide a way to suppress logging configuration during import #169

Type of change

Choose which options apply, and delete the ones which do not apply.

• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Code maintenance/cleanup

[prjemian]

• preserves original intent to start logging early in startup
• falls back to logging.debug when bsdev level is undefined

[prjemian]

Needs these style items to be resolved:

src/apsbits/core/__init__.py:11:5: D103 Missing docstring in public function
src/apsbits/demo_instrument/startup.py:16:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:20:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:21:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:24:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:26:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:27:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:28:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:29:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:30:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:33:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:34:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:37:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:38:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:39:1: E402 Module level import not at top of file
src/apsbits/demo_instrument/startup.py:40:1: E402 Module level import not at top of file
Found 17 errors (1 fixed, 16 remaining).

[canismarko]

I made the lint checker happy in startup.py by suppressing errors about module level imports not being at the top of the file. I don't think there's a way to satisfy that rule without setting up logging after all the imports have been done (which is not what we wanted). Open to any other suggestions.

@MDecarabas, mind taking a look?

[prjemian]

According to argo/sonnet46:

These are not valid ruff directives — they have no effect. The correct per-file suppression is # ruff: noqa: E402 at the top (already added), which suppresses E402 for the entire file. The disable/enable block syntax doesn't exist in ruff. These stale comments should be removed.

[canismarko]

Well, the ruff documentation disagrees. Though line 12 # ruff: noqa: E402 is redundant, so how about I remove that and we keep the scop of the error suppression as tight as possible?

What does Pete the human think?

[canismarko]

The latest version of ruff honors the block-level suppression # ruff: disable[E402]. The version that is pinned in the pre-commit yaml file (0.4.8) is from June 2024 and doesn't have this feature.

I can upgrade ruff to 0.15.10, or stay at 0.4.8 and use the file-level skips to skip more checking in startup.py? What would you prefer?

[prjemian]

argo/sonnet46 summarizes with this recommendation:

The main actionable issue is removing the ineffective # ruff: disable[E402] / # ruff: enable[E402] lines in startup.py — they're dead code and misleading. The # ruff: noqa: E402 at the top of the file already handles suppression for the whole file. Everything else looks good.

One of my goals when logging was first added was to help beamline staff identify where problems occurred in their code. This means logging has to be enabled as soon as possible on startup. Your edits here adhere to that goal.

[prjemian]

Speaking, as a human, great. (as a response to your question which has not appeared before my "reply")

@canismarko commented on this pull request.

In [src/apsbits/demo_instrument/startup.py](https://github.com/BCDA-APS/BITS/pull/185#discussion_r3080352681):

>  # Standard Library Imports
 import logging
 from pathlib import Path
 
+# Needs to run before import other apsbits modules
+from apsbits.utils.logging_setup import configure_logging  # isort:skip
+
+configure_logging()
+
+# ruff: disable[E402]

Well, the [ruff documentation](https://docs.astral.sh/ruff/linter/#block-level) disagrees. Though line 12 # ruff: noqa: E402 is redundant, so how about I remove that and we keep the scop of the error suppression as tight as possible?

What does Pete the human think?

[prjemian]

@MDecarabas Your opinion on this?

[MDecarabas]

I made the lint checker happy in startup.py by suppressing errors about module level imports not being at the top of the file. I don't think there's a way to satisfy that rule without setting up logging after all the imports have been done (which is not what we wanted). Open to any other suggestions.

@MDecarabas, mind taking a look?

Is there a reason why we need to do this work around. Since everything is a function and nothing should run on import then we can have the imports first and then the function. The goal of the startup.py is to not obfuscate any code, and for the startup process to be legible as a story. If things are running on import then that makes it harder to trace, and that is something we have to fix as opposed to work around.

[canismarko]

Is there a reason why we need to do this work around.

I think the idea was that setting up logging first would make it easier to diagnose problems while importing other things.

I have no objections to following the linting rules and putting all the imports at the top like you suggested. Special cases aren't special enough to break the rules.

[MDecarabas]

Is there a reason why we need to do this work around.

I think the idea was that setting up logging first would make it easier to diagnose problems while importing other things.

I have no objections to following the linting rules and putting all the imports at the top like you suggested. Special cases aren't special enough to break the rules.

I think this is best in my opinion. Also if nothing gets loaded on import there is nothing to diagnose about the import 😉

[canismarko]

I restructured startup.py so all the imports are up top. That would be how I would do it in a vacuum, but I am happy with either structure. I'll leave that decision up to you, @prjemian and @MDecarabas.

[prjemian]

Ok by me

[MDecarabas]

I think this is more so what I was thinking when you guys were talking about doing the logging. Pretty sure now everything gets loaded in startup.py and there is no need for the imports to run in any specific fashion.

[MDecarabas]

I also think the init inside core can probably be removed now.

[MDecarabas]

a bit cleaner

[prjemian]

Two green check. @canismarko Press the big green button

[canismarko]

Done, thank you!