-
Notifications
You must be signed in to change notification settings - Fork 5
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 cli event handler #256
base: main
Are you sure you want to change the base?
Conversation
Code Coverage Summary
Diff against main
Results for commit: 4ca2cef Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Trivy scanning results. .venv/lib/python3.10/site-packages/litellm/proxy/_types.py (secrets)Total: 1 (MEDIUM: 1, HIGH: 0, CRITICAL: 0) MEDIUM: Slack (slack-web-hook) .venv/lib/python3.10/site-packages/PyJWT-2.9.0.dist-info/METADATA (secrets)Total: 1 (MEDIUM: 1, HIGH: 0, CRITICAL: 0) MEDIUM: JWT (jwt-token) .venv/lib/python3.10/site-packages/litellm/llms/huggingface/huggingface_llms_metadata/hf_text_generation_models.txt (secrets)Total: 1 (MEDIUM: 0, HIGH: 0, CRITICAL: 1) CRITICAL: HuggingFace (hugging-face-access-token) |
# /// | ||
""" | ||
|
||
import asyncio |
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.
are we sure that we need script example of cli-related feature ? even if - perhaps setting trace handler is just one line - perhaps we shall eventually add setting trace handlers to already existing scripts
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.
Probably at the final version we don;t need. I just wanted to see how CLI event handler is working. I can remove it before merge.
"parent": self.parent.name if self.parent else None, | ||
} | ||
|
||
def to_tree(self, tree: Tree | None = None, color: str = "bold blue") -> Tree | None: |
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.
to_tree
name - to me suggests construction of a new object from the class instance
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.
Could you give me better name?
tree = Tree(f"[{color}]{self.name}[/{color}] (Duration: {self.end_time - self.start_time:.3f}s)") | ||
|
||
elif tree and self.end_time: | ||
child_tree = tree.add(f"[{color}]{self.name}[/{color}] (Duration: {self.end_time - self.start_time:.3f}s)") |
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 do we need to instantiate child_tree ?
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.
When we have nested spans and we don't have child_tree we will get flat tree, i.e:
A Duration: 0.000s
├── B Duration: 0.000s
│
├── C Duration: 0.000s
│
├── D Duration: 0.000s
│
└── E Duration: 0.000s
Instead of:
A Duration: 0.000s
├── B Duration: 0.000s
│ └── C Duration: 0.000s
│
└── D Duration: 0.000s
└── E Duration: 0.000s
i noticed we’re using rich tree for rendering traces - it looks great! But it doesn’t show any info about inputs and outputs. maybe we could think about how to include those in the layout? btw, the tree is rendered at the end of processing, maybe there is an option to do it irl - https://rich.readthedocs.io/en/stable/live.html 🤔 |
…cli-event-handler
else: | ||
child_tree = tree.add( | ||
f"[{color}]{self.name}[/{color}] Duration: {duration:.3f}s\n" | ||
f"[{secondary_color}]Inputs: {self.inputs}\nOutputs: {self.outputs})[/{secondary_color}]" |
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.
maybe we can make this more readable, lets try to print each input/output key separately and use different colors for keys and values
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.
we can also add some parameters to the CLITraceHandler
to control whether we want the input/output data to be printed + optional truncation
self.start_time: float = time.perf_counter() | ||
self.end_time: float | None = None | ||
self.children: list[CLISpan] = [] | ||
self.status: str = "started" |
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.
lets make it enum, e.g. SpanStatus
Added:
TODO:
add rendering inputs and outputs