Moved logger to common #45

AshishA26 · 2024-08-11T00:51:22Z

Moved logger to common.

PR must be merged before the PR in computer-vision-python UWARG/computer-vision-python#194.

TongguangZhang

reviewed

TongguangZhang · 2024-08-17T17:31:56Z

read_yaml/yaml.py

rename to read_yaml.py

maxlou05

Added comments. Also, would need to rebase to main after Python 3.11 fix is merged.

maxlou05 · 2024-08-23T19:27:18Z

logger/modules/logger_setup_main.py

+    logging_path = pathlib.Path(log_directory_path, start_time)
+
+    # Create logging directory
+    pathlib.Path(log_directory_path).mkdir(exist_ok=True)


Perhaps we want to have parent=True in case parent directories need to be created as well

Edit: Just set parent=True in the next line, and delete this one

logger/modules/logger_setup_main.py

maxlou05 · 2024-08-23T19:56:18Z

logger/modules/logger.py

+# pylint: disable-next=unused-import
+import types
+
+from read_yaml.modules import read_yaml


This warrants some discussion. Since common is meant to be imported as a submodule, everything needs to be a relative import. This code will fail when being used as a submodule. However, because Python does not support sibling module imports, (ie you cannot import read_yaml from logger since they're both at the root level) this causes some problems. We must ensure that all top-level modules (like logger) are independent and do not depend upon anything other top-level modules. So we must put yaml_tools inside of logger. Then there is the question "what if another modules wants to use yaml tools?". (eg. camera_qr_example.py would not work if you tried to do py -m modules.common.camera_qr_example after importing common into airside)

You can keep it as it is and manually edit all the imports when you use common in another project (since now common is no longer the root). (eg changing from camera.modules import ... to from .camera.modules import ... after initializing the common submodule, seems annoying and lot of effort)

We can put everything in another folder (like common/tools), but then all of our imports will have an extra .tools, which may look ugly. (eg py -m modules.common.tools.camera_qr_example)

We copy yaml_tools in all of the modules that needs it (this is sort of how dronekit is implemented currently, except only one of the modules, mavlink, needs dronekit)

Feel free to discuss, but this is an organizational issue we need to resolve

maxlou05 · 2024-08-23T20:15:45Z

logger/test_logger_integration.py

+from read_yaml.modules import read_yaml
+
+
+def main() -> int:


Can this not be tested with pytest? If all logger does is write to stdout and stderr, you should be able to test that using pytest. If so, I would recommend making this a pytest because at least I would be too lazy to manually run all these tests lmao. (I'm not too familiar with python's logging module)

maxlou05

Reviewed some more

maxlou05 · 2024-08-25T15:03:18Z

config_logger.yaml

@@ -0,0 +1,5 @@
+logger:


It would be nice to have a link to documentation on the format strings

maxlou05 · 2024-08-25T15:11:07Z

logger/modules/logger.py

+from read_yaml.modules import read_yaml
+
+
+CONFIG_FILE_PATH = pathlib.Path("config_logger.yaml")


I have not checked, but I believe relative paths like this depend on where the command is executed. So similar to the relative imports issue, this would work if run from py -m common... (since you are in common folder) but will probably fail when running from py -m modules.common... (since you are in airside folder). Someone should test this, and I think using the __file__ variable can help in this situation.

maxlou05 · 2024-08-25T15:21:24Z

logger/modules/logger_setup_main.py

+    logging_path = pathlib.Path(log_directory_path, start_time)
+
+    # Create logging directory
+    pathlib.Path(log_directory_path).mkdir(exist_ok=True)


Edit: Just set parent=True in the next line, and delete this one

maxlou05 · 2024-08-25T15:29:17Z

logger/modules/logger.py

+            entries = os.listdir(log_directory_path)
+
+            if len(entries) == 0:
+                print("ERROR: Must create a new log directory for this run before starting logger")


Rather than "log directory", which is confusing with the base "directory_path" from the yaml, we can say "No previous logging sessions available"

maxlou05 · 2024-08-25T15:35:09Z

logger/test_logger_unit.py

+import inspect
+import pytest
+
+from logger.modules import logger


Same: relative path

maxlou05 · 2024-08-25T15:35:16Z

read_yaml/test_read_yaml.py

+
+import pathlib
+
+from read_yaml.modules import read_yaml


Same: relative path

maxlou05

Reviewed

logger/modules/config_logger.yaml

maxlou05 · 2024-08-26T14:39:52Z

logger/modules/logger_setup_main.py

+    assert main_logger is not None
+
+    frame = inspect.currentframe()
+    main_logger.info(f"{MAIN_LOGGER_NAME} logger initialized", frame)


Do we still not want to use the custom name? If so I'd rather just write "main" straight into the string so it's less confusing, because this looks like the default name if the user didn't provide one.

maxlou05 · 2024-08-26T14:44:34Z

logger/test_logger.py

+@pytest.fixture
+def logger_instance_to_file_enabled() -> logger.Logger:  # type: ignore
+    """
+    Returns a logger with logging to file disabled.


Typo in docstring: enabled not disabled?

logger/test_logger.py

maxlou05 · 2024-08-26T14:50:47Z

logger/modules/logger_setup_main.py

+def setup_main_logger(
+    config: "dict", main_logger_name: str = MAIN_LOGGER_NAME
+) -> "tuple[bool, logger.Logger | None, pathlib.Path | None]":


Why do we choose to force main logger to log to a file? It should be fine as long as the directory exists, and not the specific main log file?

logger/read_yaml/test_read_yaml.py

maxlou05 · 2024-08-26T15:13:55Z

logger/modules/logger.py

+    def debug(self, message: str, frame: "types.FrameType | None") -> None:
+        """
+        Logs a debug level message.
+        """
+        message = self.message_and_metadata(message, frame)
+        self.logger.debug(message)
+
+    def info(self, message: str, frame: "types.FrameType | None") -> None:
+        """
+        Logs an info level message.
+        """
+        message = self.message_and_metadata(message, frame)
+        self.logger.info(message)
+
+    def warning(self, message: str, frame: "types.FrameType | None") -> None:
+        """
+        Logs a warning level message.
+        """
+        message = self.message_and_metadata(message, frame)
+        self.logger.warning(message)
+
+    def error(self, message: str, frame: "types.FrameType | None") -> None:
+        """
+        Logs an error level message.
+        """
+        message = self.message_and_metadata(message, frame)
+        self.logger.error(message)
+
+    def critical(self, message: str, frame: "types.FrameType | None") -> None:
+        """
+        Logs a critical level message.
+        """
+        message = self.message_and_metadata(message, frame)
+        self.logger.critical(message)


I propose that rather than asking the user to pass in a frame, we use inspect in the Logger itself, and we can use frame.f_back to find the caller's frame, which is the one we are interested in. Then we can just have frame be a boolean option that defaults to true, since it seems to be enabled by default on most stack traces and loggers.

maxlou05 · 2024-08-26T15:17:40Z

logger/test_logger.py

+        test_message = "test message"
+        logger_instance_to_file_enabled.debug(test_message, None)
+        logger_instance_to_file_enabled.info(test_message, None)
+        logger_instance_to_file_enabled.warning(test_message, None)
+        logger_instance_to_file_enabled.error(test_message, None)
+        logger_instance_to_file_enabled.critical(test_message, None)


It would be nice if one had no frame and this one does have frame info

maxlou05 · 2024-08-26T15:20:11Z

logger/test_logger.py

+        """
+        test_message = "test message"
+
+        logger_instance_to_file_disabled.debug(test_message, None)


Same for all of these, would be nice to log twice, one with frame and one without

maxlou05 · 2024-08-26T15:20:53Z

requirements.txt

Also could you please remove line 17-18 (the pymavlink) since it's now moved to the submodule. Thanks!

maxlou05

Reviewed

maxlou05 · 2024-08-27T21:51:46Z

logger/modules/logger_setup_main.py

+    assert main_logger is not None
+
+    frame = inspect.currentframe()
+    main_logger.info(f"{main_logger_name} logger initialized", frame)


frame should be true/false, since we changed that.

maxlou05 · 2024-08-27T21:52:54Z

logger/modules/logger.py

+    def debug(self, message: str, log_with_frame_info: bool) -> None:
+        """
+        Logs a debug level message.
+        """
+        if log_with_frame_info:
+            logger_frame = inspect.currentframe()
+            caller_frame = logger_frame.f_back
+            message = self.message_and_metadata(message, caller_frame)
+        self.logger.debug(message)
+
+    def info(self, message: str, log_with_frame_info: bool) -> None:
+        """
+        Logs an info level message.
+        """
+        if log_with_frame_info:
+            logger_frame = inspect.currentframe()
+            caller_frame = logger_frame.f_back
+            message = self.message_and_metadata(message, caller_frame)
+        self.logger.info(message)
+
+    def warning(self, message: str, log_with_frame_info: bool) -> None:
+        """
+        Logs a warning level message.
+        """
+        if log_with_frame_info:
+            logger_frame = inspect.currentframe()
+            caller_frame = logger_frame.f_back
+            message = self.message_and_metadata(message, caller_frame)
+        self.logger.warning(message)
+
+    def error(self, message: str, log_with_frame_info: bool) -> None:
+        """
+        Logs an error level message.
+        """
+        if log_with_frame_info:
+            logger_frame = inspect.currentframe()
+            caller_frame = logger_frame.f_back
+            message = self.message_and_metadata(message, caller_frame)
+        self.logger.error(message)
+
+    def critical(self, message: str, log_with_frame_info: bool) -> None:
+        """
+        Logs a critical level message.
+        """
+        if log_with_frame_info:
+            logger_frame = inspect.currentframe()
+            caller_frame = logger_frame.f_back
+            message = self.message_and_metadata(message, caller_frame)
+        self.logger.critical(message)


I think it would be nicer to have log_with_frame_info default to True? Up to you tho

maxlou05 · 2024-08-27T21:53:18Z

logger/modules/logger_setup_main.py

+    logging_path = pathlib.Path(log_directory_path, start_time)
+
+    # Create logging directory
+    pathlib.Path(log_directory_path).mkdir(exist_ok=True)


maxlou05

Approved!

AshishA26 marked this pull request as draft August 11, 2024 00:51

AshishA26 requested review from Xierumeng, TongguangZhang and DylanFinlay August 11, 2024 00:51

AshishA26 marked this pull request as ready for review August 16, 2024 23:57

AshishA26 mentioned this pull request Aug 16, 2024

Move logger to common UWARG/computer-vision-python#194

Closed

TongguangZhang reviewed Aug 17, 2024

View reviewed changes

read_yaml/yaml.py Outdated

Copy link

Contributor

TongguangZhang Aug 17, 2024

Choose a reason for hiding this comment

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

rename to read_yaml.py

AshishA26 reacted with thumbs up emoji

AshishA26 requested a review from TongguangZhang August 18, 2024 05:44

maxlou05 reviewed Aug 23, 2024

View reviewed changes

maxlou05 mentioned this pull request Aug 23, 2024

Fix mavutil imports #49

Merged

maxlou05 reviewed Aug 25, 2024

View reviewed changes

AshishA26 and others added 15 commits August 25, 2024 14:06

Moved logger

04d1b3f

Moved yaml

03ef3a1

Made folder

039002c

Fix error

d2fe56c

fixed folders

8160db6

logger unit tests

4de4013

logger integration tests

cb7b8ee

unit test for read_yaml

ed9e8c3

black formatting changes

0725b4a

return annotations

49a851d

log dir path config changed

01bc7c8

updated requirements.txt

10a54f3

changed file paths

a269c19

move read_yaml to logger

bf87453

relative imports

57233f5

TongguangZhang force-pushed the move_logger_to_common branch from b7c4a8f to 57233f5 Compare August 25, 2024 18:07

TongguangZhang added 3 commits August 25, 2024 21:18

fixed imports

1549dcf

fixed paths and added config_logger

34837ca

added ignore to pyproject.toml

1c899fe

TongguangZhang added 8 commits August 25, 2024 23:15

relative path for logger setup main

1a02750

fixed test read yaml paths

903ce46

explicit log to stdout

1139f8c

wrote tests

88d662c

black formatting changes

844e75e

type annotation

ae6b2ad

pylint errors

fcede5a

testing bug

b29a7c2

maxlou05 reviewed Aug 26, 2024

View reviewed changes

TongguangZhang added 11 commits August 27, 2024 10:02

added documentation to config_logger.yaml

fa78340

main logger custom name

134bafe

typo

1efe68d

update reqs

16c36ff

optional file logging for main logger

2177602

changed to get frame information in log functions

ede4cb3

tests for logging with frame

f3b2d8b

logger check if file already open

c5134ff

black formatting changes

5154224

fixed testing bug

11cb86c

black formatting changes

839a4f1

maxlou05 reviewed Aug 27, 2024

View reviewed changes

TongguangZhang added 2 commits August 27, 2024 20:45

setup log directory and enable frame fix

8e6c1f1

pylint fix

7a694e1

maxlou05 approved these changes Aug 28, 2024

View reviewed changes

TongguangZhang merged commit 09811f6 into main Aug 28, 2024
1 check passed

TongguangZhang deleted the move_logger_to_common branch August 28, 2024 01:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved logger to common #45

Moved logger to common #45

AshishA26 commented Aug 11, 2024 •

edited

Loading

TongguangZhang left a comment

TongguangZhang Aug 17, 2024

maxlou05 left a comment

maxlou05 Aug 23, 2024

maxlou05 Aug 25, 2024

maxlou05 Aug 27, 2024

maxlou05 Aug 23, 2024

maxlou05 Aug 23, 2024

maxlou05 left a comment

maxlou05 Aug 25, 2024

maxlou05 Aug 25, 2024

maxlou05 Aug 25, 2024

maxlou05 Aug 25, 2024

maxlou05 Aug 25, 2024

maxlou05 Aug 25, 2024

maxlou05 left a comment

maxlou05 Aug 26, 2024

maxlou05 Aug 26, 2024

maxlou05 Aug 26, 2024

maxlou05 Aug 26, 2024

maxlou05 Aug 26, 2024

maxlou05 Aug 26, 2024

maxlou05 Aug 26, 2024

maxlou05 left a comment

maxlou05 Aug 27, 2024

maxlou05 Aug 27, 2024

maxlou05 Aug 27, 2024

maxlou05 left a comment

		from read_yaml.modules import read_yaml


		CONFIG_FILE_PATH = pathlib.Path("config_logger.yaml")

Moved logger to common #45

Moved logger to common #45

Conversation

AshishA26 commented Aug 11, 2024 • edited Loading

TongguangZhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxlou05 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxlou05 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxlou05 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxlou05 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxlou05 left a comment

Choose a reason for hiding this comment

AshishA26 commented Aug 11, 2024 •

edited

Loading