Uh oh!
There was an error while loading. Please reload this page.
gh-154194: Degrade frames in Tachyon instead of failing the sample - #154195
gh-154194: Degrade frames in Tachyon instead of failing the sample#154195maurycy wants to merge 7 commits into
Conversation
I believe that the CI failures are caused by the fact that |
pablogsal
left a comment
There was a problem hiding this comment.
I still need more time but some nits
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| # Build coroutine stack string | ||
| frames = [frame for coro in task_info.coroutine_stack | ||
| for frame in coro.call_stack] | ||
| coro_stack = " -> ".join(_format_stack_entry(x).split(" ")[0] |
There was a problem hiding this comment.
This does not work for the <unknown function> sentinel.
| def _format_stack_entry(elem: str|FrameInfo) -> str: | ||
| if not isinstance(elem, str): |
There was a problem hiding this comment.
I believe that isinstance(elem, str) is dead since #135436. Perhaps we should clean this defensive code everywhere.
| # ─── indexing helpers ─────────────────────────────────────────── | ||
| def _format_stack_entry(elem: str|FrameInfo) -> str: | ||
| if not isinstance(elem, str): | ||
| if elem.location is None: |
There was a problem hiding this comment.
Regular collectors rely on these helpers:
cpython/Lib/profiling/sampling/collector.py
Lines 46 to 47 in 40f7fbf
cpython/Lib/profiling/sampling/collector.py
Lines 29 to 30 in 40f7fbf
| PyObject *func = NULL; | ||
| PyObject *file = NULL; | ||
| PyObject *linetable = NULL; | ||
| int code_metadata_incomplete = 0; |
There was a problem hiding this comment.
Perhaps parse_code_object() should return this information, and we should not store incomplete objects in the frame_cache?
Perhaps we should collect degradation stats? Important caveat is that we will likely have more types of degradation.
| "<GC>", | ||
| "<native>", | ||
| "<unknown function>", | ||
| "<unknown file>", | ||
| "<unreadable frame>", |
There was a problem hiding this comment.
Perhaps we could add something like is_synthetic to FrameInfo?
As always, see #154194 for the context. Shortly: right now remote inspection fails the whole sample when it hits the limits (and we're really not sure what are the best limits) ORalways when reading a part fails. This is too strict, wasteful and introduces scary bias, as observed in https://discuss.python.org/t/tachyon-97-error-rate/107619.
The PR introduces graceful degradation with the sentinels like
<unreadable frame>(funcname),<unknown function>(funcname),<unknown file>(filename) and the(len=N)suffix if we hit the cap. Also, attemps to better differentiate between transient failures in_Py_RemoteDebug_IsFatalReadError().Importantly: degraded frames are never cached, while the truncated strings are cached.
_remote_debugging: Degrade gracefully reads instead of failing the whole read completely #154194