feature: add command line logging option #971

leejiwon1125 · 2023-01-11T06:53:13Z

"mindgitrwx" wanted to use various logging options directly on command line in #299.

But we can only use 'debug' option. (In fact, we can use 'info' option by simply removing "--debug" in ./backend.ai mgr start-server --debug command)

So, I accepted the demand for "mindgitrwx".

From now on, we can use options such as debug, info, warning, error and critical.

To run manager server, type in terminal
./backend.ai mgr start-server --log-level=~ (~ is one of the logging options: debug, info, warning, error, critical)
instead of ./backend.ai mgr start-server --debug

Plus, according to changed code, the default logging option is info so if you just type ./backend.ai mgr start-server in terminal, then logging option is info.

resolves #299

kyujin-cho

Looks great! Though there are some changes needs to be done before merging:

Removing argument flag at once can cause confusion to users (and of course, this is why modern APIs have somewhat "deprecated" mark). So let's retain debug flag along with newly introduced log-level flag, which acts like --log-level debug option, but warns user that this flag can be removed at any time in future releases.
Please add validating logic to log_level argument. We need to halt program when unexpected (that means, some values other than defined log levels) input is received and notify user that they have typed unknown log level.
There are other packages other than ai.backend.manager.server which uses same argument scheme (e.g. agent, web, ...). Please expand changes to these packages also.

fregataa · 2023-01-17T05:58:18Z

How about impl Enum type for log level like below?

class LogLevel(str, Enum):
    INFO = "info"
    DEBUG = "debug"
    ...

…com/lablup/backend.ai into fix/add-command-line-logging-option

rapsealk · 2023-01-19T09:01:28Z

src/ai/backend/agent/server.py

+    help="This option will soon change to --log-level TEXT option.",
+)
+@click.option(
+    "--log-level",


FYI: You may narrow down the range of supported values using Choices.

https://click.palletsprojects.com/en/8.1.x/options/#choice-options

rapsealk · 2023-01-19T09:05:01Z

src/ai/backend/agent/server.py

 ) -> int:

+    if debug:
+        print("Please use --log-level options instead")


It is recommended to use click.echo() rather than print() for printing.

https://click.palletsprojects.com/en/8.1.x/utils/#printing-to-stdout

kyujin-cho · 2023-01-26T04:05:03Z

src/ai/backend/agent/server.py

+    if log_level not in ["debug", "info", "warning", "error", "critical"]:
+        click.echo("Undefined log-level")
+        click.echo("Try 'backend.ai ag start-server -h' for help")
+        exit(1)


Bit of a combination of @fregataa and @rapsealk 's review. How about muxing log levels as an enum and using click's Choice option? That way we can relieve the burden to manually validate log level string user provided.
Ref: pallets/click#605 (comment)

kyujin-cho · 2023-01-26T04:05:41Z

src/ai/backend/agent/watcher.py

+    if log_level not in ["debug", "info", "warning", "error", "critical"]:
+        click.echo("Undefined log-level")
+        exit(1)


kyujin-cho · 2023-01-26T04:05:47Z

src/ai/backend/manager/server.py

+    if log_level not in ["debug", "info", "warning", "error", "critical"]:
+        click.echo("Undefined log-level")
+        click.echo("Try 'backend.ai mgr start-server -h' for help")
+        exit(1)


fregataa

I left some comments.
Please ask me if you have any questions!

fregataa · 2023-01-29T08:25:46Z

src/ai/backend/agent/server.py

+class LogLevel(str, Enum):
+    debug = "debug"
+    info = "info"
+    warning = "warning"
+    error = "error"
+    critical = "critical"


we name members of Enum as capital letters, like below.

class LogLevel(str, Enum): DEBUG = "debug" INFO = "info" ...

Thank you for your comment!
I wonder if there is any way to reduce duplicated code which means I want to put LogLevel class in such a file that contain classes so that I can import that file to agent/server.py, agent/watcher.py and so on.

fregataa · 2023-01-29T08:31:10Z

src/ai/backend/agent/watcher.py

+class LogLevel(str, Enum):
+    debug = "debug"
+    info = "info"
+    warning = "warning"
+    error = "error"
+    critical = "critical"


