Skip to content
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.

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

feat: add options to output registered entity summary #3028

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
15 changes: 9 additions & 6 deletions flytekit/clis/sdk_in_container/pyflyte.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@
from flytekit.clis.sdk_in_container.serve import serve
from flytekit.clis.sdk_in_container.utils import ErrorHandlingCommand, validate_package
from flytekit.clis.version import info
from flytekit.configuration.file import FLYTECTL_CONFIG_ENV_VAR, FLYTECTL_CONFIG_ENV_VAR_OVERRIDE
from flytekit.configuration.file import FLYTECTL_CONFIG_ENV_VAR_OVERRIDE
from flytekit.configuration.internal import LocalSDK
from flytekit.configuration.plugin import get_plugin
from flytekit.loggers import logger

# from flytekit.configuration.file import FLYTECTL_CONFIG_ENV_VAR, FLYTECTL_CONFIG_ENV_VAR_OVERRIDE
# from flytekit.loggers import logger


@click.group("pyflyte", invoke_without_command=True, cls=ErrorHandlingCommand)
Expand Down Expand Up @@ -66,11 +68,12 @@ def main(ctx, pkgs: typing.List[str], config: str, verbose: int):
if config:
ctx.obj[CTX_CONFIG_FILE] = config
cfg = configuration.ConfigFile(config)
# Temporarily commented out to ensure proper output format when using --quiet flag in pyflyte register
# Set here so that if someone has Config.auto() in their user code, the config here will get used.
if FLYTECTL_CONFIG_ENV_VAR in os.environ:
logger.info(
f"Config file arg {config} will override env var {FLYTECTL_CONFIG_ENV_VAR}: {os.environ[FLYTECTL_CONFIG_ENV_VAR]}"
)
# if FLYTECTL_CONFIG_ENV_VAR in os.environ:
# logger.info(
# f"Config file arg {config} will override env var {FLYTECTL_CONFIG_ENV_VAR}: {os.environ[FLYTECTL_CONFIG_ENV_VAR]}"
# )
os.environ[FLYTECTL_CONFIG_ENV_VAR_OVERRIDE] = config
if not pkgs:
pkgs = LocalSDK.WORKFLOW_PACKAGES.read(cfg)
Expand Down
109 changes: 73 additions & 36 deletions flytekit/clis/sdk_in_container/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
the root of your project, it finds the first folder that does not have a ``__init__.py`` file.
"""

_original_secho = click.secho
_original_log_level = logger.level
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using context manager for logger

Consider using a context manager to handle logger level changes instead of storing the level in a global variable. This would ensure proper cleanup and avoid potential side effects. A similar issue was also found in flytekit/tools/repo.py (line 290).

Code suggestion
Check the AI-generated fix before applying
Suggested change
_original_log_level = logger.level
class LogLevelManager:
def __init__(self):
self.original_level = logger.level
def __enter__(self):
return self
def __exit__(self, exc_type, exc_val, exc_tb):
logger.level = self.original_level

Code Review Run #7297c4


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged



@click.command("register", help=_register_help)
@project_option_dec
Expand Down Expand Up @@ -142,6 +145,20 @@
help="Skip errors during registration. This is useful when registering multiple packages and you want to skip "
"errors for some packages.",
)
@click.option(
"--summary-format",
"-f",
required=False,
type=click.Choice(["json", "yaml"], case_sensitive=False),
default=None,
help="Output format for registration summary. Lists registered workflows, tasks, and launch plans. 'json' and 'yaml' supported.",
)
@click.option(
"--quiet",
is_flag=True,
default=False,
help="Suppress output messages, only displaying errors.",
)
@click.argument("package-or-module", type=click.Path(exists=True, readable=True, resolve_path=True), nargs=-1)
@click.pass_context
def register(
Expand All @@ -162,12 +179,25 @@ def register(
activate_launchplans: bool,
env: typing.Optional[typing.Dict[str, str]],
skip_errors: bool,
summary_format: typing.Optional[str],
quiet: bool,
):
"""
see help
"""

if summary_format is not None:
quiet = True

if quiet:
# Mute all secho output through monkey patching
click.secho = lambda *args, **kw: None
# Output only log at ERROR or CRITICAL level
logger.setLevel("ERROR")

# Set the relevant copy option if non_fast is set, this enables the individual file listing behavior
# that the copy flag uses.

if non_fast:
click.secho("The --non-fast flag is deprecated, please use --copy none instead", fg="yellow")
if "--copy" in sys.argv:
Expand Down Expand Up @@ -195,39 +225,46 @@ def register(
"Missing argument 'PACKAGE_OR_MODULE...', at least one PACKAGE_OR_MODULE is required but multiple can be passed",
)

# Use extra images in the config file if that file exists
config_file = ctx.obj.get(constants.CTX_CONFIG_FILE)
if config_file:
image_config = patch_image_config(config_file, image_config)

click.secho(
f"Running pyflyte register from {os.getcwd()} "
f"with images {image_config} "
f"and image destination folder {destination_dir} "
f"on {len(package_or_module)} package(s) {package_or_module}",
dim=True,
)

# Create and save FlyteRemote,
remote = get_and_save_remote_with_click_context(ctx, project, domain, data_upload_location="flyte://data")
click.secho(f"Registering against {remote.config.platform.endpoint}")
repo.register(
project,
domain,
image_config,
output,
destination_dir,
service_account,
raw_data_prefix,
version,
deref_symlinks,
copy_style=copy,
package_or_module=package_or_module,
remote=remote,
env=env,
dry_run=dry_run,
activate_launchplans=activate_launchplans,
skip_errors=skip_errors,
show_files=show_files,
verbosity=ctx.obj[constants.CTX_VERBOSE],
)
try:
# Use extra images in the config file if that file exists
config_file = ctx.obj.get(constants.CTX_CONFIG_FILE)
if config_file:
image_config = patch_image_config(config_file, image_config)

click.secho(
f"Running pyflyte register from {os.getcwd()} "
f"with images {image_config} "
f"and image destination folder {destination_dir} "
f"on {len(package_or_module)} package(s) {package_or_module}",
dim=True,
)

# Create and save FlyteRemote,
remote = get_and_save_remote_with_click_context(ctx, project, domain, data_upload_location="flyte://data")
click.secho(f"Registering against {remote.config.platform.endpoint}")
repo.register(
project,
domain,
image_config,
output,
destination_dir,
service_account,
raw_data_prefix,
version,
deref_symlinks,
copy_style=copy,
package_or_module=package_or_module,
remote=remote,
env=env,
summary_format=summary_format,
quiet=quiet,
dry_run=dry_run,
activate_launchplans=activate_launchplans,
skip_errors=skip_errors,
show_files=show_files,
verbosity=ctx.obj[constants.CTX_VERBOSE],
)
finally:
# Restore original secho
click.secho = _original_secho
logger.setLevel(_original_log_level)
3 changes: 2 additions & 1 deletion flytekit/clis/sdk_in_container/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ def validate_package(ctx, param, values):
pkgs.extend(val.split(","))
else:
pkgs.append(val)
logger.debug(f"Using packages: {pkgs}")
# Temporarily commented out to ensure proper output format when using --quiet flag in pyflyte register
# logger.debug(f"Using packages: {pkgs}")
return pkgs


Expand Down
Loading
Loading