Skip to content

Commit

Permalink
Lint and fix python SDK (Py)RecordingStream upcasting issues (#8184)
Browse files Browse the repository at this point in the history
* Fixes #8167

It is unapologetically disgusting, but at least it works... Someone can
come up with a better solution later, I hope?

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/8184?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/8184?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/8184)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

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

To deploy documentation changes immediately after merging this PR, add
the `deploy docs` label.
  • Loading branch information
teh-cmc authored Nov 25, 2024
1 parent 317319c commit d1b80ae
Show file tree
Hide file tree
Showing 14 changed files with 149 additions and 84 deletions.
12 changes: 12 additions & 0 deletions docs/snippets/all/concepts/explicit_recording.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
"""Just makes sure that explicit recordings actually work."""

import os

import rerun as rr

rec = rr.new_recording("rerun_example_explicit_recording")

rr.log("points", rr.Points3D([[0, 0, 0], [1, 1, 1]]), recording=rec)

dir = os.path.dirname(os.path.abspath(__file__))
rr.log_file_from_path(os.path.join(dir, "../../../../tests/assets/cube.glb"), recording=rec)
4 changes: 4 additions & 0 deletions docs/snippets/snippets.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
# You should only ever use this if the test isn't implemented and cannot yet be implemented
# for one or more specific SDKs.
[opt_out.run]
"concepts/explicit_recording" = [ # python-specific check
"cpp",
"rust",
]
"concepts/static/log_static" = [ # pseudo-code
"py",
"cpp",
Expand Down
2 changes: 1 addition & 1 deletion rerun_notebook/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 4 additions & 7 deletions rerun_py/rerun_sdk/rerun/_log.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ def log(
components=components,
num_instances=num_instances,
static=static,
recording=recording,
recording=recording, # NOLINT
)


Expand Down Expand Up @@ -253,9 +253,6 @@ def log_components(
)
static = True

# Convert to a native recording
recording = RecordingStream.to_native(recording)

instanced: dict[str, pa.Array] = {}

components = list(components)
Expand Down Expand Up @@ -294,7 +291,7 @@ def log_components(
entity_path,
components=instanced,
static_=static,
recording=recording,
recording=recording.to_native() if recording is not None else None,
)


Expand Down Expand Up @@ -358,7 +355,7 @@ def log_file_from_path(
Path(file_path),
entity_path_prefix=entity_path_prefix,
static_=static,
recording=recording,
recording=recording.to_native() if recording is not None else None,
)


Expand Down Expand Up @@ -419,7 +416,7 @@ def log_file_from_contents(
file_contents,
entity_path_prefix=entity_path_prefix,
static_=static,
recording=recording,
recording=recording.to_native() if recording is not None else None,
)


Expand Down
2 changes: 1 addition & 1 deletion rerun_py/rerun_sdk/rerun/_send_columns.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,5 +258,5 @@ def send_columns(
entity_path,
timelines={t.timeline_name(): t.as_arrow_array() for t in times},
components=components_args,
recording=recording,
recording=recording.to_native() if recording is not None else None,
)
2 changes: 1 addition & 1 deletion rerun_py/rerun_sdk/rerun/error_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def _send_warning_or_raise(
# TODO(jleibs): Context/stack should be its own component.
context_descriptor = _build_warning_context_string(skip_first=depth_to_user_code + 1)

log("rerun", TextLog(text=f"{message}\n{context_descriptor}", level="WARN"), recording=recording)
log("rerun", TextLog(text=f"{message}\n{context_descriptor}", level="WARN"), recording=recording) # NOLINT
_rerun_exception_ctx.sending_warning = False
else:
warnings.warn(
Expand Down
9 changes: 7 additions & 2 deletions rerun_py/rerun_sdk/rerun/legacy_notebook.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def as_html(
if blueprint is not None:
output_stream.send_blueprint(blueprint, make_active=True) # type: ignore[attr-defined]

data_memory = memory_recording(recording=recording)
data_memory = memory_recording(recording=recording) # NOLINT
output_memory = output_stream.memory_recording() # type: ignore[attr-defined]

base64_data = base64.b64encode(output_memory.storage.concat_as_bytes(data_memory.storage)).decode("utf-8")
Expand Down Expand Up @@ -165,7 +165,12 @@ def legacy_notebook_show(
"""
html = as_html(
width=width, height=height, app_url=app_url, timeout_ms=timeout_ms, blueprint=blueprint, recording=recording
width=width,
height=height,
app_url=app_url,
timeout_ms=timeout_ms,
blueprint=blueprint,
recording=recording, # NOLINT
)
try:
from IPython.core.display import HTML
Expand Down
7 changes: 5 additions & 2 deletions rerun_py/rerun_sdk/rerun/memory.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,11 @@ def memory_recording(recording: RecordingStream | None = None) -> MemoryRecordin
"""

recording = RecordingStream.to_native(recording)
return MemoryRecording(bindings.memory_recording(recording=recording))
return MemoryRecording(
bindings.memory_recording(
recording=recording.to_native() if recording is not None else None,
)
)


class MemoryRecording:
Expand Down
2 changes: 1 addition & 1 deletion rerun_py/rerun_sdk/rerun/notebook.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,6 @@ def notebook_show(
width=width,
height=height,
blueprint=blueprint,
recording=recording,
recording=recording, # NOLINT
)
viewer.display()
24 changes: 12 additions & 12 deletions rerun_py/rerun_sdk/rerun/recording_stream.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ def new_recording(
if spawn:
from rerun.sinks import spawn as _spawn

_spawn(recording=recording)
_spawn(recording=recording) # NOLINT

return recording

Expand Down Expand Up @@ -271,7 +271,7 @@ def __del__(self): # type: ignore[no-untyped-def]
#
# See: https://github.com/rerun-io/rerun/issues/6223 for context on why this is necessary.
if recording is not None and not recording.is_forked_child():
bindings.flush(blocking=False, recording=recording)
bindings.flush(blocking=False, recording=recording) # NOLINT


def binary_stream(recording: RecordingStream | None = None) -> BinaryStream:
Expand Down Expand Up @@ -307,9 +307,7 @@ def binary_stream(recording: RecordingStream | None = None) -> BinaryStream:
An object that can be used to flush or read the data.
"""

recording = RecordingStream.to_native(recording)
return BinaryStream(bindings.binary_stream(recording=recording))
return BinaryStream(bindings.binary_stream(recording=recording.to_native() if recording is not None else None))


class BinaryStream:
Expand Down Expand Up @@ -382,7 +380,7 @@ def is_enabled(
This can be controlled with the environment variable `RERUN` (e.g. `RERUN=on` or `RERUN=off`).
"""
return bindings.is_enabled(recording=RecordingStream.to_native(recording)) # type: ignore[no-any-return]
return bindings.is_enabled(recording=recording.to_native() if recording is not None else None) # type: ignore[no-any-return]


def get_application_id(
Expand All @@ -404,7 +402,7 @@ def get_application_id(
The application ID that this recording is associated with.
"""
app_id = bindings.get_application_id(recording=RecordingStream.to_native(recording))
app_id = bindings.get_application_id(recording=recording.to_native() if recording is not None else None)
return str(app_id) if app_id is not None else None


Expand Down Expand Up @@ -436,7 +434,7 @@ def get_recording_id(
The recording ID that this recording is logging to.
"""
rec_id = bindings.get_recording_id(recording=RecordingStream.to_native(recording))
rec_id = bindings.get_recording_id(recording=recording.to_native() if recording is not None else None)
return str(rec_id) if rec_id is not None else None


Expand Down Expand Up @@ -469,7 +467,7 @@ def get_data_recording(
The most appropriate recording to log data to, in the current context, if any.
"""
result = bindings.get_data_recording(recording=RecordingStream.to_native(recording))
result = bindings.get_data_recording(recording=recording.to_native() if recording is not None else None)
return RecordingStream(result) if result is not None else None


Expand Down Expand Up @@ -497,7 +495,7 @@ def set_global_data_recording(recording: RecordingStream) -> RecordingStream | N
The newly active global recording.
"""
result = bindings.set_global_data_recording(RecordingStream.to_native(recording))
result = bindings.set_global_data_recording(recording.to_native())
return RecordingStream(result) if result is not None else None


Expand All @@ -515,7 +513,7 @@ def get_thread_local_data_recording() -> RecordingStream | None:
return RecordingStream(result) if result is not None else None


def set_thread_local_data_recording(recording: RecordingStream) -> RecordingStream | None:
def set_thread_local_data_recording(recording: RecordingStream | None) -> RecordingStream | None:
"""
Replaces the currently active thread-local recording with the specified one.
Expand All @@ -525,7 +523,9 @@ def set_thread_local_data_recording(recording: RecordingStream) -> RecordingStre
The newly active thread-local recording.
"""
result = bindings.set_thread_local_data_recording(recording=RecordingStream.to_native(recording))
result = bindings.set_thread_local_data_recording(
recording=recording.to_native() if recording is not None else None,
)
return RecordingStream(result) if result is not None else None


Expand Down
61 changes: 38 additions & 23 deletions rerun_py/rerun_sdk/rerun/sinks.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ def connect(
)

return connect_tcp(
addr, flush_timeout_sec=flush_timeout_sec, default_blueprint=default_blueprint, recording=recording
addr,
flush_timeout_sec=flush_timeout_sec,
default_blueprint=default_blueprint,
recording=recording, # NOLINT
)


Expand Down Expand Up @@ -111,7 +114,7 @@ def connect_tcp(
logging.warning("Rerun is disabled - connect() call ignored")
return

application_id = get_application_id(recording=recording)
application_id = get_application_id(recording=recording) # NOLINT
if application_id is None:
raise ValueError(
"No application id found. You must call rerun.init before connecting to a viewer, or provide a recording."
Expand All @@ -124,10 +127,11 @@ def connect_tcp(
application_id=application_id, blueprint=default_blueprint
).storage

recording = RecordingStream.to_native(recording)

bindings.connect_tcp(
addr=addr, flush_timeout_sec=flush_timeout_sec, default_blueprint=blueprint_storage, recording=recording
addr=addr,
flush_timeout_sec=flush_timeout_sec,
default_blueprint=blueprint_storage,
recording=recording.to_native() if recording is not None else None,
)


Expand Down Expand Up @@ -163,7 +167,7 @@ def save(
logging.warning("Rerun is disabled - save() call ignored. You must call rerun.init before saving a recording.")
return

application_id = get_application_id(recording=recording)
application_id = get_application_id(recording=recording) # NOLINT
if application_id is None:
raise ValueError(
"No application id found. You must call rerun.init before connecting to a viewer, or provide a recording."
Expand All @@ -176,9 +180,11 @@ def save(
application_id=application_id, blueprint=default_blueprint
).storage

recording = RecordingStream.to_native(recording)

bindings.save(path=str(path), default_blueprint=blueprint_storage, recording=recording)
bindings.save(
path=str(path),
default_blueprint=blueprint_storage,
recording=recording.to_native() if recording is not None else None,
)


def stdout(default_blueprint: BlueprintLike | None = None, recording: RecordingStream | None = None) -> None:
Expand Down Expand Up @@ -210,7 +216,7 @@ def stdout(default_blueprint: BlueprintLike | None = None, recording: RecordingS
logging.warning("Rerun is disabled - save() call ignored. You must call rerun.init before saving a recording.")
return

application_id = get_application_id(recording=recording)
application_id = get_application_id(recording=recording) # NOLINT
if application_id is None:
raise ValueError(
"No application id found. You must call rerun.init before connecting to a viewer, or provide a recording."
Expand All @@ -223,8 +229,10 @@ def stdout(default_blueprint: BlueprintLike | None = None, recording: RecordingS
application_id=application_id, blueprint=default_blueprint
).storage

recording = RecordingStream.to_native(recording)
bindings.stdout(default_blueprint=blueprint_storage, recording=recording)
bindings.stdout(
default_blueprint=blueprint_storage,
recording=recording.to_native() if recording is not None else None,
)


def disconnect(recording: RecordingStream | None = None) -> None:
Expand All @@ -243,8 +251,9 @@ def disconnect(recording: RecordingStream | None = None) -> None:
"""

recording = RecordingStream.to_native(recording)
bindings.disconnect(recording=recording)
bindings.disconnect(
recording=recording.to_native() if recording is not None else None,
)


@deprecated(
Expand Down Expand Up @@ -309,7 +318,7 @@ def serve(
web_port=web_port,
ws_port=ws_port,
default_blueprint=default_blueprint,
recording=recording,
recording=recording, # NOLINT
server_memory_limit=server_memory_limit,
)

Expand Down Expand Up @@ -362,7 +371,7 @@ def serve_web(
logging.warning("Rerun is disabled - serve() call ignored")
return

application_id = get_application_id(recording=recording)
application_id = get_application_id(recording=recording) # NOLINT
if application_id is None:
raise ValueError(
"No application id found. You must call rerun.init before connecting to a viewer, or provide a recording."
Expand All @@ -375,15 +384,14 @@ def serve_web(
application_id=application_id, blueprint=default_blueprint
).storage

recording = RecordingStream.to_native(recording)
# TODO(#5531): keep static data around.
bindings.serve_web(
open_browser,
web_port,
ws_port,
server_memory_limit=server_memory_limit,
default_blueprint=blueprint_storage,
recording=recording,
recording=recording.to_native() if recording is not None else None,
)


Expand Down Expand Up @@ -417,16 +425,19 @@ def send_blueprint(
See also: [`rerun.init`][], [`rerun.set_global_data_recording`][].
"""
application_id = get_application_id(recording=recording)
application_id = get_application_id(recording=recording) # NOLINT

if application_id is None:
raise ValueError("No application id found. You must call rerun.init before sending a blueprint.")

recording = RecordingStream.to_native(recording)

blueprint_storage = create_in_memory_blueprint(application_id=application_id, blueprint=blueprint).storage

bindings.send_blueprint(blueprint_storage, make_active, make_default, recording=recording)
bindings.send_blueprint(
blueprint_storage,
make_active,
make_default,
recording=recording.to_native() if recording is not None else None,
)


def spawn(
Expand Down Expand Up @@ -477,4 +488,8 @@ def spawn(
_spawn_viewer(port=port, memory_limit=memory_limit, hide_welcome_screen=hide_welcome_screen)

if connect:
connect_tcp(f"127.0.0.1:{port}", recording=recording, default_blueprint=default_blueprint)
connect_tcp(
f"127.0.0.1:{port}",
recording=recording, # NOLINT
default_blueprint=default_blueprint,
)
Loading

0 comments on commit d1b80ae

Please sign in to comment.