-
Couldn't load subscription status.
- Fork 1.2k
refactor(logging): rename llama_stack logger categories #3065
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
refactor(logging): rename llama_stack logger categories #3065
Conversation
50b5ffe to
91cd285
Compare
91cd285 to
96fbb8e
Compare
|
How have you checked the file base for missing alignment of the log category with the package name? Can you please show me your vibe coding context & prompt? When I use I find quite a few more spots (actually 27 instead of 5). Can you check again, please? Also, can you please specify which "rule" you used to map the package name (file path)? I'm assuming you always use the first path component, but it sometimes seems that the category is randomly picked from* some* path component of the Python file. It should be consistent and unambiguous, so that there is a clear rule also for the future, which category name to pick.
|
| from llama_stack.log import get_logger | ||
|
|
||
| logger = get_logger(name=__name__, category="auth") | ||
| logger = get_logger(name=__name__, category="core") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the rule to extract the category ? First path element ("core") or shouldn't it be last path element ("server") ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I initially use the first path element, but over some reviews on the original PR, I have changed them accordingly :)
| from llama_stack.providers.utils.responses.responses_store import ResponsesStore | ||
|
|
||
| logger = get_logger(name=__name__, category="openai_responses") | ||
| logger = get_logger(name=__name__, category="agents") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which "agents" (randomly from within the package name) and not "providers" (first path element in the package) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto :)
|
It's also ok to introduce a more complex rule-set like:
Please propose such a rule-set. It shouldn't be too involved, though. Try to keep it less than 5 rules. |
thanks for your review so from the table, I conclude that the correct category, is the outer package name, (e.g. for |
thanks for your guidance please find my proposal below
ptal 👍🏽 |
|
thanks for the proposal
"if relevant" should never be part of any rule, as there is a lot of leeway in interpretation about what is actually "relevant". You could say:
However, I would be equally acceptable to handpick a selection and not rely solely on the file path. For that, one should define the full list of categories. Perhaps it's better not to connect the package name, as I don't see any regularity right now that could be leveraged to fully determine the log category from the package / directory name. |
+1, I will remove it above, it should be clear to avoid confusion in the future 👍🏽
+1, therefore I sometimes used the package name and other use the top-level package name. I would appreciate input from others in order to have consensus over the categories names used now, and in the future cc @leseb @ashwinb @mattf @nathan-weinberg @skamenan7 @derekhiggins @bbrowning @cdoern @ehhuang |
878808b to
4e5bc9a
Compare
This PR renames logging categores of llama_stack logger across the codebase according to llamastack#3065 (comment). Signed-off-by: Mustafa Elbehery <[email protected]>
4e5bc9a to
b73b1db
Compare
This PR renames logging categores of llama_stack logger across the codebase according to llamastack#3065 (comment). Signed-off-by: Mustafa Elbehery <[email protected]>
b73b1db to
bbcd6b0
Compare
This PR renames logging categores of llama_stack logger across the codebase according to llamastack#3065 (comment). Signed-off-by: Mustafa Elbehery <[email protected]>
f991345 to
1bad897
Compare
This PR renames logging categores of llama_stack logger across the codebase according to llamastack#3065 (comment). Signed-off-by: Mustafa Elbehery <[email protected]>
This PR renames logging categores of llama_stack logger across the codebase according to llamastack#3065 (comment). Signed-off-by: Mustafa Elbehery <[email protected]>
1bad897 to
7515c9c
Compare
This PR renames logging categores of llama_stack logger across the codebase according to llamastack#3065 (comment). Signed-off-by: Mustafa Elbehery <[email protected]>
7515c9c to
c97dc1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am good with this PR mostly I have a couple of comments inline mostly the addition of a file which feels like a mistake.
llama_stack/core/store/registry.py
Outdated
| from llama_stack.providers.utils.kvstore.config import KVStoreConfig, SqliteKVStoreConfig | ||
|
|
||
| logger = get_logger(__name__, category="core") | ||
| logger = get_logger(__name__, category="core::store") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just name this core::registry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 👍🏽
| ) | ||
|
|
||
| logger = get_logger(name=__name__, category="responses") | ||
| logger = get_logger(name=__name__, category="agents::meta_reference") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should stay responses really, maybe call this openai::responses. this is why we cannot take these dictums too formally sometimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 👍🏽
| @@ -0,0 +1,1154 @@ | |||
| # Copyright (c) Meta Platforms, Inc. and affiliates. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this file added? is that a bad rebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the commit is older than removing this file
thanks for catching this 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed 👍🏽
This PR renames logging categores of llama_stack logger across the codebase according to llamastack#3065 (comment). Signed-off-by: Mustafa Elbehery <[email protected]>
c97dc1c to
39cc12e
Compare
This PR renames logging categores of llama_stack logger across the codebase according to llamastack#3065 (comment). Signed-off-by: Mustafa Elbehery <[email protected]>
39cc12e to
7a98003
Compare
This PR renames logging categores of llama_stack logger across the codebase according to llamastack#3065 (comment). Signed-off-by: Mustafa Elbehery <[email protected]>
7a98003 to
23ef375
Compare
This PR renames logging categores of llama_stack logger across the codebase according to llamastack#3065 (comment). Signed-off-by: Mustafa Elbehery <[email protected]>
23ef375 to
430ea87
Compare
) # What does this PR do? <!-- Provide a short summary of what this PR does and why. Link to relevant issues if applicable. --> This PR renames categories of llama_stack loggers. This PR aligns logging categories as per the package name, as well as reviews from initial llamastack#2868. This is a follow up to llamastack#3061. <!-- If resolving an issue, uncomment and update the line below --> <!-- Closes #[issue-number] --> Replaces llamastack#2868 Part of llamastack#2865 cc @leseb @rhuss Signed-off-by: Mustafa Elbehery <[email protected]>
What does this PR do?
This PR renames categories of llama_stack loggers.
This PR aligns logging categories as per the package name, as well as reviews from initial #2868. This is a follow up to #3061.
Replaces #2868
Part of #2865
cc @leseb @rhuss