Spans: capture code snippets

[crusaderky]

Part of #7860

[github-actions]

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       20 files  +       1         20 suites  +1   12h 56m 41s ⏱️ + 40m 1s
  3 689 tests +       2    3 578 ✔️ +       3     106 💤 ±  0  5 ❌  - 1 
35 680 runs  +1 360  33 921 ✔️ +1 275  1 754 💤 +86  5 ❌  - 1 

For more details on these failures, see this check.

Results for commit 7f8c0fd. ± Comparison against base commit 49437c2.

♻️ This comment has been updated with latest results.

[fjetter]

I'm OK moving forward even without the "split code by span" hack I mentioned in the thread. @crusaderky up to you

[fjetter]

Based on my testing, all spans have the same code. This is not a blocker but a bit unforunate.

I played briefly with ast to see if we can improve this. This code is a bit of a hack but works and we could apply something like this on a best-effort basis to only attach the code that is relevant to the given span.

I'm basing this on the span function name in the contextmanager. This could be refined further but should be good enough for most cases.

import ast
def parse_frame_by_span(frame):
    code_by_span = {}
    ast_fr = ast.parse(inspect.getsource(fr))
    for node in ast.walk(ast_fr):
        if isinstance(node, ast.With) and node.items[0].context_expr.func.id == "span":
            try:
                # This is a literal
                label = node.items[0].context_expr.args[0].value
            except:
                # This is a variable. We can't get all of them, only whatever
                # is stored in the locals at the time. Still, nice.
                label = node.items[0].context_expr.args[0].id
                label = fr.f_locals[label]
            code_by_span[label] = ast.unparse(node)
    return code_by_span

[mrocklin]

Disclaimer: I haven't looked at this PR at all, so sorry for the potentially uninformed comment.

I'm pretty ok capturing more code than we need to if we can also capture the line number. Getting line numbers would, I think, help in many cases that I've seen. Often a big frame has many different compute calls within it (such as in a script or a large Jupyter cell). Getting line numbers would help us to disambiguate.

This is an expansion of scope, so maybe it makes sense to defer to another PR, but I thought I'd mention it as an alternative (or addition to) Florian's suggestion above (although I like Florian's thought as well).

[fjetter]

Getting line numbers would help us to disambiguate.

We should be capturing the line of code already since #7786 (The fact that this is not shown on the Coiled UI, yet, is a Coiled issue)

[mrocklin]

Ah ok, grand. Sorry for the distraction

…

On Tue, Jun 20, 2023, 8:22 AM Florian Jetter ***@***.***> wrote: Getting line numbers would help us to disambiguate. We should be capturing the line of code already since #7786 <#7786> (The fact that this is not shown on the Coiled UI, yet, is a Coiled issue) — Reply to this email directly, view it on GitHub <#7930 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACKZTA26RP4ULS4YQLQQMLXMGP3DANCNFSM6AAAAAAZLSAARU> . You are receiving this because you commented.Message ID: ***@***.***>

[crusaderky]

I would rather keep the scope as small as possible and just ensure that spans receive the same code as computations here.

I think that the AST code snippet above is quite fragile. Namely, it wouldn't work with the @span decorators. The round-trip text->AST->text will make it very likely that a copy-paste from the dashboard to your IDE's search box won't give you the expected results - particularly if you're not using black.

[crusaderky]

I've updated the test to reflect that the three frames are different in line number