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

Autogenerated partial updates APIs for Python #8671

Merged
merged 18 commits into from
Jan 15, 2025
Merged

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Jan 13, 2025

There really is not much to be said that wouldn't be made clearer by just looking at the code.

Specifically:

  • Look at the codegen changes.
  • Now look at the changes to one of the generated archetypes (probably Transform3D).
  • Now look especially at the changes to transform3d_ext. That one's important.
  • Now look at how the different "partial updates" snippets evolved.

Overall I hate everything here, literally every single bit -- but that's the best I managed to get out of the status quo without ridiculous amounts of efforts.

Copy link

github-actions bot commented Jan 13, 2025

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

  • I have tested the web viewer
Result Commit Link Manifest
6c438ee https://rerun.io/viewer/pr/8671 +nightly +main

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

@teh-cmc teh-cmc force-pushed the cmc/python_partial_updates branch 5 times, most recently from b84de79 to f533df7 Compare January 14, 2025 17:56
@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 14, 2025

@rerun-bot full-check

@teh-cmc teh-cmc changed the base branch from main to cmc/transform_partial_snippets January 14, 2025 18:02
Copy link

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

@teh-cmc teh-cmc changed the title Python partial updates playground Autogenerated partial updates APIs for Python Jan 14, 2025
@teh-cmc teh-cmc added the do-not-merge Do not merge this PR label Jan 14, 2025
@teh-cmc teh-cmc marked this pull request as ready for review January 14, 2025 18:13
@teh-cmc teh-cmc force-pushed the cmc/python_partial_updates branch 3 times, most recently from 9fb8035 to b6b8506 Compare January 15, 2025 09:01
@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 15, 2025

@rerun-bot full-check

Copy link

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

Base automatically changed from cmc/transform_partial_snippets to main January 15, 2025 09:09
@teh-cmc teh-cmc force-pushed the cmc/python_partial_updates branch from b6b8506 to c34467b Compare January 15, 2025 09:09
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Jan 15, 2025
@nikolausWest
Copy link
Member

I have more of a naming feedback: The use of "update" here makes me assume some state is being mutated on an object when what's actually happening is we're calling a class method that returns a list of component batches. Some other naming directions to consider:

rr.log("points", rr.Points3D.fields(clear=True, positions=positions, radii=0.3))
rr.log("points", rr.Points3D.build_fields(clear=True, positions=positions, radii=0.3))
rr.log("points", rr.Points3D.construct_fields(clear=True, positions=positions, radii=0.3))
rr.log("points", rr.Points3D.updating_fields(clear=True, positions=positions, radii=0.3))
rr.log("points", rr.Points3D.incremental_fields(clear=True, positions=positions, radii=0.3))

@teh-cmc teh-cmc force-pushed the cmc/python_partial_updates branch from 7d9630b to 4904bf9 Compare January 15, 2025 09:36
@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 15, 2025

Archetype.fields() is probably best now that we've decided to add a clear: bool parameter to it.

There's still the question of whether Archetype.clear_fields() should be Archetype.clear() instead.

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.

I have so far only looked at the codegen'ed archetypes, and I'm a bit uncomfortable with letting this go as is. I think that, with some reasonable effort, we can make this more self consistent.

First: let's accept that archetype data members are not typed about required-ness, because required-ness is contextual (aka applies only upon first logging an archetype, not updating). So all archetypes field should be: : ComponentXXXBatch | None | [Never].

I don't quite recall with this [] business is about in clear_fields(). I assume it somehow results in clearing that component. Apparently, we can use [Never] as typing hint (as I did above) for list that must be empty. We can try to roll with it 🤷🏻 An alternative would be have some kind of marker object.

Second: I think metadata={"component": "optional"} breaks at least Archetype.num_instances(), so this shouldn't be changed. If that change was required, then we should get to the bottom of it.

Comment on lines 228 to 234
positions=[], # type: ignore[arg-type]
radii=[], # type: ignore[arg-type]
colors=[], # type: ignore[arg-type]
labels=[], # type: ignore[arg-type]
show_labels=[], # type: ignore[arg-type]
class_ids=[], # type: ignore[arg-type]
keypoint_ids=[], # type: ignore[arg-type]
Copy link
Member

Choose a reason for hiding this comment

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

With my proposal above these "ignore" should not be required.

rerun_py/rerun_sdk/rerun/archetypes/points3d.py Outdated Show resolved Hide resolved
metadata={"component": "required"},
converter=components.Position3DBatch._required, # type: ignore[misc]
metadata={"component": "optional"},
converter=components.Position3DBatch._optional, # type: ignore[misc]
Copy link
Member

Choose a reason for hiding this comment

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

Typing wise, all component become optional with my proposal. So we can remove BaseBatch._required and rename BaseBatch._optional to _converter or something.

if clear:
kwargs = {k: v if v is not None else [] for k, v in kwargs.items()} # type: ignore[misc]

return Points3D(**kwargs) # type: ignore[arg-type]
Copy link
Member

Choose a reason for hiding this comment

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

We should use the __new__/__attrs_init__ trick here too. Then I think it should by type checkable even.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to call the real (possibly custom) constructor though.

@abey79
Copy link
Member

abey79 commented Jan 15, 2025

On second thought, I can't find a single place where this Archetype.num_instances() is used. The docstring even lies about it. I'd kill it altogether. The value of "component" seem to matter little in the two other places were it's used, but having it set to "required" makes __str__ more correct, so let's revert that.

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 15, 2025

The lack of | None in the archetype definition itself was not by design -- I missed yet another is_nullable() condition in the codegen. This is fixed now. It removed the need for some of the new type[ignore]s, but apparently it also made new ones appear...? I'll have a quick look.

Copy link

github-actions bot commented Jan 15, 2025

Latest documentation preview deployed successfully.

Result Commit Link
6c438ee https://landing-pvcg1rerw-rerun.vercel.app/docs

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

@abey79
Copy link
Member

abey79 commented Jan 15, 2025

@rerun-bot full-check

Copy link

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.

Okay, I think we're good do go. Let's see what the full check says. As discussed, I would like to have a stronger testing story about that. Hopefully we'll at least built some snippets/roundtrips over time.

To emphasis an important design points (for the benefit of bystanders). This PR provides a codegened, archetype-fields/components-centric update mechanism. As such, update_fields doesn't provide the convenience that some custom __init__ do. For example, Asset3D() accepts a file path, but Asset3D.update_fields() doesn't, because the Asset3D archetype doesn't have a path field (it just has a blob field).

As such, update_fields() can be seen as a lower-level mechanism than the __init__. It is however more compact and convenient than the status quo, which consists of spelling out components explicitly. So this PR is a net gain.

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 15, 2025

@rerun-bot full-check

Copy link

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

@teh-cmc teh-cmc merged commit b19ec4f into main Jan 15, 2025
69 of 71 checks passed
@teh-cmc teh-cmc deleted the cmc/python_partial_updates branch January 15, 2025 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New partial updates logging APIs: Python
3 participants