Skip to content

[MLGO] Refactored verbosity flag in mlgo-utils to common location #128541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 24, 2025
Merged

[MLGO] Refactored verbosity flag in mlgo-utils to common location #128541

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
651fbfe
add common lib file to setup arguments for parser #107898
aliraeisdanaei Feb 24, 2025
cd67627
Change the name to flags in accordance with review, #107898
aliraeisdanaei Feb 24, 2025
7d6eea7
Add new line at the end of file #107898
aliraeisdanaei Feb 24, 2025
a6979af
Update llvm/utils/mlgo-utils/mlgo/corpus/flags.py
aliraeisdanaei Feb 24, 2025
d6b095c
autoformat using black #107898
aliraeisdanaei Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions llvm/utils/mlgo-utils/mlgo/corpus/combine_training_corpus.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import logging

from mlgo.corpus import combine_training_corpus_lib
from mlgo.corpus import parser_setup_lib


def parse_args_and_run():
Expand All @@ -36,15 +37,7 @@ def parse_args_and_run():
parser.add_argument(
"--root_dir", type=str, help="The root dir of module paths to combine."
)
# TODO(#107898): Refactor this into a common location.
parser.add_argument(
"--verbosity",
type=str,
help="The verbosity level to use for logging",
default="INFO",
nargs="?",
choices=["DEBUG", "INFO", "WARNING", "ERROR"],
)
parser_setup_lib.add_verbosity_arguments(parser)
args = parser.parse_args()
main(args)

Expand Down
12 changes: 2 additions & 10 deletions llvm/utils/mlgo-utils/mlgo/corpus/extract_ir.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import logging

from mlgo.corpus import extract_ir_lib

from mlgo.corpus import parser_setup_lib

def parse_args_and_run():
parser = argparse.ArgumentParser(
Expand Down Expand Up @@ -111,15 +111,7 @@ def parse_args_and_run():
default=".llvmbc",
nargs="?",
)
# TODO(#107898): Refactor this into a common location.
parser.add_argument(
"--verbosity",
type=str,
help="The verbosity level to use for logging",
default="INFO",
nargs="?",
choices=["DEBUG", "INFO", "WARNING", "ERROR"],
)
parser_setup_lib.add_verbosity_arguments(parser)
args = parser.parse_args()
main(args)

Expand Down
21 changes: 21 additions & 0 deletions llvm/utils/mlgo-utils/mlgo/corpus/parser_setup_lib.py
View file
Open in desktop
Copy link
Member

@mtrofin mtrofin Feb 24, 2025

Choose a reason for hiding this comment

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

nit: can you rename this to flags or tool_flags (open to other suggestions):

  • we only use the _lib naming pattern when we have a tool and want to unittest it, so most of the tool logic is moved to the _lib, leaving the tool itself be basically the main
  • an advantage of your patch is that it encapsulates the dependency on a specific arg parsing library (argparse), so renaming it away from what it encapsulates and towards what it provides its users makes sense, imho.

Thanks!

Copy link
Contributor Author

@aliraeisdanaei aliraeisdanaei Feb 24, 2025

Choose a reason for hiding this comment

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

Thank you I can name my file to parser_flags if you like

Copy link
Member

@mtrofin mtrofin Feb 24, 2025

Choose a reason for hiding this comment

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

That would mean they are flags for the parser :) Would flags work?

Copy link
Contributor Author

@aliraeisdanaei aliraeisdanaei Feb 24, 2025

Choose a reason for hiding this comment

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

Ye sure flags its is

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
# See https://llvm.org/LICENSE.txt for license information.
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
"""Library functions for setting up common parser arguments"""

from argparse import ArgumentParser

def add_verbosity_arguments(parser: ArgumentParser) -> None:
"""Adds the arguments for verbosity to the ArgumentParser

Arguments:
parser -- the argument parser being modified with verbosity arguments
"""
parser.add_argument(
"--verbosity",
type=str,
help="The verbosity level to use for logging",
default="INFO",
nargs="?",
choices=["DEBUG", "INFO", "WARNING", "ERROR"],
)
Loading