Skip to content

Conversation

jpcornil-git
Copy link
Contributor

@jpcornil-git jpcornil-git commented Oct 4, 2025

Remove default logging.basicConfig in Communication class init; it has no effect and should be set once in main.py anyway.

The DEBUG log level should not be set in the root log as it may be used as the default for other modules and thus unnecessarily increase verbosity.

I added a setlevel(DEBUG) to the Communication's logger to keep the intended DEBUG level there but this should probably be set to INFO as well (or removed, added when required/while debugging).

Summary by CodeRabbit

  • Refactor
    • Switched to per-instance logging, improving isolation and preventing unintended changes to global logging behavior. This results in clearer, component-specific logs.
  • Chores
    • Adjusted default file logging level from DEBUG to INFO, reducing noise in log files while preserving existing formats and timestamps. Users should see cleaner logs with essential information prioritized, making routine monitoring easier and troubleshooting more focused without altering runtime behavior.

Remove default logging.basicConfig in Communication class init; it has
no effect and should be set once in main.py anyway.
Copy link
Contributor

coderabbitai bot commented Oct 4, 2025

Walkthrough

Logging configuration was adjusted in two places: communication.py now sets an instance-level logger to DEBUG instead of configuring the root logger, and main.py changes the file handler log level from DEBUG to INFO. No control flow or public API changes.

Changes

Cohort / File(s) Summary
Logging configuration adjustments
src/main/python/main/ayab/engine/communication.py, src/main/python/main/main.py
In communication.py, removed global logging.basicConfig(level=logging.DEBUG) and set self.logger.setLevel(logging.DEBUG) per instance. In main.py, updated the file handler log level from DEBUG to INFO; other logger settings unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately and concisely reflects the primary change of adjusting the default log level to INFO, which aligns directly with the modifications in main.py and the PR objectives without unnecessary detail.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

sonarqubecloud bot commented Oct 4, 2025

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/main/python/main/ayab/engine/communication.py (1)

76-76: Reconsider DEBUG as the default level.

Setting the Communication logger to DEBUG by default creates a special case where this module's DEBUG messages appear on console while other modules are filtered at INFO. As noted in the PR description, this level might be better set to INFO to align with the root logger, or applied only when specifically needed for debugging.

Consider:

  • Whether Communication truly needs DEBUG logging in normal operation
  • If DEBUG is necessary for development, using an environment variable or configuration option to enable it selectively
  • Setting to INFO to match the root logger's default behavior

Apply this diff if DEBUG logging should not be enabled by default:

-        self.logger.setLevel(logging.DEBUG)
+        # Inherits INFO level from root logger; set to DEBUG only when debugging

Or configure it dynamically:

-        self.logger.setLevel(logging.DEBUG)
+        # Set to DEBUG only if needed for debugging
+        import os
+        if os.getenv('AYAB_DEBUG'):
+            self.logger.setLevel(logging.DEBUG)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6c62a69 and be83099.

📒 Files selected for processing (2)
  • src/main/python/main/ayab/engine/communication.py (1 hunks)
  • src/main/python/main/main.py (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Create and upload Linux AppImage release
  • GitHub Check: Create and upload Windows build
  • GitHub Check: run-pytest
  • GitHub Check: Create and upload Mac OSX build
🔇 Additional comments (1)
src/main/python/main/main.py (1)

56-56: Verify alignment with console handler level.

Changing the root logger level to INFO is consistent with the PR objectives. However, the console handler on line 61 remains at DEBUG level. With the root logger at INFO, DEBUG messages from most loggers will be filtered out before reaching the console handler, making its DEBUG level largely ineffective.

Only loggers that explicitly override their level to DEBUG (like Communication in the other file) will have DEBUG messages reach the console.

Consider whether the console handler should also be set to INFO for clarity, or document why specific loggers need DEBUG console output while the default is INFO.

Based on learnings

@jonathanperret
Copy link
Contributor

jonathanperret commented Oct 4, 2025

Good catch on the extra basicConfig, I agree it does not make sense (and per the python docs it does nothing except maybe cause a race condition in some cases).

In fact storing a logger in self.logger is explicitly discouraged in the docs: https://docs.python.org/3/howto/logging-cookbook.html#using-loggers-as-attributes-in-a-class-or-passing-them-as-parameters — it would be better to move to the pythonic approach of a module-level logger, though the wrong pattern appears to be used multiple times in the current code so that would be a wider change.

In any case, configuring the logger in the Communication constructor is not the right place to do it because it will override any configuration that may have been set up in the proper place (i.e. in main or a configuration file), and reset it on every instance creation (since loggers are singletons, logging.getlogger will always return the same instance for a given name).

A better alternative would be to add a call to logging.getLogger("…Communication").setLevel(DEBUG) in main.py right after the basicConfig call). A generalized version of this would be to call dictConfig and configure the desired level for any logger of interest.

