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.

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

Work around some issues where recording streams leaking context when used with generators #6240

Merged
merged 4 commits into from
May 7, 2024

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented May 6, 2024

What

In thread_local_stream, don't yield while holding the stream context open. Re-create context before continuing the generator.

Also introduce a new recording_stream_generator_ctx for more advanced usage. This is mainly an escape hatch.

The problem from the example can now be handled using:

import rerun as rr

@rr.recording_stream_generator_ctx
def my_gen_func(stream, name):
    with stream:
        for i in range(10):
            print(f"{name} {i}")
            rr.log("stream", rr.TextLog(f"{name} {i}"))
            yield

rr.init("rerun_example_leak_context")

stream1 = rr.new_recording("rerun_example_stream1")
stream1.save("stream1.rrd")
stream2 = rr.new_recording("rerun_example_stream2")
stream2.save("stream2.rrd")

gen1 = my_gen_func(stream1, "stream1")
gen2 = my_gen_func(stream2, "stream2")

next(gen1)
next(gen2)
rr.log("stream", rr.TextLog("This should go to the global stream"))
next(gen1)
next(gen1)
next(gen1)

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

To run all checks from main, comment on the PR with @rerun-bot full-check.

@jleibs jleibs added 🐍 Python API Python logging API 🪳 bug Something isn't working exclude from changelog PRs with this won't show up in CHANGELOG.md labels May 6, 2024
@jleibs jleibs marked this pull request as ready for review May 6, 2024 18:38
@jleibs jleibs force-pushed the jleibs/stream_context_generators branch from 0d4b52f to 8e7c6f3 Compare May 6, 2024 20:15
@jleibs jleibs changed the title Fix issue with the thread_local_stream leaking context Work around some issues with recording streams leaking context when used with generators May 6, 2024
@jleibs jleibs added include in changelog and removed exclude from changelog PRs with this won't show up in CHANGELOG.md labels May 6, 2024
@jleibs jleibs force-pushed the jleibs/stream_context_generators branch 2 times, most recently from bbd4043 to 5977f52 Compare May 6, 2024 20:36
@jleibs jleibs force-pushed the jleibs/stream_context_generators branch from 5977f52 to 935f63c Compare May 6, 2024 20:39
@jleibs jleibs changed the title Work around some issues with recording streams leaking context when used with generators Work around some issues where recording streams leaking context when used with generators May 7, 2024
@abey79 abey79 self-requested a review May 7, 2024 12:33
Copy link
Member

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

Great stuff! Extensively discussed in a huddle. TLDR: some more comments welcome and the __enter__ call in the finally clause of recording_stream_generator_ctx needs revisiting.

@jleibs jleibs merged commit 9032b01 into main May 7, 2024
34 checks passed
@jleibs jleibs deleted the jleibs/stream_context_generators branch May 7, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working include in changelog 🐍 Python API Python logging API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RecordingStream context managers are broken when used with generators
2 participants