Skip to content
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.

Sign up for GitHub

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

Add optional crash dumping on ConnectionError #1077

Closed
wants to merge 22 commits into from
+38 −0
Closed

Add optional crash dumping on ConnectionError #1077

Changes from 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c0f3745
Add crush dumping if content size limit exceeded
Oct 8, 2024
e6cbc10
Check that request is not None
Oct 8, 2024
ea9682b
Reformat with poetry
Oct 8, 2024
1e12160
Kick off file write on separate thread
Oct 8, 2024
1830cf0
Make function private
Oct 8, 2024
69f02cc
Sort imports
Oct 8, 2024
4903f7a
Remove threading, write should be inexpensive
Oct 8, 2024
9e39b7a
Ran poetry
Oct 8, 2024
47a16dc
Test if write is the issue
Oct 8, 2024
e5d1415
Add dump
Oct 8, 2024
d946180
Address comments + mask api key
Oct 9, 2024
f3d6aa2
Add type hint for copied request_data
Oct 9, 2024
b6fd11b
Fix type hints
Oct 9, 2024
5c6888b
Add try except around crash dump writing
Oct 9, 2024
9dd556c
Reformat for style
Oct 9, 2024
e41f3a1
Update python/langsmith/client.py
nhuang-lc Oct 10, 2024
feb769c
Manually build request to log to prevent other sensitive fields
Oct 10, 2024
5af0b29
Remove unused imports
Oct 10, 2024
e6be326
Merge branch 'main' into nh/add-crash-dump
nhuang-lc Oct 11, 2024
61772b9
Merge branch 'main' into nh/add-crash-dump
nhuang-lc Oct 11, 2024
8bfe9a5
Merge branch 'main' into nh/add-crash-dump
nhuang-lc Oct 11, 2024
6743172
Merge branch 'main' into nh/add-crash-dump
nhuang-lc Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions python/langsmith/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""Client for interacting with the LangSmith API.

Check notice on line 1 in python/langsmith/client.py

View workflow job for this annotation

GitHub Actions / benchmark

Benchmark results 

.........................................
create_5_000_run_trees: Mean +- std dev: 577 ms +- 47 ms
.........................................
create_10_000_run_trees: Mean +- std dev: 1.13 sec +- 0.06 sec
.........................................
create_20_000_run_trees: Mean +- std dev: 1.13 sec +- 0.06 sec
.........................................
dumps_class_nested_py_branch_and_leaf_200x400: Mean +- std dev: 769 us +- 12 us
.........................................
dumps_class_nested_py_leaf_50x100: Mean +- std dev: 26.9 ms +- 0.4 ms
.........................................
dumps_class_nested_py_leaf_100x200: Mean +- std dev: 111 ms +- 3 ms
.........................................
dumps_dataclass_nested_50x100: Mean +- std dev: 27.8 ms +- 0.5 ms
.........................................
WARNING: the benchmark result may be unstable
* the standard deviation (16.2 ms) is 26% of the mean (61.3 ms)
* the maximum (93.6 ms) is 53% greater than the mean (61.3 ms)

Try to rerun the benchmark with more runs, values and/or loops.
Run 'python -m pyperf system tune' command to reduce the system jitter.
Use pyperf stats, pyperf dump and pyperf hist to analyze results.
Use --quiet option to hide these warnings.

dumps_pydantic_nested_50x100: Mean +- std dev: 61.3 ms +- 16.2 ms
.........................................
WARNING: the benchmark result may be unstable
* the standard deviation (31.9 ms) is 15% of the mean (219 ms)

Try to rerun the benchmark with more runs, values and/or loops.
Run 'python -m pyperf system tune' command to reduce the system jitter.
Use pyperf stats, pyperf dump and pyperf hist to analyze results.
Use --quiet option to hide these warnings.

dumps_pydanticv1_nested_50x100: Mean +- std dev: 219 ms +- 32 ms

Check notice on line 1 in python/langsmith/client.py

View workflow job for this annotation

GitHub Actions / benchmark

Comparison against main 

+-----------------------------------------------+---------+-----------------------+
| Benchmark                                     | main    | changes               |
+===============================================+=========+=======================+
| dumps_class_nested_py_leaf_100x200            | 113 ms  | 111 ms: 1.02x faster  |
+-----------------------------------------------+---------+-----------------------+
| dumps_class_nested_py_leaf_50x100             | 27.3 ms | 26.9 ms: 1.01x faster |
+-----------------------------------------------+---------+-----------------------+
| dumps_dataclass_nested_50x100                 | 27.9 ms | 27.8 ms: 1.01x faster |
+-----------------------------------------------+---------+-----------------------+
| dumps_class_nested_py_branch_and_leaf_200x400 | 762 us  | 769 us: 1.01x slower  |
+-----------------------------------------------+---------+-----------------------+
| Geometric mean                                | (ref)   | 1.00x faster          |
+-----------------------------------------------+---------+-----------------------+

Benchmark hidden because not significant (5): dumps_pydantic_nested_50x100, dumps_pydanticv1_nested_50x100, create_5_000_run_trees, create_10_000_run_trees, create_20_000_run_trees

Use the client to customize API keys / workspace ocnnections, SSl certs,
etc. for tracing.
Expand Down Expand Up @@ -731,7 +731,11 @@

return self._settings

def _content_above_size(self, content_length: Optional[int]) -> Optional[str]:
def _content_above_size(
self,
content_length: Optional[int],
request: Optional[requests.Request | requests.PreparedRequest],
) -> Optional[str]:
if content_length is None or self._info is None:
return None
info = cast(ls_schemas.LangSmithInfo, self._info)
Expand All @@ -742,6 +746,14 @@
if size_limit is None:
return None
if content_length > size_limit:
should_debug_crash_dump = os.getenv("LANGSMITH_DEBUG_CRASH_DUMP") in [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use get_env_var in utils to cache this

Copy link
Collaborator

@hinthornw hinthornw Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt of having the value be a file path?
LANGSMITH_DEBUG_CRASH_DUMP=my_file.jsonl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I like that!

"1",
"true",
]
if should_debug_crash_dump and request is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't belong in a function that checks content length - it should go outside as the next step.

Also I think we would benefit from seeing other things in the dump even if the payload size doesn't surpass (since these are also of interest)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should always dump? Had intended this to only dump if we exceeded the content length limit (and also potentially dump here if other errors occur)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only on error, but this is intended for debugging connection errors, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes it's a connection error but specifically the one triggered by the content length limit.

Failed to batch ingest runs: langsmith.utils.LangSmithConnectionError: Connection error caused failure to POST https://api.smith.langchain.com/runs/batch in LangSmith API. The content length of 270646872 bytes exceeds the maximum size limit of 20971520 bytes.

Their input and output tokens and traces seem to be pretty small, so we wanted to see what else was in their payload which was causing them to exceed 20mb.

We can also log more generically on ConnectionError though if you think that's better! It does make sense to log for other types of connection errors too

with open("content_size_limit_crash_dump.jsonl", "a") as f:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to specify 'utf-8' as the encoding.
Also, let's use gzip - by definition these will get pretty big pretty fast

json.dump(request, f)
f.write("\n")
return (
f"The content length of {content_length} bytes exceeds the "
f"maximum size limit of {size_limit} bytes."
Expand Down Expand Up @@ -918,7 +930,7 @@
if e.request
else ""
)
size_rec = self._content_above_size(content_length)
size_rec = self._content_above_size(content_length, e.request)
if size_rec:
recommendation = size_rec
except ValueError:
Expand Down
Loading