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

Add serializers for different var types #1816

Merged
merged 32 commits into from
Sep 16, 2023
Merged

Add serializers for different var types #1816

merged 32 commits into from
Sep 16, 2023

Conversation

picklelo
Copy link
Contributor

Factored out var serialization so that it's not hardcoded within the compiler. The user can now define a serializer using the @rx.serializer decorator.

Added serializers for Plotly, dict, and date time.

@masenf masenf mentioned this pull request Sep 14, 2023
8 tasks
@@ -25,7 +25,7 @@ class Plotly(PlotlyLib):
is_default = True

# The figure to display. This can be a plotly figure or a plotly data json.
data: Var[Figure]
data: Var[Any]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
data: Var[Any]
data: Var["Figure"]

And then at the top of the file, do something like

try:
    from plotly.graph_objects import Figure
except ImportError:
    Figure = Any

That way the type checking at least works nicer when you do have plotly installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea

Comment on lines 66 to 73
try:
from PIL.Image import Image as Img

# If the src is an image, convert it to a base64 string.
if types._isinstance(self.src, Img):
self.src = Var.create(serialize(self.src)) # type: ignore
except ImportError:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the import guard here? Doesn't seem like this block is actually using Img

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use it in the isinstance check

Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess i was thinking we can just call self.src = Var.create(serialize(self.src)) and not care about what self.src is... looks like serialize has a default str handler.

registered_fn = SERIALIZERS.get(type_)
if registered_fn is not None and registered_fn != fn:
raise ValueError(
f"Serializer for type {type_} is already registered as {registered_fn}."
Copy link
Collaborator

Choose a reason for hiding this comment

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

registered_fn.__qualname__ might be useful here, in case the name is ambiguous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Returns:
The serialized value, or None if a serializer is not found.
"""
global SERIALIZERS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
global SERIALIZERS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed



@serializer
def serialize_dict(prop: Dict[str, Any]) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

can the existing format.format_dict function just be decorated as @serializer and then moved into this file later? the duplication of such complex functionality makes me nervous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the original function

@picklelo picklelo force-pushed the nikhil/serialization branch from 2539631 to eff6dfa Compare September 15, 2023 20:28
masenf
masenf previously approved these changes Sep 15, 2023
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

killer!

Comment on lines 66 to 73
try:
from PIL.Image import Image as Img

# If the src is an image, convert it to a base64 string.
if types._isinstance(self.src, Img):
self.src = Var.create(serialize(self.src)) # type: ignore
except ImportError:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess i was thinking we can just call self.src = Var.create(serialize(self.src)) and not care about what self.src is... looks like serialize has a default str handler.

tests/utils/test_serializers.py Outdated Show resolved Hide resolved
@@ -39,14 +43,22 @@ class Plotly(PlotlyLib):
# If true, the graph will resize when the window is resized.
use_resize_handler: Var[bool]

def _render(self) -> Tag:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this wasn't actually doing anything... We can reinvestigate how to add this logic back in.

def _render(self) -> Tag:
self.src.is_string = True

# Render the table.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Render the table.
# Render the image.

😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix this in the follow up

@picklelo picklelo merged commit 1938a6c into main Sep 16, 2023
35 checks passed
@picklelo picklelo deleted the nikhil/serialization branch September 16, 2023 00:19
Alek99 pushed a commit that referenced this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants