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

fix(weave): Use sdk-local deserializer instead of saved deserializer for known types like Images #2696

Merged
merged 13 commits into from
Nov 8, 2024

Conversation

andrewtruong
Copy link
Collaborator

@andrewtruong andrewtruong commented Oct 15, 2024

Description

https://wandb.atlassian.net/browse/WB-21517

  • Prefer using the SDK's local registered deserializer instead of the saved deserializer for known types like Images

Testing

Unit tests and manual testing

@circle-job-mirror
Copy link

circle-job-mirror bot commented Oct 15, 2024

@andrewtruong andrewtruong changed the title chore(weave): Use lib-local deserializer for known types like Images chore(weave): Use sdk-local deserializer for known types like Images Oct 15, 2024
@andrewtruong andrewtruong changed the title chore(weave): Use sdk-local deserializer for known types like Images chore(weave): Use sdk-local deserializer instead of saved deserializer for known types like Images Oct 15, 2024
@andrewtruong andrewtruong marked this pull request as ready for review October 15, 2024 03:49
@andrewtruong andrewtruong requested a review from a team as a code owner October 15, 2024 03:49
@andrewtruong andrewtruong changed the title chore(weave): Use sdk-local deserializer instead of saved deserializer for known types like Images fix(weave): Use sdk-local deserializer instead of saved deserializer for known types like Images Oct 30, 2024
load_instance_op = None
if load_instance_op_uri is not None:
if weave_type["type"] in KNOWN_TYPES:
serializer = get_serializer_by_id(weave_type["type"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if the serialization changes? It seems like we should first "try" the local version, then fallback to the remote version, then error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok yeah that's safer

found_serializer = False

# First, try to load the object using a known serializer
if _type in KNOWN_TYPES:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is still not quite right - we need to try with the local one first, else fallback to the the remote one

@andrewtruong andrewtruong merged commit e9c0ee4 into master Nov 8, 2024
115 checks passed
@andrewtruong andrewtruong deleted the andrew/known-serde branch November 8, 2024 03:00
@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants