[Fix] Removed unnecessarily changing tuple output to list output

[zli11010]

📥 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.)

[areibman]

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.

[zli11010]

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.

[teocns]

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

[areibman]

Left a review @zli11010-- should be an easy fix! Please let me know if it still causes problems with the repo you're trying

[teocns]

@areibman can we have more documentation around the expected functionality of returns? I too had a hard time figuring out, maybe some docstrings will do 👍

[zli11010]

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

[the-praxs]

What's the status of this PR?

[areibman]

@teocns can you take a stab at completing this? Just want to make sure we don’t have any downstream effects if we merge

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag	Coverage Δ
unittests	37.80% <100.00%> (+0.30%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
agentops/decorators.py	71.34% <100.00%> (+3.06%)	⬆️

... and 2 files with indirect coverage changes

[teocns]

The removal of the tuple-to-list conversion seems to be the right approach; the conversion was actually unnecessary since:

• Our serialization layer (safe_serialize and filter_unjsonable) already handles tuples correctly
• The conversion was happening too early (at decorator level) rather than at serialization time

Added some tests to ensure @record_* decorators behave correctly in regards to the issue, especially proving the json serialization utilities already handle tuple types correctly without needing to convert them to lists first, proving the point that the conversion was unnecessary :)

If this gets merged, we should open a ticket to unify:

• test_record_action.py
• test_record_tool.py

into:

• test_decorators.py

---

Rationale: Both record_action and record_tool are decorators with similar functionality and testing patterns.
Having their tests in separate files creates unnecessary fragmentation.

resolved