Py-SDK: More kw-args (#11384)

Co-authored-by: Nick <[email protected]>

Author: emilk

crates/build/re_types_builder/src/codegen/python/mod.rs

@@ -2,11 +2,14 @@
 
 mod views;
 
-use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
+use std::{
+    collections::{BTreeMap, BTreeSet, HashMap, HashSet},
+    iter,
+};
 
 use anyhow::Context as _;
 use camino::{Utf8Path, Utf8PathBuf};
-use itertools::Itertools as _;
+use itertools::{Itertools as _, chain};
 use unindent::unindent;
 
 use crate::{
@@ -267,7 +270,7 @@ impl ExtensionClass {
                 .collect();
 
             let valid_converter_overrides = if obj.is_union() {
-                itertools::Either::Left(std::iter::once("inner"))
+                itertools::Either::Left(iter::once("inner"))
             } else {
                 itertools::Either::Right(obj.fields.iter().map(|field| field.name.as_str()))
             }
@@ -2434,10 +2437,13 @@ fn compute_init_parameters(obj: &Object, objects: &Objects) -> Vec<String> {
     // If the type is fully transparent (single non-nullable field and not an archetype),
     // we have to use the "{obj.name}Like" type directly since the type of the field itself might be too narrow.
     // -> Whatever type aliases there are for this type, we need to pick them up.
-    if obj.kind != ObjectKind::Archetype && obj.fields.len() == 1 && !obj.fields[0].is_nullable {
+    if obj.kind != ObjectKind::Archetype
+        && let [single_field] = obj.fields.as_slice()
+        && !single_field.is_nullable
+    {
         vec![format!(
             "{}: {}",
-            obj.fields[0].name,
+            single_field.name,
             quote_parameter_type_alias(&obj.fqname, &obj.fqname, objects, false)
         )]
     } else if obj.is_union() {
@@ -2460,17 +2466,19 @@ fn compute_init_parameters(obj: &Object, objects: &Objects) -> Vec<String> {
             .map(|field| quote_init_parameter_from_field(field, objects, &obj.fqname))
             .collect_vec();
 
-        if optional.is_empty() {
-            required
-        } else if obj.kind == ObjectKind::Archetype {
-            // Force kw-args for all optional arguments:
+        if 2 < required.len() {
+            // There's a lot of required arguments.
+            // Using positional arguments would make usage hard to read.
+            // better for force kw-args for ALL arguments:
+            chain!(std::iter::once("*".to_owned()), required, optional).collect()
+        } else if optional.is_empty() {
             required
-                .into_iter()
-                .chain(std::iter::once("*".to_owned()))
-                .chain(optional)
-                .collect()
+        } else if obj.name == "AnnotationInfo" {
+            // TODO(#6836): rewrite AnnotationContext
+            chain!(required, optional).collect()
         } else {
-            required.into_iter().chain(optional).collect()
+            // Force kw-args for all optional arguments:
+            chain!(required, std::iter::once("*".to_owned()), optional).collect()
         }
     }
 }
@@ -2514,10 +2522,9 @@ fn quote_init_method(
     ext_class: &ExtensionClass,
     objects: &Objects,
 ) -> String {
-    let parameters = compute_init_parameters(obj, objects);
     let head = format!(
         "def __init__(self: Any, {}) -> None:",
-        parameters.join(", ")
+        compute_init_parameters(obj, objects).join(", ")
     );
 
     let parameter_docs = compute_init_parameter_docs(reporter, obj, objects);

docs/content/reference/migration/migration-0-26.md

@@ -10,6 +10,26 @@ For non-blocking, use `timeout_sec=0`.
 Mostly you can just call `.flush()` with no arguments.
 That will block until all writes either finishes or an error occurs (e.g. the gRPC connection is severed).
 
+## Python SDK: more use of kw-args
+We have started using named arguments (kw-args) for more of our functions.
+This will make it easier for us to evolve our APIs in the future, when adding new arguments, or renaming old ones.
+
+Before:
+```py
+rr.ImageFormat(width, height, "YUV420")
+
+blueprint.spawn("my_app", 1234)
+```
+
+After:
+```py
+rr.ImageFormat(width, height, pixel_format="YUV420")
+
+blueprint.spawn("my_app", port=1234)
+```
+
+[ruff](https://github.com/astral-sh/ruff) (or your preferred Python linter) will find all the places in your code that need to be updated!
+
 ## Python DataFusion interface: update to 49.0.0
 The DataFusion FFI that we rely on for user defined functions and
 table providers requires users to upgrade their `datafusion-python`

docs/snippets/compare_snippet_output.py

@@ -133,7 +133,7 @@ def main() -> None:
         build_cpp_snippets()
 
     # Always build rust since we use it as the baseline for comparison.
-    build_rust_snippets(build_env, args.release, args.target, args.target_dir)
+    build_rust_snippets(build_env=build_env, release=args.release, target=args.target, target_dir=args.target_dir)
 
     examples = []
     if len(args.example) > 0:
@@ -281,13 +281,17 @@ def run_example(example: Example, language: str, args: argparse.Namespace) -> No
         python_output_path = run_python(example)
         check_non_empty_rrd(python_output_path)
     elif language == "rust":
-        rust_output_path = run_prebuilt_rust(example, args.release, args.target, args.target_dir)
+        rust_output_path = run_prebuilt_rust(
+            example, release=args.release, target=args.target, target_dir=args.target_dir
+        )
         check_non_empty_rrd(rust_output_path)
     else:
         raise AssertionError(f"Unknown language: {language}")
 
 
-def build_rust_snippets(build_env: dict[str, str], release: bool, target: str | None, target_dir: str | None) -> None:
+def build_rust_snippets(
+    *, build_env: dict[str, str], release: bool, target: str | None, target_dir: str | None
+) -> None:
     print("----------------------------------------------------------")
     print("Building snippets for Rust…")
 
@@ -343,7 +347,7 @@ def run_python(example: Example) -> str:
     return output_path
 
 
-def run_prebuilt_rust(example: Example, release: bool, target: str | None, target_dir: str | None) -> str:
+def run_prebuilt_rust(example: Example, *, release: bool, target: str | None, target_dir: str | None) -> str:
     output_path = example.output_path("rust")
 
     extension = ".exe" if os.name == "nt" else ""

pixi.lock

@@ -155,6 +155,7 @@ lint.select = [
   "I",       # Isort
   "ISC",     # Ensure implicit string concat syntax
   "PIE",     # flake8-pic: various idomatic python lints
+  "PLR0917", # too-many-positional-parameters
   "PLW1514", # Force setting `encoding` for open calls. This is in order to prevent issues when opening utf8 files on windows where the default encoding may depend on the active locale. https://docs.astral.sh/ruff/rules/unspecified-encoding/
   "RUF",     # Ruff-specific rules
   "RUF027",  # Ensure that strings which look like format-strings have the `f` prefix
@@ -176,6 +177,9 @@ lint.unfixable = [
   "PLW1514", # Automatic fix for `encoding` doesn't do what we want - it queries the locale for the preferred encoding which is exactly what we want to avoid.
 ]
 
+[tool.ruff.lint.pylint]
+max-positional-args = 3
+
 [tool.ruff.lint.per-file-ignores]
 "*" = ["RUF012"] # TODO(emilk): consider enabling this
 
@@ -193,6 +197,23 @@ lint.unfixable = [
   "RUF001",
 ]
 
+"examples/*" = [
+  # We don't care about nice APIs in our examples:
+  "PLR0917",
+]
+"scripts/*" = [
+  # We don't care about nice APIs in our scripts:
+  "PLR0917",
+]
+"rerun_py/tests/*" = [
+  # We don't care about nice APIs in our tests:
+  "PLR0917",
+]
+"tests/python/*" = [
+  # We don't care about nice APIs in our tests:
+  "PLR0917",
+]
+
 [tool.ruff.lint.isort]
 required-imports = ["from __future__ import annotations"]
 combine-as-imports = true                                 # needed so keep rerun_sdk/__init__.py clean

rerun_py/pyproject.toml

@@ -247,6 +247,7 @@ def send_columns(
     entity_path: str,
     indexes: Iterable[TimeColumnLike],
     columns: Iterable[ComponentColumn],
+    *,
     recording: RecordingStream | None = None,
     strict: bool | None = None,  # noqa: ARG001 - `strict` handled by `@catch_and_log_exceptions`
 ) -> None:

rerun_py/rerun_sdk/rerun/_send_columns.py

@@ -127,7 +127,7 @@ class AnyBatchValue(ComponentBatchLike):
     See also [rerun.AnyValues][].
     """
 
-    def __init__(self, descriptor: str | ComponentDescriptor, value: Any, drop_untyped_nones: bool = True) -> None:
+    def __init__(self, descriptor: str | ComponentDescriptor, value: Any, *, drop_untyped_nones: bool = True) -> None:
         """
         Construct a new AnyBatchValue.
 
@@ -248,5 +248,5 @@ def column(
             previously logged with a type.
 
         """
-        inst = cls(descriptor, value, drop_untyped_nones)
+        inst = cls(descriptor, value, drop_untyped_nones=drop_untyped_nones)
         return ComponentColumn(descriptor, inst)

rerun_py/rerun_sdk/rerun/any_batch_value.py

@@ -84,12 +84,12 @@ def __init__(self, drop_untyped_nones: bool = True, **kwargs: Any) -> None:
         "`rr.AnyValues.with_field` is deprecated, use `rr.AnyValues.with_component_from_data` instead.",
     )
     def with_field(
-        self, descriptor: str | ComponentDescriptor, value: Any, drop_untyped_nones: bool = True
+        self, descriptor: str | ComponentDescriptor, value: Any, *, drop_untyped_nones: bool = True
     ) -> AnyValues:
-        return self.with_component_from_data(descriptor, value, drop_untyped_nones)
+        return self.with_component_from_data(descriptor, value, drop_untyped_nones=drop_untyped_nones)
 
     def with_component_from_data(
-        self, descriptor: str | ComponentDescriptor, value: Any, drop_untyped_nones: bool = True
+        self, descriptor: str | ComponentDescriptor, value: Any, *, drop_untyped_nones: bool = True
     ) -> AnyValues:
         """Adds an `AnyValueBatch` to this `AnyValues` bundle."""
         # TODO(#10908): Prune this type in 0.26
@@ -108,13 +108,15 @@ def with_component_from_data(
     @deprecated(
         "`rr.AnyValues.with_component` is deprecated, use `rr.AnyValues.with_component_override` instead.",
     )
-    def with_component(self, field: str, component_type: str, value: Any, drop_untyped_nones: bool = True) -> AnyValues:
+    def with_component(
+        self, field: str, component_type: str, value: Any, *, drop_untyped_nones: bool = True
+    ) -> AnyValues:
         """Adds an `AnyValueBatch` to this `AnyValues` bundle with name and component type."""
         self._builder.with_component_override(field, component_type, value, drop_untyped_nones=drop_untyped_nones)
         return self
 
     def with_component_override(
-        self, field: str, component_type: str, value: Any, drop_untyped_nones: bool = True
+        self, field: str, component_type: str, value: Any, *, drop_untyped_nones: bool = True
     ) -> AnyValues:
         """Adds an `AnyValueBatch` to this `AnyValues` bundle with name and component type."""
         self._builder.with_component_override(field, component_type, value, drop_untyped_nones=drop_untyped_nones)