docs: detailing in docstrings the scope and context of each serialisation function#11903
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
anakin87
left a comment
anakin87
left a comment
There was a problem hiding this comment.
I left some questions and comments
| Convert an object to a serializable dict recursively. | ||
|
|
||
| Used for OpenAI SDK response objects, so it skips any attribute starting with "_" (SDK-internal | ||
| fields). `base_serialization`'s serializer doesn't skip those, so don't swap this out for it. |
There was a problem hiding this comment.
base_serialization's serializer
Which function in this file? Let's specify this.
I am also realizing that serialize_class_instance in base_serialization is not used. Right?
| """ | ||
| Convert an object to a serializable dict recursively. | ||
|
|
||
| Used for OpenAI SDK response objects, so it skips any attribute starting with "_" (SDK-internal |
There was a problem hiding this comment.
If I am not wrong it's only used for logprobs serialization. Maybe it's worth specifying this. WDYT?
|
|
||
| This is the format used for saved pipeline files (`Pipeline.dump`/`Pipeline.load`). Don't merge | ||
| it with `base_serialization`'s serializer — that one uses a different envelope for a different | ||
| job (arbitrary runtime values, not Components) and changing either would break saved files. |
There was a problem hiding this comment.
Which function in base_serialization?
|
|
||
| Output looks like `{"text": "hello"}` or `{"image": {...}}` — short, hand-writable keys, not a | ||
| full class name. People write these by hand (e.g. in chat templates), so keep it this way. | ||
|
|
There was a problem hiding this comment.
This docstring addition does not seem particularly helpful. I'd also delete it, because it already seems clear that this function is very specific to the ChatMessage content part.
| This is mostly necessary for components but can be used by any object. Reverses the | ||
| `{"type": ..., "init_parameters": ...}` envelope produced by `default_to_dict` — see that | ||
| function's docstring for why this envelope is not interchangeable with | ||
| `haystack.utils.base_serialization`'s. |
There was a problem hiding this comment.
Which function in base_serialization?
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
anakin87
left a comment
anakin87
left a comment
There was a problem hiding this comment.
Looks good.
Keep in mind to remove unused functions from base_serialization (in another PR).
davidsbatista
deleted the
docs/clarify-serialization-envelope-boundaries-v3
branch
2 participants
Related Issues
Proposed Changes:
Docstring-only additions, no behavior change. Added short "why" notes to 5 functions that looked like they could be merged or simplified, but can't:
default_to_dict/default_from_dict(core/serialization.py) — notes this is the on-disk pipeline YAML format, incompatible withbase_serialization's._serialize_value_with_schema(utils/base_serialization.py) — notes it's for runtime values, not Components, and points back to the note above._serializable_value(tracing/utils.py) — notes it's deliberately one-way/lossy, unlike the schema serializer._serialize_content_part(dataclasses/chat_message.py) — notes the short tags are a deliberate hand-writable public format._serialize_object(components/generators/utils.py) — notes it skips private attributes for OpenAI SDK objects, unlike the generic serializer.These came out of an investigation into duplicated/scattered serialization code (see related PRs). Most of what looked like duplication turned out to be intentional separation — these notes capture the reasoning directly in the code so it doesn't get
"helpfully" merged later.
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.