feat: add cli event handler #256
Conversation
|
Code Coverage SummaryDiff against mainResults for commit: 6fd5a22 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
|
Trivy scanning results. .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) .venv/lib/python3.10/site-packages/litellm/proxy/_types.py (secrets)Total: 1 (MEDIUM: 1, HIGH: 0, CRITICAL: 0) MEDIUM: Slack (slack-web-hook) |
| "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.
to_tree name - to me suggests construction of a new object from the class instance
There was a problem hiding this comment.
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.
why do we need to instantiate child_tree ?
There was a problem hiding this comment.
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.
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.
we can also add some parameters to the CLITraceHandler to control whether we want the input/output data to be printed + optional truncation
There was a problem hiding this comment.
I've added printing input/output keys separately and truncation of the long values. I haven't add the parameter for optional input/output printing.
| 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.
lets make it enum, e.g. SpanStatus
I've added this. It is working well. |
| @@ -28,9 +29,15 @@ def ragbits_cli( | |||
| output: Annotated[ | |||
| OutputType, typer.Option("--output", "-o", help="Set the output type (text or json)") | |||
| ] = OutputType.text.value, # type: ignore | |||
| verbose: bool = typer.Option(False, "--verbose", "-v", help="Enable verbose mode"), | |||
There was a problem hiding this comment.
Maybe some less redundant description? Like "Print additional debug information"?
| return tree | ||
|
|
||
|
|
||
| def dicts_to_string(input_dict: dict) -> str: |
There was a problem hiding this comment.
nit: I would make this private (so that external users don't depend on it)
Added: