[Fix] Removed unnecessarily changing tuple output to list output #608
Conversation
| # If the function returns multiple values, record them all in the same event | ||
| if isinstance(returns, tuple): | ||
| returns = list(returns) | ||
|
|
There was a problem hiding this comment.
Instead of removing these functions, we can instead do what we intended at fist-- just return a tuple
if istinatnce(returns, tuple):
returns = tuple(returns)
Removing these lines of code altogether will have unintended consequences.
There was a problem hiding this comment.
May I ask what the suggested change does? It seems (in most cases) equivalent to removing the two lines of code.
Unfortunately your suggested change would still break the code I'm using, since the code checks the output of the function is an instance of a specific subclass of tuple (more specifically, a custom namedtuple). Maybe one could argue that this is bad practice, but I believe it's more important that a decorator that does logging doesn't change the output of the function.
There was a problem hiding this comment.
but AFAIK the only reason the tuple was being cast to a list was so that it can be dumped into a JSON, but json.dumps actually handles that automatically.
Indeed that should be handled by a JSONEncoder which is where these sorts of casts belong
|
Left a review @zli11010-- should be an easy fix! Please let me know if it still causes problems with the repo you're trying |
|
@areibman can we have more documentation around the expected functionality of |
|
@areibman Since you had concerns about deleting the lines, I tried to keep the original semantics with the only change being fixing the fact that the type casting modified the output of the original function. Hopefully that's better! |
📥 Pull Request
📘 Description
Fixed this bug: #607
🧪 Testing
Ran my code that uses AgentOps and now it works. (Changing list to tuple doesn't affect what gets logged.)