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

MVP Blueprint APIs for layout and contents #5408

Merged
merged 28 commits into from
Mar 12, 2024
Merged

MVP Blueprint APIs for layout and contents #5408

merged 28 commits into from
Mar 12, 2024

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Mar 5, 2024

What

  • Remove the old experimental blueprint flags from init
  • Introduces a new file rerun_py/rerun_sdk/rerun/blueprint/api.py which defines the beginning of an API surface
  • Create declarative-object-helpers for Viewport, Container, and SpaceView
    • These follow a pattern of recursively logging their content + themselves to a stream.
  • Introduce a new mechanism for optionally sending a blueprint on connect. This is sent inline on the sink before setting up the reset of the stream.
    • On the rust side we just take an optional Vec<LogMsg>
    • On the python side we use the MemorySink as a convenient mechanism of logging to the stream APIs and then re-extracting the data as messages.
  • Fix some assorted errors:
    • space_views should have been optional
    • stream.log apparently never worked

Known issues:

  • There are several fields that we still need to plumb through.
  • The old blueprint example is broken.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@jleibs jleibs changed the base branch from main to andreas/serializable-spaceviewblueprint March 5, 2024 22:04
Base automatically changed from andreas/serializable-spaceviewblueprint to main March 5, 2024 22:26
@jleibs jleibs force-pushed the jleibs/blueprint_test branch 3 times, most recently from 680ac6f to 50d6106 Compare March 7, 2024 21:28
@jleibs jleibs force-pushed the jleibs/blueprint_test branch from 50d6106 to 6664cf3 Compare March 8, 2024 20:10
@jleibs jleibs changed the title WIP: Very trivial blueprint test MVP Blueprint APIs for layout and contents Mar 8, 2024
@jleibs jleibs added 🟦 blueprint The data that defines our UI exclude from changelog PRs with this won't show up in CHANGELOG.md labels Mar 8, 2024
@jleibs jleibs marked this pull request as ready for review March 8, 2024 21:42
@@ -218,6 +218,9 @@ def log_components(
if None, use the global default from `rerun.strict_mode()`

"""
# Convert to a native recording
recording = RecordingStream.to_native(recording)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was just an error before on this API. Apparently we don't use stream-overrides in any of our other examples.

@Wumpf Wumpf self-requested a review March 11, 2024 12:56
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

Both Container and SpaceView are veeery close to just being extension methods to ContainerBlueprint and SpaceViewBlueprint. I think that would be still worth considering as it means we have less types around and things will look less magic overall, making it a little bit more transparent what's going on - it's arguably both an advantage and disadvantage that this would expose the archetype's properties directly.

The derived Spatial2D/3D/Vertical/Horizontal etc. classes can still work the exact same way then I reckon

Love how easy and small api.py is. I'd recommend getting this in with above higher level comment(s) unresolved so we can all get our hands at the actual API asap :)

rerun_py/rerun_sdk/rerun/blueprint/api.py Outdated Show resolved Hide resolved
Comment on lines 68 to 80
# Handle the cases for SpaceViewContentsLike
if isinstance(self.contents, str):
# str
contents = SpaceViewContents(query=self.contents)
elif isinstance(self.contents, Sequence) and len(self.contents) > 0 and isinstance(self.contents[0], str):
# list[str]
contents = SpaceViewContents(query="\n".join(self.contents))
elif isinstance(self.contents, SpaceViewContents):
# SpaceViewContents
contents = self.contents
else:
# Anything else we let SpaceViewContents handle
contents = SpaceViewContents(query=self.contents) # 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.

it would be nicer if these were handled by SpaceViewContents in such a way that passing any of them to SpaceViewContents gives you a new usable SpaceViewContents
That way we can also unit test that separately

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Uggh, this is a bit annoying. Leaving a TODO.

rerun_py/rerun_sdk/rerun/blueprint/api.py Show resolved Hide resolved
rerun_py/rerun_sdk/rerun/blueprint/api.py Outdated Show resolved Hide resolved
Copy link

Size changes

Name main 5408/merge Change
incremental_logging.rrd 0 MiB 0 MiB +100.00%

@jleibs jleibs merged commit 6275a33 into main Mar 12, 2024
39 checks passed
@jleibs jleibs deleted the jleibs/blueprint_test branch March 12, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🟦 blueprint The data that defines our UI exclude from changelog PRs with this won't show up in CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants