Python SDK spring cleaning: 3.9, no more monkey patching, more lints #9182

abey79 · 2025-03-04T16:09:19Z

This PR cleans up multiple things:

Enforce Python 3.9 for the SDK (3.8 was deprecated in 0.20)
Remove the nasty monkey patching of stuff in RecordingStream (mostly by resolving the circular import issues)
Fix the use of mutable argument default in the View class hierarchy
Removed as many # type: ignore[....] comment as possible
- Unfortunately, I cannot enable the expect-like behaviour of mypy without make the codegen way too smart about adding these comments (or fixing the underlying logic to avoid needing these comment in the first place).
Enable the B class of lint for Ruff and fix what it caught

Commit-by-commit review suggested:

First commit has the bulk of the interesting RecordingStream stuff
791c685 worth taking a look to as manual work is going there
last two commits are generated from tooling (especially 510c83f) so best filter out that noise

Fixes RecordingStream monkey-patched functions result in mypy errors #5445

github-actions · 2025-03-04T16:10:44Z

Latest documentation preview deployed successfully.

Result	Commit	Link
✅	`0f44d68`	https://landing-i11vyazba-rerun.vercel.app/docs

^{Note: This comment is updated whenever you push a commit.}

abey79 · 2025-03-04T16:11:08Z

rerun_py/rerun_sdk/rerun/_unions.py

-    except ValueError as e:
-        raise ValueError(e.args)


this whole diff is weird but i basically just removed that weird catch/raise thing

github-actions · 2025-03-04T16:11:47Z

Web viewer built successfully. If applicable, you should also test it:

I have tested the web viewer

Result	Commit	Link	Manifest
✅	`0f44d68`	https://rerun.io/viewer/pr/9182	`+nightly` `+main`

^{Note: This comment is updated whenever you push a commit.}

abey79 · 2025-03-04T16:12:08Z

rerun_py/rerun_sdk/rerun/recording_stream.py

the interesting stuff is here, which is why github collapses it by default

Add `set_index` (post-rebase)

Unfortunately, it's not possible to remove the `--no-warn-unused-ignore` mypy flag without making the codegen smarter about which annotation to put when.

…ing 3.8 incompatibility

abey79 · 2025-03-04T16:27:23Z

@rerun-bot full-check

github-actions · 2025-03-04T16:28:01Z

Started a full build: https://github.com/rerun-io/rerun/actions/runs/13658311657

emilk

I'm not a big fan of the 700 lines of new code we need to maintain.

What is the benefit of this duplication?
What are the downsides?
How can we ensure the two APIs don't diverge?

emilk · 2025-03-04T20:21:40Z

examples/python/detect_and_track_objects/detect_and_track_objects.py

@@ -7,9 +7,10 @@
 import json
 import logging
 import os
+from collections.abc import Sequence


why are some of these moved from typing to collection.abc? Is there a lint to catch the wrong usage?

emilk · 2025-03-04T20:22:37Z

examples/python/rgbd/rgbd.py

+            unit_scale=True,
+            unit_divisor=1024,
+        ) as bar,
+    ):


what are the extra () good for ?

I'm not sure, ruff did that, probably following the 3.9 min version.

rerun_py/pyproject.toml

rerun_py/rerun_sdk/rerun/__init__.py

emilk · 2025-03-04T20:26:09Z

rerun_py/rerun_sdk/rerun/archetypes/depth_image_ext.py

@@ -115,7 +115,7 @@ def __init__(
        try:
            datatype = ChannelDatatype.from_np_dtype(image.dtype)
        except KeyError:
-            raise ValueError(f"Unsupported dtype {image.dtype} for DepthImage")
+            raise ValueError(f"Unsupported dtype {image.dtype} for DepthImage") from None


What does raise X from None mean? O.o

This is a bit of an opinionated lint from Ruff, but I like it because it forces developer to learn about that feature ad think of how to use it.

This is the lint description: https://docs.astral.sh/ruff/rules/raise-without-from-inside-except/

Whether or not to chain exception is a logical/semantic question and only the dev can make the call, so the lint rejects the implicit behaviour.

emilk · 2025-03-04T20:27:08Z

rerun_py/rerun_sdk/rerun/blueprint/api.py

-        self.overrides = overrides
+        self.properties = properties if properties is not None else {}
+        self.defaults = defaults if defaults is not None else []
+        self.overrides = overrides if overrides is not None else {}


Sometimes I wish I was writing Lua and could just write overrides or {} instead of all this verbosity…

Well you can, but I don't like it much because of how many things are falsy in python.

emilk · 2025-03-04T20:28:03Z

rerun_py/rerun_sdk/rerun/blueprint/api.py

+        stream.log(self.blueprint_path(), arch)

        for prop_name, prop in self.properties.items():
-            stream.log(f"{self.blueprint_path()}/{prop_name}", prop, recording=stream)  # type: ignore[attr-defined]
+            stream.log(f"{self.blueprint_path()}/{prop_name}", prop)

        for default in self.defaults:
-            if hasattr(default, "as_component_batches"):
-                stream.log(f"{self.blueprint_path()}/defaults", default, recording=stream)  # type: ignore[attr-defined]
-            elif hasattr(default, "component_descriptor"):
-                stream.log(f"{self.blueprint_path()}/defaults", [default], recording=stream)  # type: ignore[attr-defined]
+            if isinstance(default, AsComponents):
+                stream.log(f"{self.blueprint_path()}/defaults", default)
+            elif isinstance(default, ComponentBatchLike):
+                stream.log(f"{self.blueprint_path()}/defaults", [default])  # type: ignore[list-item]
            else:
                raise ValueError(f"Provided default: {default} is neither a component nor a component batch.")

        for path, components in self.overrides.items():
-            stream.log(  # type: ignore[attr-defined]
-                f"{self.blueprint_path()}/ViewContents/individual_overrides/{path}", components, recording=stream
+            stream.log(
+                f"{self.blueprint_path()}/ViewContents/individual_overrides/{path}",
+                components,  # type: ignore[arg-type]
            )


Not sure what happened here, but I assume you know what you're doing

Yes. This is a first step towards removing all these # type: ignore, but there is more work to do.

emilk · 2025-03-04T20:30:38Z

rerun_py/rerun_sdk/rerun/recording_stream.py

+            entity_path_prefix=entity_path_prefix,
+            static=static,
+            recording=self,
+        )


This is a lot of new code. It looks like it is all just forwarding calls - can we forward the docstring too o we don't need to maintain two sets of them?

abey79 · 2025-03-05T09:55:49Z

I'm not a big fan of the 700 lines of new code we need to maintain.

What is the benefit of this duplication? What are the downsides? How can we ensure the two APIs don't diverge?

The primary benefit of this duplication is to make LSP happy with both the stateful and stateless API. For library code, this is very significant and, IMO, outweighs some of the annoyance required.

These docstring cannot be just dynamically duplicated, because the stateful apis don't have a recording argument. This could maybe done by removing the recording argument from the stateful API, which I advocate.

I see this PR as a first step toward a saner situation where the stateless API is first class. Here is my plan, which addresses the duplication (somewhat):

Project: first-class stateless API for the Python SDK #9187

# Conflicts: # rerun_py/rerun_sdk/rerun/_send_columns.py

…e stateless API (#9196) ### Related - Follow up to #9182 ### What Some (too little imo) existing snippets and test explicitly create a recording stream but didn't use its API, relying on the `recording=rec` shenanigans. This PR fixes this.

Currently, our python package is advertised as >=3.9, but our wheels still specify 3.8: <img width="968" alt="image" src="https://github.com/user-attachments/assets/b7d6e5c9-f6ab-44a7-b13f-a32dd655cfff" /> With this PR, our wheels should specify 3.9 as well. related: * #9182 --------- Co-authored-by: Emil Ernerfeldt <[email protected]>

abey79 commented Mar 4, 2025

View reviewed changes

rerun_py/rerun_sdk/rerun/recording_stream.py

Copy link

Member Author

abey79 Mar 4, 2025

Choose a reason for hiding this comment

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

the interesting stuff is here, which is why github collapses it by default

abey79 added 🐍 Python API Python logging API 🚜 refactor Change the code, not the functionality include in changelog labels Mar 4, 2025

abey79 marked this pull request as ready for review March 4, 2025 16:13

abey79 added 8 commits March 4, 2025 17:17

Get rid of the monkey patching on RecordingStream

c6ebc5b

Add `set_index` (post-rebase)

No mutable default arg

35ebced

Silence all mypy errors, test passing

89ce39d

Removed as many # type: ... annotation as possible.

2c91bc2

Unfortunately, it's not possible to remove the `--no-warn-unused-ignore` mypy flag without making the codegen smarter about which annotation to put when.

Enable ruff's B rules and fix all related error / WARNING: introduc…

791c685

…ing 3.8 incompatibility

Remove python 3.8 compatibility

f121c5e

Ruff fixes for 3.9 minimum version

510c83f

pixi.lock post rebase

8f9532d

abey79 force-pushed the antoine/no-monkey-patch branch from be1c7c6 to 8f9532d Compare March 4, 2025 16:17

Somehow send_column managed to escape the initial commit

b375cd5

emilk reviewed Mar 4, 2025

View reviewed changes

abey79 mentioned this pull request Mar 5, 2025

Project: first-class stateless API for the Python SDK #9187

Open

7 tasks

Reintroduce the version check that ruff doesnt like

27ce16d

emilk approved these changes Mar 5, 2025

View reviewed changes

Merge branch 'main' into antoine/no-monkey-patch

0f44d68

# Conflicts: # rerun_py/rerun_sdk/rerun/_send_columns.py

abey79 merged commit 678caad into main Mar 5, 2025
41 checks passed

abey79 deleted the antoine/no-monkey-patch branch March 5, 2025 10:54

abey79 mentioned this pull request Mar 5, 2025

Update test and snippets using new_recording() to _actually_ use the stateless API #9196

Merged

emilk mentioned this pull request Apr 11, 2025

Release 0.23 #9588

Closed

16 tasks

abey79 mentioned this pull request Apr 17, 2025

Set wheels' min python version to 3.9 #9757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python SDK spring cleaning: 3.9, no more monkey patching, more lints #9182

Python SDK spring cleaning: 3.9, no more monkey patching, more lints #9182

abey79 commented Mar 4, 2025 •

edited

Loading

github-actions bot commented Mar 4, 2025 •

edited

Loading

abey79 Mar 4, 2025

github-actions bot commented Mar 4, 2025 •

edited

Loading

abey79 Mar 4, 2025

abey79 commented Mar 4, 2025

github-actions bot commented Mar 4, 2025

emilk left a comment

emilk Mar 4, 2025

emilk Mar 4, 2025

abey79 Mar 5, 2025

emilk Mar 4, 2025

abey79 Mar 5, 2025

emilk Mar 4, 2025

abey79 Mar 5, 2025

emilk Mar 4, 2025

abey79 Mar 5, 2025

emilk Mar 4, 2025

abey79 commented Mar 5, 2025

Python SDK spring cleaning: 3.9, no more monkey patching, more lints #9182

Python SDK spring cleaning: 3.9, no more monkey patching, more lints #9182

Conversation

abey79 commented Mar 4, 2025 • edited Loading

github-actions bot commented Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

abey79 commented Mar 4, 2025

github-actions bot commented Mar 4, 2025

emilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abey79 commented Mar 5, 2025

abey79 commented Mar 4, 2025 •

edited

Loading

github-actions bot commented Mar 4, 2025 •

edited

Loading

github-actions bot commented Mar 4, 2025 •

edited

Loading