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

End-to-end tagging: Python #8298

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Dec 3, 2024

This semantically mimics very closely the way things are done in Rust, minus all technical differences due to the differences between both the languages and the SDKs.
For that reason, everything stated in #8304 (comment) basically applies as-is.

Pretty happy about it, I must say.

@teh-cmc teh-cmc added the do-not-merge Do not merge this PR label Dec 3, 2024
Copy link

github-actions bot commented Dec 3, 2024

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

  • I have tested the web viewer
Result Commit Link
9591fd0 https://rerun.io/viewer/pr/8298

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

@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch 3 times, most recently from 1d4d9b6 to 15f6e4d Compare December 3, 2024 18:12
@teh-cmc teh-cmc changed the base branch from main to cmc/end_to_end_tags_rust December 3, 2024 18:17
@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch 2 times, most recently from e00694e to 3e1d785 Compare December 4, 2024 11:25
@teh-cmc teh-cmc force-pushed the cmc/end_to_end_tags_rust branch from a20cd08 to f201277 Compare December 4, 2024 14:41
@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch from 3e1d785 to fd719d0 Compare December 4, 2024 14:53
@teh-cmc teh-cmc changed the base branch from cmc/end_to_end_tags_rust to cmc/end_to_end_tags_cpp December 4, 2024 14:53
@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch 2 times, most recently from e0bc4ca to a59280a Compare December 4, 2024 17:21
@teh-cmc teh-cmc changed the title Tagged components experimentation room End-to-end tagging: Python Dec 4, 2024
@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch from a59280a to 9099039 Compare December 5, 2024 08:36
examples/cpp/dna/main.cpp Outdated Show resolved Hide resolved
@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch 6 times, most recently from f2cd070 to 8bd7ff1 Compare December 5, 2024 14:49
@teh-cmc teh-cmc force-pushed the cmc/end_to_end_tags_cpp branch from e9be108 to 98a76e4 Compare December 5, 2024 15:04
@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch from 8bd7ff1 to 7fe3e87 Compare December 5, 2024 15:05
@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch from 7fe3e87 to 56ef384 Compare December 5, 2024 15:12
@teh-cmc
Copy link
Member Author

teh-cmc commented Dec 5, 2024

@rerun-bot full-check

Copy link

github-actions bot commented Dec 5, 2024

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

@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch from e0d39ae to 56ef384 Compare December 5, 2024 15:43
@teh-cmc
Copy link
Member Author

teh-cmc commented Dec 5, 2024

oh god what on earth is that failure that makes zero sense oO

image

@teh-cmc teh-cmc marked this pull request as ready for review December 5, 2024 17:14
component_name, archetype_name=archetype_name, archetype_field_name=archetype_field_name
)

def or_with_overrides(self, *, archetype_name: str | None, archetype_field_name: str | None) -> ComponentDescriptor:
Copy link
Member

Choose a reason for hiding this comment

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

The exact meaning of this method doesn't feel obvious to me based on the name... esp the fact that the two maybe-override terms act independently.

Do we ever actually need this? Is there an example when we don't just want the unconditional version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've already ended up in the situation of re-tagging data that might or might not be tagged as it reaches me.
It's probably not the right solution for it, but the underlying concern of certainly exists...

I expect these APIs will change quite a lot as we learn more about how things go in practice.


def __init__(self, batch: ComponentBatchLike, descriptor: ComponentDescriptor):
self._batch = batch
self._descriptor = descriptor
Copy link
Member

Choose a reason for hiding this comment

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

Should this check that the batch.component_descriptor().component_name matches the descriptor.component_name?

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 haven't set such rules in the other SDKs -- it is technically legal to override the component name if you know what you're doing... for now.

Comment on lines +44 to +49
rr.DescribedComponentBatch(
confidences,
confidences.component_descriptor().or_with_overrides(
archetype_name="user.CustomPoints3D", archetype_field_name="confidences"
),
)
Copy link
Member

Choose a reason for hiding this comment

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

this pattern feels quite verbose, I'd be tempted to do something like:

Suggested change
rr.DescribedComponentBatch(
confidences,
confidences.component_descriptor().or_with_overrides(
archetype_name="user.CustomPoints3D", archetype_field_name="confidences"
),
)
rr.DescribedComponentBatch(
confidences,
override_archetype="user.CustomPoints3D",
override_field="confidences"
)

and move the confidences.component_descriptor().or_with_overrides(... ) to the implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that was very low-level on purpose -- I don't think end-users should ever see DescribedComponentBatch directly, even.
See:

@@ -43,6 +43,9 @@ def component_name(self) -> str:
def as_arrow_array(self) -> pa.Array:
return self.data

def component_descriptor(self) -> ComponentDescriptor:
return ComponentDescriptor(self.component_name())
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this always set the archetype since we have it?

Suggested change
return ComponentDescriptor(self.component_name())
return ComponentDescriptor(self.component_name(), archetype_name=self._achetype_name)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet -- because other SDKs don't and therefore roundtrips would fail.

But yes, for milestone 2 this will be the case, so that our backwards compat plan can work.

rerun_py/rerun_sdk/rerun/_baseclasses.py Show resolved Hide resolved
@teh-cmc teh-cmc force-pushed the cmc/end_to_end_tags_cpp branch from 7dd6190 to 87ec140 Compare December 9, 2024 09:27
Base automatically changed from cmc/end_to_end_tags_cpp to main December 9, 2024 09:32
@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch from 56ef384 to 77b1f3f Compare December 9, 2024 09:33
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Dec 9, 2024
@teh-cmc teh-cmc force-pushed the cmc/investigating_component_descriptors_chunk_diff branch from 7d83fd1 to 9591fd0 Compare December 9, 2024 09:47
@teh-cmc teh-cmc merged commit a580884 into main Dec 9, 2024
29 checks passed
@teh-cmc teh-cmc deleted the cmc/investigating_component_descriptors_chunk_diff branch December 9, 2024 09:54
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.

2 participants