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

[Elbehery]

What does this PR do?

This PR adds enforce using llama_stack.log only.

It creates

• scripts/check-logger-usage.py to enforce using llama_stack.log.
• add - id: check-logger-usage in .pre-commit-config.yaml.
• separate commit which fixes the logger usage. Ideally should be merged separately before this PR.

Fixes #2865

Test Plan

[leseb]

please use github output https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions

[Elbehery]

please use github output https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions

@leseb sorry, where should I use GH output 🤔

[leseb]

please use github output https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions

@leseb sorry, where should I use GH output 🤔

in the output, when printing the wrong files @mfleader wrote a similar script.

[Elbehery]

please use github output https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions

@leseb sorry, where should I use GH output 🤔

in the output, when printing the wrong files @mfleader wrote a similar script.

ah u mean in the CI failure, like this

distribution/library_client.py
   Found 'import logging' at line(s): 10
   Use the custom logger instead:
     from llama_stack.log import get_logger
     logger = get_logger(name=__name__, category="core")

?

[Elbehery]

CI Failure is not related to this PR .. investigating

[mattf]

instead of the script, how about just use grep like forbid-pytest-asyncio.

something like grep -e "import logging" -e "from logging" | grep -v "allow-direct-logging, then put import logging # allow-direct-logging where appropriate

[Elbehery]

instead of the script, how about just use grep like forbid-pytest-asyncio.

something like grep -e "import logging" -e "from logging" | grep -v "allow-direct-logging, then put import logging # allow-direct-logging where appropriate

cc @leseb @nathan-weinberg @cdoern @derekhiggins @ashwinb

[leseb]

instead of the script, how about just use grep like forbid-pytest-asyncio.
something like grep -e "import logging" -e "from logging" | grep -v "allow-direct-logging, then put import logging # allow-direct-logging where appropriate

cc @leseb @nathan-weinberg @cdoern @derekhiggins @ashwinb

Agreed I don't see much value in Python here, bash is great for these sorts of simple things (and probably faster too).

[Elbehery]

@leseb @mattf ptal now 👍🏽

[mattf]

lgtm

[Elbehery]

@mattf @ashwinb ptal

[Elbehery]

@leseb ptal 👍🏽

[leseb]

Please do not change the name of the variable; either log or logger is fine, I don't think the naming of the logger variable is something we want to enforce. This will also help reduce the size of the changes. Thanks!

[Elbehery]

thanks for your review :)

either log or logger is fine,

lets keep logger for now, we need to use get_logger(name=__name__, category="core") anyways to use the new logger :)

[Elbehery]

ah u meant to use log instead of logger where the logger is being used to reduce the changes 👍🏽

make sense 👍🏽

[Elbehery]

ptal 👍🏽

[leseb]

This is still unresolved. Please do not resolve if not addressed.

[ashwinb]

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.

[Elbehery]

thanks for your review

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

How to proceed ?

[Elbehery]

i am preserving this commit here for your ref.

This commit was using logger

[ashwinb]

@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.

[Elbehery]

lgtm 👍🏽

[Elbehery]

raised #3060 👍🏽

[rhuss]

@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.

[Elbehery]

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

[Elbehery]

raised #3060 👍🏽

[rhuss]

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')

[Elbehery]

lgtm 👍🏽

[Elbehery]

raised #3061 👍🏽

[Elbehery]

/hold

[Elbehery]

replaced by #3061