fregataa · 2023-01-29T08:31:23Z

src/ai/backend/manager/cli/__main__.py

+class LogLevel(str, Enum):
+    debug = "debug"
+    info = "info"
+    warning = "warning"
+    error = "error"
+    critical = "critical"


fregataa · 2023-01-29T08:31:32Z

src/ai/backend/manager/server.py

+class LogLevel(str, Enum):
+    debug = "debug"
+    info = "info"
+    warning = "warning"
+    error = "error"
+    critical = "critical"


fregataa · 2023-01-29T08:31:38Z

src/ai/backend/storage/server.py

+class LogLevel(str, Enum):
+    debug = "debug"
+    info = "info"
+    warning = "warning"
+    error = "error"
+    critical = "critical"


fregataa · 2023-01-29T08:41:29Z

src/ai/backend/agent/server.py

@@ -920,7 +942,7 @@ def main(
                log.info("runtime: {0}", utils.env_info())

                log_config = logging.getLogger("ai.backend.agent.config")
-                if debug:
+                if log_level == "debug":


Suggested change

if log_level == "debug":

if log_level == LogLevel.DEBUG:

fregataa · 2023-01-29T08:42:02Z

src/ai/backend/agent/watcher.py

+@click.option(
+    "--log-level",
+    type=click.Choice(LogLevel, case_sensitive=False),
+    default="info",


Please check the type here

fregataa · 2023-01-29T08:42:12Z

src/ai/backend/manager/cli/__main__.py

+@click.option(
+    "--log-level",
+    type=click.Choice(LogLevel, case_sensitive=False),
+    default="info",


fregataa · 2023-01-29T08:42:18Z

src/ai/backend/manager/server.py

+@click.option(
+    "--log-level",
+    type=click.Choice(LogLevel, case_sensitive=False),
+    default="info",


fregataa · 2023-01-29T08:42:26Z

src/ai/backend/storage/server.py

+@click.option(
+    "--log-level",
+    type=click.Choice(LogLevel, case_sensitive=False),
+    default="info",


leejiwon1125 · 2023-01-30T04:55:06Z

I reflected all the comments!
According to my check, there is no problem in terms of function.
But I think the 'LogLevel' class repeated several times.
Is there any way to compress this scattered code into one?

kyujin-cho · 2023-01-30T05:03:18Z

I reflected all the comments! According to my check, there is no problem in terms of function. But I think the 'LogLevel' class repeated several times. Is there any way to compress this scattered code into one?

Best way to prevent duplication is to define common code under ai.backend.common.types package and import it on anywhere needed.

kyujin-cho · 2023-01-30T05:04:39Z

And... it seems there already is an enum which does exactly same thing: ai.backend.common.types.LogSeverity. Let's utilize this Enum instead of creating new one.

kyujin-cho · 2023-02-07T06:22:21Z

src/ai/backend/agent/server.py

-        config.override_key(raw_cfg, ("logging", "level"), "DEBUG")
-        config.override_key(raw_cfg, ("logging", "pkg-ns", "ai.backend"), "DEBUG")
+
+    config.override_key(raw_cfg, ("debug", "enabled"), log_level.value == "debug")


Since we're now utilizing LogSeverity to represent log level, please replace all occurrences of raw "debug" string with LogSeverity.DEBUG.

Suggested change

config.override_key(raw_cfg, ("debug", "enabled"), log_level.value == "debug")

config.override_key(raw_cfg, ("debug", "enabled"), log_level== LogSeverity.DEBUG)

kyujin-cho · 2023-02-07T06:22:46Z

src/ai/backend/agent/server.py

@@ -920,7 +935,7 @@ def main(
                log.info("runtime: {0}", utils.env_info())

                log_config = logging.getLogger("ai.backend.agent.config")
-                if debug:
+                if log_level.value == "debug":


Same here.

Suggested change

if log_level.value == "debug":

if log_level == LogSeverity.DEBUG:

kyujin-cho · 2023-02-07T06:23:02Z

src/ai/backend/agent/watcher.py

@@ -383,7 +397,7 @@ def main(cli_ctx, config_path, debug):
    config.override_with_env(
        raw_cfg, ("watcher", "service-addr", "port"), "BACKEND_WATCHER_SERVICE_PORT"
    )
-    if debug:
+    if log_level.value == "debug":


Suggested change

if log_level.value == "debug":

if log_level == LogSeverity.DEBUG:

kyujin-cho · 2023-02-07T06:23:11Z

src/ai/backend/manager/config.py

-        config.override_key(raw_cfg, ("logging", "pkg-ns", "ai.backend"), "DEBUG")
-        config.override_key(raw_cfg, ("logging", "pkg-ns", "aiohttp"), "DEBUG")
+
+    config.override_key(raw_cfg, ("debug", "enabled"), log_level == "debug")


Suggested change

config.override_key(raw_cfg, ("debug", "enabled"), log_level == "debug")

config.override_key(raw_cfg, ("debug", "enabled"), log_level == LogSeverity.DEBUG)

I didn't change this part because log_level is raw string in load function in this file.

kyujin-cho · 2023-02-07T06:23:45Z

src/ai/backend/storage/server.py

    # Determine where to read configuration.
    raw_cfg, cfg_src_path = config.read_from_file(config_path, "storage-proxy")

    config.override_with_env(raw_cfg, ("etcd", "namespace"), "BACKEND_NAMESPACE")
    config.override_with_env(raw_cfg, ("etcd", "addr"), "BACKEND_ETCD_ADDR")
    config.override_with_env(raw_cfg, ("etcd", "user"), "BACKEND_ETCD_USER")
    config.override_with_env(raw_cfg, ("etcd", "password"), "BACKEND_ETCD_PASSWORD")
-    if debug:
+    if log_level.value == "debug":


Suggested change

if log_level.value == "debug":

if log_level == LogSeverity.DEBUG:

…com/lablup/backend.ai into fix/add-command-line-logging-option

changed three files

d97e2a9

github-actions bot assigned leejiwon1125 Jan 11, 2023

github-actions bot added comp:manager Related to Manager component type:feature Add new features effort:easy Need to understand only a specific region of codes (good first issue, easy). labels Jan 11, 2023

leejiwon1125 added this to the 21.03 milestone Jan 11, 2023

leejiwon1125 mentioned this pull request Jan 11, 2023

fix: add command line logging option #967

Closed

add .md file in changes folder

63d2224

leejiwon1125 requested a review from kyujin-cho January 11, 2023 10:16

kyujin-cho requested changes Jan 16, 2023

View reviewed changes

leejiwon1125 changed the title ~~fix: add command line logging option~~ feature: add command line logging option Jan 16, 2023

leejiwon1125 added 4 commits January 19, 2023 04:22

Merge branch 'fix/add-command-line-logging-option' of https://github.…

e722ea0

…com/lablup/backend.ai into fix/add-command-line-logging-option

add validating logic with log_level and revive --debug option

7a3ebc6

add validating logic with log_level and revive --debug option 2

40809c3

add logging level option in agent and storage

156509f

github-actions bot added the comp:storage-proxy Related to Storage proxy component label Jan 19, 2023

add change log

cb4a24a

leejiwon1125 requested a review from kyujin-cho January 19, 2023 08:06

rapsealk reviewed Jan 19, 2023

View reviewed changes

change print to click.echo

15f99fa

kyujin-cho requested changes Jan 26, 2023

View reviewed changes

leejiwon1125 added 5 commits January 27, 2023 08:13

change with click's Choice options

7c3788b

change with click's Choice options

31d8631

saving

7a9520f

I pulled remote/main to local's main

e9191e4

merge remote/main

f2cb725

github-actions bot added the webui label Jan 27, 2023

remove one line

a191bce

fregataa requested changes Jan 29, 2023

View reviewed changes

leejiwon1125 added 4 commits January 30, 2023 01:48

Merge branch 'main' of https://github.com/lablup/backend.ai into main

211c6d1

Merge branch 'main' into fix/add-command-line-logging-option

06dbfcc

fix LogLevel class and fix type

5969864

fix LogLevel class and fix type

f7234b6

leejiwon1125 removed the webui label Jan 30, 2023

leejiwon1125 added 2 commits January 30, 2023 04:21

Merge branch 'main' of https://github.com/lablup/backend.ai into main

8610f9b

Merge branch 'main' into fix/add-command-line-logging-option

b73ffa4

replace LogLevel class to already made LogSeverity class

cab3574

github-actions bot added the webui label Jan 30, 2023

leejiwon1125 and others added 4 commits January 30, 2023 07:29

Merge branch 'main' of https://github.com/lablup/backend.ai into main

e3144db

Merge branch 'main' of https://github.com/lablup/backend.ai into main

ac206db

deps: update backend.ai-app to 22.09.8

0ac52b3

Merge branch 'main' into fix/add-command-line-logging-option

d38be5a

kyujin-cho requested changes Feb 7, 2023

View reviewed changes

leejiwon1125 added 6 commits February 7, 2023 08:24

replace raw string "debug" with LogSeverity.DEBUG

54e0591

Merge branch 'main' of https://github.com/lablup/backend.ai into main

7a3be32

Merge branch 'main' into fix/add-command-line-logging-option

793fd61

Merge branch 'fix/add-command-line-logging-option' of https://github.…

08c621a

…com/lablup/backend.ai into fix/add-command-line-logging-option

replace raw string 'debug' with LogSeverity.DEBUG

270e199

change import form

3a81beb

kyujin-cho approved these changes Feb 13, 2023

View reviewed changes

Merge branch 'main' into fix/add-command-line-logging-option

93f4071

kyujin-cho merged commit d011e8d into main Feb 13, 2023

kyujin-cho deleted the fix/add-command-line-logging-option branch February 13, 2023 05:03

Yaminyam added the size:L 100~500 LoC label Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add command line logging option #971

feature: add command line logging option #971

leejiwon1125 commented Jan 11, 2023 •

edited

Loading

kyujin-cho left a comment

fregataa commented Jan 17, 2023

rapsealk Jan 19, 2023

rapsealk Jan 19, 2023 •

edited

Loading

kyujin-cho Jan 26, 2023

kyujin-cho Jan 26, 2023

kyujin-cho Jan 26, 2023

fregataa left a comment

fregataa Jan 29, 2023

leejiwon1125 Jan 30, 2023 •

edited

Loading

fregataa Jan 29, 2023

fregataa Jan 29, 2023

fregataa Jan 29, 2023

fregataa Jan 29, 2023

fregataa Jan 29, 2023

fregataa Jan 29, 2023

fregataa Jan 29, 2023

fregataa Jan 29, 2023

fregataa Jan 29, 2023

leejiwon1125 commented Jan 30, 2023

kyujin-cho commented Jan 30, 2023

kyujin-cho commented Jan 30, 2023

kyujin-cho Feb 7, 2023

kyujin-cho Feb 7, 2023

kyujin-cho Feb 7, 2023

kyujin-cho Feb 7, 2023

leejiwon1125 Feb 7, 2023

kyujin-cho Feb 7, 2023

	config.override_key(raw_cfg, ("debug", "enabled"), log_level.value == "debug")
	config.override_key(raw_cfg, ("debug", "enabled"), log_level== LogSeverity.DEBUG)

	if log_level.value == "debug":
	if log_level == LogSeverity.DEBUG:

feature: add command line logging option #971

feature: add command line logging option #971

Conversation

leejiwon1125 commented Jan 11, 2023 • edited Loading

kyujin-cho left a comment

Choose a reason for hiding this comment

fregataa commented Jan 17, 2023

Choose a reason for hiding this comment

rapsealk Jan 19, 2023 • edited Loading

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

fregataa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejiwon1125 Jan 30, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejiwon1125 commented Jan 30, 2023

kyujin-cho commented Jan 30, 2023

kyujin-cho commented Jan 30, 2023

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

leejiwon1125 commented Jan 11, 2023 •

edited

Loading

rapsealk Jan 19, 2023 •

edited

Loading

leejiwon1125 Jan 30, 2023 •

edited

Loading