-
Notifications
You must be signed in to change notification settings - Fork 384
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
Conversation
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
b84de79
to
f533df7
Compare
@rerun-bot full-check |
Started a full build: https://github.com/rerun-io/rerun/actions/runs/12773618966 |
9fb8035
to
b6b8506
Compare
@rerun-bot full-check |
Started a full build: https://github.com/rerun-io/rerun/actions/runs/12784907004 |
b6b8506
to
c34467b
Compare
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)) |
7d9630b
to
4904bf9
Compare
There's still the question of whether |
There was a problem hiding this 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.
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] |
There was a problem hiding this comment.
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.
metadata={"component": "required"}, | ||
converter=components.Position3DBatch._required, # type: ignore[misc] | ||
metadata={"component": "optional"}, | ||
converter=components.Position3DBatch._optional, # type: ignore[misc] |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
On second thought, I can't find a single place where this |
The lack of |
Latest documentation preview deployed successfully.
Note: This comment is updated whenever you push a commit. |
@rerun-bot full-check |
Started a full build: https://github.com/rerun-io/rerun/actions/runs/12792177087 |
There was a problem hiding this 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.
@rerun-bot full-check |
Started a full build: https://github.com/rerun-io/rerun/actions/runs/12792692784 |
There really is not much to be said that wouldn't be made clearer by just looking at the code.
Specifically:
Transform3D
).transform3d_ext
. That one's important.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.
Transform3D
partial updates snippet for all languages #8690