Skip to content

chore: add pre-commit hook to enforce llama_stack.log logger #2868

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

Closed
wants to merge 1 commit into from
Closed

chore: add pre-commit hook to enforce llama_stack.log logger #2868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,24 @@ repos:
pass_filenames: false
require_serial: true
files: ^.github/workflows/.*$
- id: check-log-usage
name: Check for proper log usage (use llama_stack.log instead)
entry: bash
language: system
types: [python]
pass_filenames: true
args:
- -c
- |
matches=$(grep -EnH '^[^#]*\b(import logging|from logging\b)' "$@" | grep -v '# allow-direct-logging' || true)
Copy link
Contributor

Choose a reason for hiding this comment

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

you could make the regexp more robust by allowing an arbitrary amount of spaces like with \s+. Also you might want allow #allow-direct-logging and # allow-direct-logging, too (so maybe grep -v -e '#\s*allow-direct-logging')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lgtm 👍🏽

Copy link
Contributor Author

Choose a reason for hiding this comment

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

raised #3061 👍🏽

if [ -n "$matches" ]; then
# GitHub Actions annotation format
while IFS=: read -r file line_num rest; do
echo "::error file=$file,line=$line_num::Do not use 'import logging' or 'from logging import' in $file. Use the custom log instead: from llama_stack.log import get_logger; logger = get_logger(). If direct logging is truly needed, add: # allow-direct-logging"
Copy link
Contributor

Choose a reason for hiding this comment

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

@Elbehery Can you please also use log here instead of logger ? Or some more neutral wording.

If you want to rename all instances from log to logger, please do this in a separate PR. It would be nice to reason about the benefits of such a cosmetic change that touches 92 files (and increases quite a bit the likelihood of rebase conflicts for other open PRs). There needs to be a very good reason for doing this. Personal taste is not one of these.

IMO, one should always assess the value of a change to the size of the change, which needs to be in balance. This is not the case here IMO as the value (logger instead of log) is negligible compared to the size of the change and the efforts for other to adapt to this change (it's not only open PR, but also people actively working right now on new feature who would need to rebase as well).

But if you detangle both concerns (add a recommit check vs. _rename all log to logger) into two separate PRs, then it makes it easier to get at least one of those included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks so much for your review 🙏🏽

makes perfect sense to me 👍🏽

I actually initally made the pre-commit change in a separate commit than changing the logging in the code base :)

I am fine with the renaming, and I can do it in a separate PR as you suggested

just could we agree on a name (i.e. log or logger)

thanks in advance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

raised #3060 👍🏽

done <<< "$matches"
exit 1
fi
exit 0

ci:
autofix_commit_msg: 🎨 [pre-commit.ci] Auto format from pre-commit.com hooks
Expand Down
20 changes: 10 additions & 10 deletions llama_stack/cli/stack/run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

REPO_ROOT = Path(__file__).parent.parent.parent.parent

logger = get_logger(name=__name__, category="server")
log = get_logger(name=__name__, category="server")
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah why this spurious change? please move it back to logger as the variable name please.

It is extremely important that you don't add gratuitous changes when making PRs. Stick to the core thesis of the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for your review

i just changed this due to this review #2868 (comment)

How to proceed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i am preserving this commit here for your ref.

This commit was using logger

Copy link
Contributor

@ashwinb ashwinb Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@Elbehery let me rephrase myself and be extremely clear.

This PR should only have changes which look like this:

  • Change import logging to from llama_stack.log import get_logger
  • Change a X = logging.get_logger() to X = get_logger(name=..., category=...)

There should be no other changes in the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lgtm 👍🏽

Copy link
Contributor Author

Choose a reason for hiding this comment

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

raised #3060 👍🏽



class StackRun(Subcommand):
Expand Down Expand Up @@ -126,7 +126,7 @@ def _run_stack_run_cmd(self, args: argparse.Namespace) -> None:
self.parser.error("Config file is required for venv environment")

if config_file:
logger.info(f"Using run configuration: {config_file}")
log.info(f"Using run configuration: {config_file}")

try:
config_dict = yaml.safe_load(config_file.read_text())
Expand All @@ -145,7 +145,7 @@ def _run_stack_run_cmd(self, args: argparse.Namespace) -> None:
# If neither image type nor image name is provided, assume the server should be run directly
# using the current environment packages.
if not image_type and not image_name:
logger.info("No image type or image name provided. Assuming environment packages.")
log.info("No image type or image name provided. Assuming environment packages.")
from llama_stack.core.server.server import main as server_main

# Build the server args from the current args passed to the CLI
Expand Down Expand Up @@ -185,11 +185,11 @@ def _run_stack_run_cmd(self, args: argparse.Namespace) -> None:
run_command(run_args)

def _start_ui_development_server(self, stack_server_port: int):
logger.info("Attempting to start UI development server...")
log.info("Attempting to start UI development server...")
# Check if npm is available
npm_check = subprocess.run(["npm", "--version"], capture_output=True, text=True, check=False)
if npm_check.returncode != 0:
logger.warning(
log.warning(
f"'npm' command not found or not executable. UI development server will not be started. Error: {npm_check.stderr}"
)
return
Expand All @@ -214,13 +214,13 @@ def _start_ui_development_server(self, stack_server_port: int):
stderr=stderr_log_file,
env={**os.environ, "NEXT_PUBLIC_LLAMA_STACK_BASE_URL": f"http://localhost:{stack_server_port}"},
)
logger.info(f"UI development server process started in {ui_dir} with PID {process.pid}.")
logger.info(f"Logs: stdout -> {ui_stdout_log_path}, stderr -> {ui_stderr_log_path}")
logger.info(f"UI will be available at http://localhost:{os.getenv('LLAMA_STACK_UI_PORT', 8322)}")
log.info(f"UI development server process started in {ui_dir} with PID {process.pid}.")
log.info(f"Logs: stdout -> {ui_stdout_log_path}, stderr -> {ui_stderr_log_path}")
log.info(f"UI will be available at http://localhost:{os.getenv('LLAMA_STACK_UI_PORT', 8322)}")