Regarding the default logging level, I agree that DEBUG for everything by default probably isn’t right. However, until there is a user-accessible way to enable increased logging I’d rather err on the side of logging too much, in the interest of being able to diagnose issues from the field.

Keeping just the Communication logger at DEBUG and moving everything else back to INFO may well keep sufficient logging, but I’d want to look at which messages we’d be removing from the logs by doing so to assess whether it is OK to make the change.

@jpcornil-git
Copy link
Contributor Author

jpcornil-git commented Oct 5, 2025

You could do something like illustrated below if you want to conditionally enable DEBUG level without a UI/preference file setting, in main.py:

...
        logging.basicConfig(
            filename=logfile,
            level=logging.DEBUG if os.getenv('AYAB_DEBUG') else logging.INFO,
            format="%(asctime)s %(name)-8s %(levelname)-8s %(message)s",
            datefmt="%y-%m-%d %H:%M:%S",
        )
        console = logging.StreamHandler()
...

=> loglevel is INFO unless you define AYAB_DEBUG in your environment

List of logging/logger.debug calls with suggested loglevel modification (first column)

$ grep -r 'logging.debug' src/*
  src/main/python/main/ayab/ayab.py:        logging.debug("Quitting")
  src/main/python/main/ayab/utils.py:    logging.debug(f"MessageBox {message_type}: '{message}'")
  src/main/python/main/ayab/gui_fsm.py:        self.NO_IMAGE.entered.connect(lambda: logging.debug("Entered state NO_IMAGE"))
  src/main/python/main/ayab/gui_fsm.py:            lambda: logging.debug("Entered state TESTING_NO_IMAGE")
  src/main/python/main/ayab/gui_fsm.py:            lambda: logging.debug("Entered state CONFIGURING")
  src/main/python/main/ayab/gui_fsm.py:        self.CHECKING.entered.connect(lambda: logging.debug("Entered state CHECKING"))
  src/main/python/main/ayab/gui_fsm.py:        self.KNITTING.entered.connect(lambda: logging.debug("Entered state KNITTING"))
  src/main/python/main/ayab/gui_fsm.py:        self.TESTING.entered.connect(lambda: logging.debug("Entered state TESTING"))


$ grep -r 'logger.debug' src/*
  src/main/python/main/ayab/firmware_flash.py:        self.__logger.debug("port " + str(self.port))
  src/main/python/main/ayab/firmware_flash.py:        self.__logger.debug(exec_command)
  src/main/python/main/ayab/version_checker.py:        self.logger.debug(
  src/main/python/main/ayab/version_checker.py:        self.logger.debug("Getting %s", latest_relase_url)
  src/main/python/main/ayab/version_checker.py:            self.logger.debug(
  src/main/python/main/ayab/version_checker.py:                self.logger.debug("Latest version is %s at %s", latest_version, url)
  src/main/python/main/ayab/version_checker.py:            self.logger.debug("Cleaning up")
W src/main/python/main/ayab/engine/communication.py:        self.logger.debug("unknown message: ")  # drop crlf
  src/main/python/main/ayab/engine/control.py:        self.logger.debug(msg)
  src/main/python/main/ayab/engine/control.py:        self.logger.debug("sending blank line as final line=%d", requested_line)
  src/main/python/main/ayab/engine/mode.py:            control.logger.debug("COLOR " + str(color))
I src/main/python/main/ayab/engine/engine_fsm.py:        control.logger.debug("State CONNECT")
  src/main/python/main/ayab/engine/engine_fsm.py:        control.logger.debug("Port name: " + control.portname)
I src/main/python/main/ayab/engine/engine_fsm.py:        control.logger.debug("State VERSION_CHECK")
I src/main/python/main/ayab/engine/engine_fsm.py:                control.logger.debug("State INIT")
I src/main/python/main/ayab/engine/engine_fsm.py:                    control.logger.debug("State REQUEST_TEST")
I src/main/python/main/ayab/engine/engine_fsm.py:                    control.logger.debug("State REQUEST_START")
I src/main/python/main/ayab/engine/engine_fsm.py:                control.logger.debug("State CONFIRM_START")
W src/main/python/main/ayab/engine/engine_fsm.py:                control.logger.debug(
I src/main/python/main/ayab/engine/engine_fsm.py:                control.logger.debug("State RUN_KNIT")
I src/main/python/main/ayab/engine/engine_fsm.py:        control.logger.debug("State CONFIRM_TEST")
I src/main/python/main/ayab/engine/engine_fsm.py:                control.logger.debug("State RUN_TEST")
I src/main/python/main/ayab/engine/engine_fsm.py:            control.logger.debug("State DISCONNECT")
I src/main/python/main/ayab/engine/engine_fsm.py:        control.logger.debug("State FINISHED")
  src/main/python/main/ayab/engine/engine.py:        self.__logger.debug(self.config.as_dict())

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