-
Notifications
You must be signed in to change notification settings - Fork 113
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 unserializable parameters value #2122
Conversation
Add a note here: We discussed in private that this is not a new bug, but never the bug never surface before. We fixed an issue previously for non-primitive type, but this one is related to non-serialisation due to customer resolver, lambda etc |
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.
The fix looks good to me. Can you post a screenshot how does it looks like for the final result with this change? We should include this in the docs to inform user in case this happens.
Awesome, i also tried this with #1712 and |
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.
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.
Awesome, thank you @ravi-kumar-pilla!
Yes, I was trying to avoid this and experimented different ways (like trying to iterate through the dict and only converting the dict value which is an object). So what I realized is, we can -
Though this is not ideal, I could not think of any better way to handle all possible cases. @astrojuanlu could you please share any info on how to use Thank you |
For the example where parameters file is - split_options:
test_size: 0.2
random_state: 3
target: 'price'
data_info: "${polars:Float64}" Custom Resolver as below -
UI looks like - Regarding the docs, we don't have much info about the metadata panel apart from stats. I can add a section about the parameter value here - https://docs.kedro.org/projects/kedro-viz/en/stable/kedro-viz_visualisation.html . What do you think ? @noklam Thank you |
In this case I think fastapi make this harder to understand as it creates additional key that doesn't exist. What if we don't use fastapi at all? Can you also give an example that using fastapi actually make this better? |
I would not say it makes better but it will handle all edge cases that we might miss. But we can also have a custom serializer function something like below which will not return any unexpected value. I was not sure if this handles all the cases. def serialize_value(value: Any) -> Any:
"""Custom serialization function to handle different types of values,
including primitives, lists, dictionaries, sets, tuples, and objects."""
# Recursively serialize dictionaries
if isinstance(value, dict):
return {k: serialize_value(v) for k, v in value.items()}
# Recursively serialize lists and sets
elif isinstance(value, (list, set)):
return [serialize_value(v) for v in value]
# Recursively serialize tuples
elif isinstance(value, tuple):
return tuple(serialize_value(v) for v in value)
# Handle primitives
elif isinstance(value, (int, float, str, bool, type(None))):
return value
# Handle bytes
elif isinstance(value, (bytes, bytearray)):
# Convert to string
return value.decode()
elif hasattr(
value, "__dict__"
): # Handle custom objects by serializing their __dict__
return serialize_value(value.__dict__)
# Fallback: convert unknown types to a string
return str(value) |
I see, what if we just naively use
Do I understand this correctly, originally we only have this part and this will pass to a pydantic model and eventually go through the fastapi json encoder? |
We can do that as well, but I thought since we have jsonable_encoder doing a better job than naively returning a string, opted for it. But may be this is creating a bit of confusion. The basic serialization function also seems to handle most of the cases. Do you think we can go with the function I commented ?
yes that is correct. So for example we have the below Response model and we allow parameters value to be Any. In case of objects, this throws error. class TaskNodeAPIResponse(BaseGraphNodeAPIResponse):
parameters: Dict
... |
Description
Resolves #2010
Development notes
QA notes
Checklist
RELEASE.md
file