except FileNotFoundError:
logger.error(
log.error(
"Failed to start UI development server: 'npm' command not found. Make sure npm is installed and in your PATH."
)
except Exception as e:
logger.error(f"Failed to start UI development server in {ui_dir}: {e}")
log.error(f"Failed to start UI development server in {ui_dir}: {e}")
2 changes: 1 addition & 1 deletion llama_stack/cli/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from llama_stack.log import get_logger

logger = get_logger(name=__name__, category="cli")
log = get_logger(name=__name__, category="cli")


# TODO: this can probably just be inlined now?
Expand Down
6 changes: 3 additions & 3 deletions llama_stack/core/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
# the root directory of this source tree.

import importlib.resources
import logging
import sys

from pydantic import BaseModel
Expand All @@ -17,10 +16,9 @@
from llama_stack.core.utils.exec import run_command
from llama_stack.core.utils.image_types import LlamaStackImageType
from llama_stack.distributions.template import DistributionTemplate
from llama_stack.log import get_logger
from llama_stack.providers.datatypes import Api

log = logging.getLogger(__name__)

# These are the dependencies needed by the distribution server.
# `llama-stack` is automatically installed by the installation script.
SERVER_DEPENDENCIES = [
Expand All @@ -33,6 +31,8 @@
"opentelemetry-exporter-otlp-proto-http",
]

log = get_logger(name=__name__, category="core")


class ApiInput(BaseModel):
api: Api
Expand Down
22 changes: 11 additions & 11 deletions llama_stack/core/configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
#
# This source code is licensed under the terms described in the LICENSE file in
# the root directory of this source tree.
import logging
import textwrap
from typing import Any

Expand All @@ -21,9 +20,10 @@
from llama_stack.core.utils.config_dirs import EXTERNAL_PROVIDERS_DIR
from llama_stack.core.utils.dynamic import instantiate_class_type
from llama_stack.core.utils.prompt_for_config import prompt_for_config
from llama_stack.log import get_logger
from llama_stack.providers.datatypes import Api, ProviderSpec

logger = logging.getLogger(__name__)
log = get_logger(name=__name__, category="core")


def configure_single_provider(registry: dict[str, ProviderSpec], provider: Provider) -> Provider:
Expand All @@ -49,7 +49,7 @@ def configure_api_providers(config: StackRunConfig, build_spec: DistributionSpec
is_nux = len(config.providers) == 0

if is_nux:
logger.info(
log.info(
textwrap.dedent(
"""
Llama Stack is composed of several APIs working together. For each API served by the Stack,
Expand All @@ -75,12 +75,12 @@ def configure_api_providers(config: StackRunConfig, build_spec: DistributionSpec

existing_providers = config.providers.get(api_str, [])
if existing_providers:
logger.info(f"Re-configuring existing providers for API `{api_str}`...")
log.info(f"Re-configuring existing providers for API `{api_str}`...")
updated_providers = []
for p in existing_providers:
logger.info(f"> Configuring provider `({p.provider_type})`")
log.info(f"> Configuring provider `({p.provider_type})`")
updated_providers.append(configure_single_provider(provider_registry[api], p))
logger.info("")
log.info("")
else:
# we are newly configuring this API
plist = build_spec.providers.get(api_str, [])
Expand All @@ -89,17 +89,17 @@ def configure_api_providers(config: StackRunConfig, build_spec: DistributionSpec
if not plist:
raise ValueError(f"No provider configured for API {api_str}?")

logger.info(f"Configuring API `{api_str}`...")
log.info(f"Configuring API `{api_str}`...")
updated_providers = []
for i, provider in enumerate(plist):
if i >= 1:
others = ", ".join(p.provider_type for p in plist[i:])
logger.info(
log.info(
f"Not configuring other providers ({others}) interactively. Please edit the resulting YAML directly.\n"
)
break

logger.info(f"> Configuring provider `({provider.provider_type})`")
log.info(f"> Configuring provider `({provider.provider_type})`")
pid = provider.provider_type.split("::")[-1]
updated_providers.append(
configure_single_provider(
Expand All @@ -111,7 +111,7 @@ def configure_api_providers(config: StackRunConfig, build_spec: DistributionSpec
),
)
)
logger.info("")
log.info("")

config.providers[api_str] = updated_providers

Expand Down Expand Up @@ -169,7 +169,7 @@ def parse_and_maybe_upgrade_config(config_dict: dict[str, Any]) -> StackRunConfi
return StackRunConfig(**cast_image_name_to_string(processed_config_dict))

if "routing_table" in config_dict:
logger.info("Upgrading config...")
log.info("Upgrading config...")
config_dict = upgrade_from_routing_table(config_dict)

config_dict["version"] = LLAMA_STACK_RUN_CONFIG_VERSION
Expand Down
32 changes: 16 additions & 16 deletions llama_stack/core/distribution.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
remote_provider_spec,
)

logger = get_logger(name=__name__, category="core")
log = get_logger(name=__name__, category="core")


def stack_apis() -> list[Api]:
Expand Down Expand Up @@ -141,18 +141,18 @@ def get_provider_registry(config=None) -> dict[Api, dict[str, ProviderSpec]]:
registry: dict[Api, dict[str, ProviderSpec]] = {}
for api in providable_apis():
name = api.name.lower()
logger.debug(f"Importing module {name}")
log.debug(f"Importing module {name}")
try:
module = importlib.import_module(f"llama_stack.providers.registry.{name}")
registry[api] = {a.provider_type: a for a in module.available_providers()}
except ImportError as e:
logger.warning(f"Failed to import module {name}: {e}")
log.warning(f"Failed to import module {name}: {e}")

# Refresh providable APIs with external APIs if any
external_apis = load_external_apis(config)
for api, api_spec in external_apis.items():
name = api_spec.name.lower()
logger.info(f"Importing external API {name} module {api_spec.module}")
log.info(f"Importing external API {name} module {api_spec.module}")
try:
module = importlib.import_module(api_spec.module)
registry[api] = {a.provider_type: a for a in module.available_providers()}
Expand All @@ -161,7 +161,7 @@ def get_provider_registry(config=None) -> dict[Api, dict[str, ProviderSpec]]:
# This assume that the in-tree provider(s) are not available for this API which means
# that users will need to use external providers for this API.
registry[api] = {}
logger.error(
log.error(
f"Failed to import external API {name}: {e}. Could not populate the in-tree provider(s) registry for {api.name}. \n"
"Install the API package to load any in-tree providers for this API."
)
Expand All @@ -183,13 +183,13 @@ def get_provider_registry(config=None) -> dict[Api, dict[str, ProviderSpec]]:
def get_external_providers_from_dir(
registry: dict[Api, dict[str, ProviderSpec]], config
) -> dict[Api, dict[str, ProviderSpec]]:
logger.warning(
log.warning(
"Specifying external providers via `external_providers_dir` is being deprecated. Please specify `module:` in the provider instead."
)
external_providers_dir = os.path.abspath(os.path.expanduser(config.external_providers_dir))
if not os.path.exists(external_providers_dir):
raise FileNotFoundError(f"External providers directory not found: {external_providers_dir}")
logger.info(f"Loading external providers from {external_providers_dir}")
log.info(f"Loading external providers from {external_providers_dir}")

for api in providable_apis():
api_name = api.name.lower()
Expand All @@ -198,13 +198,13 @@ def get_external_providers_from_dir(
for provider_type in ["remote", "inline"]:
api_dir = os.path.join(external_providers_dir, provider_type, api_name)
if not os.path.exists(api_dir):
logger.debug(f"No {provider_type} provider directory found for {api_name}")
log.debug(f"No {provider_type} provider directory found for {api_name}")
continue

# Look for provider spec files in the API directory
for spec_path in glob.glob(os.path.join(api_dir, "*.yaml")):
provider_name = os.path.splitext(os.path.basename(spec_path))[0]
logger.info(f"Loading {provider_type} provider spec from {spec_path}")
log.info(f"Loading {provider_type} provider spec from {spec_path}")

try:
with open(spec_path) as f:
Expand All @@ -217,16 +217,16 @@ def get_external_providers_from_dir(
spec = _load_inline_provider_spec(spec_data, api, provider_name)
provider_type_key = f"inline::{provider_name}"

logger.info(f"Loaded {provider_type} provider spec for {provider_type_key} from {spec_path}")
log.info(f"Loaded {provider_type} provider spec for {provider_type_key} from {spec_path}")
if provider_type_key in registry[api]:
logger.warning(f"Overriding already registered provider {provider_type_key} for {api.name}")
log.warning(f"Overriding already registered provider {provider_type_key} for {api.name}")
registry[api][provider_type_key] = spec
logger.info(f"Successfully loaded external provider {provider_type_key}")
log.info(f"Successfully loaded external provider {provider_type_key}")
except yaml.YAMLError as yaml_err:
logger.error(f"Failed to parse YAML file {spec_path}: {yaml_err}")
log.error(f"Failed to parse YAML file {spec_path}: {yaml_err}")
raise yaml_err
except Exception as e:
logger.error(f"Failed to load provider spec from {spec_path}: {e}")
log.error(f"Failed to load provider spec from {spec_path}: {e}")
raise e

return registry
Expand All @@ -241,7 +241,7 @@ def get_external_providers_from_module(
else:
provider_list = config.providers.items()
if provider_list is None:
logger.warning("Could not get list of providers from config")
log.warning("Could not get list of providers from config")
return registry
for provider_api, providers in provider_list:
for provider in providers:
Expand Down Expand Up @@ -272,6 +272,6 @@ def get_external_providers_from_module(
"get_provider_spec not found. If specifying an external provider via `module` in the Provider spec, the Provider must have the `provider.get_provider_spec` module available"
) from exc
except Exception as e:
logger.error(f"Failed to load provider spec from module {provider.module}: {e}")
log.error(f"Failed to load provider spec from module {provider.module}: {e}")
raise e
return registry
12 changes: 6 additions & 6 deletions llama_stack/core/external.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from llama_stack.core.datatypes import BuildConfig, StackRunConfig
from llama_stack.log import get_logger

logger = get_logger(name=__name__, category="core")
log = get_logger(name=__name__, category="core")


def load_external_apis(config: StackRunConfig | BuildConfig | None) -> dict[Api, ExternalApiSpec]:
Expand All @@ -28,10 +28,10 @@ def load_external_apis(config: StackRunConfig | BuildConfig | None) -> dict[Api,

external_apis_dir = config.external_apis_dir.expanduser().resolve()
if not external_apis_dir.is_dir():
logger.error(f"External APIs directory is not a directory: {external_apis_dir}")
log.error(f"External APIs directory is not a directory: {external_apis_dir}")
return {}

logger.info(f"Loading external APIs from {external_apis_dir}")
log.info(f"Loading external APIs from {external_apis_dir}")
external_apis: dict[Api, ExternalApiSpec] = {}

# Look for YAML files in the external APIs directory
Expand All @@ -42,13 +42,13 @@ def load_external_apis(config: StackRunConfig | BuildConfig | None) -> dict[Api,

spec = ExternalApiSpec(**spec_data)
api = Api.add(spec.name)
logger.info(f"Loaded external API spec for {spec.name} from {yaml_path}")
log.info(f"Loaded external API spec for {spec.name} from {yaml_path}")
external_apis[api] = spec
except yaml.YAMLError as yaml_err:
logger.error(f"Failed to parse YAML file {yaml_path}: {yaml_err}")
log.error(f"Failed to parse YAML file {yaml_path}: {yaml_err}")
raise
except Exception:
logger.exception(f"Failed to load external API spec from {yaml_path}")
log.exception(f"Failed to load external API spec from {yaml_path}")
raise

return external_apis
Loading
Loading