Skip to content

fix: chroma destination connector serialization#2425

Merged
ahmetmeleq merged 4 commits intomainfrom
ahmet/fix-chroma-serialization
Jan 19, 2024
Merged

fix: chroma destination connector serialization#2425
ahmetmeleq merged 4 commits intomainfrom
ahmet/fix-chroma-serialization

Conversation

@ahmetmeleq
Copy link
Copy Markdown
Contributor

This fixes the serialization of the ChromaDB destination connector. Presence of the _collection object breaks serialization due to TypeError: cannot pickle 'module' object. This removes that object before serialization.

def to_dict(self, **kwargs):
"""
The _collection variable in this dataclass breaks deepcopy due to:
TypeError: cannot pickle '_thread.lock' object
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This comment looks copy pasted from the Postgres. Is it correct?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is false - fixing now

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed with fix docstring, had missed those two lines

"""
self_cp = copy.copy(self)
if hasattr(self_cp, "_collection"):
setattr(self_cp, "_collection", None)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would it be safer to delattr? Though now that I think about it probably not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since we have it in dataclass initialization as a None value, is setattr to None better?

@dataclass
class ChromaDestinationConnector(BaseDestinationConnector):
write_config: ChromaWriteConfig
connector_config: SimpleChromaConfig
_collection: t.Optional["ChromaCollection"] = None

Copy link
Copy Markdown

@jasonbot jasonbot left a comment

Choose a reason for hiding this comment

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

Looks fine as long as you’ve validated it. Test would be nice.

Copy link
Copy Markdown
Contributor

@potter-potter potter-potter left a comment

Choose a reason for hiding this comment

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

Looks good. Basically the same code we've had to add to most of the dest connectors.

@ahmetmeleq ahmetmeleq enabled auto-merge January 18, 2024 21:20
@ahmetmeleq ahmetmeleq added this pull request to the merge queue Jan 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jan 19, 2024
Merged via the queue into main with commit 1a30586 Jan 19, 2024
@ahmetmeleq ahmetmeleq deleted the ahmet/fix-chroma-serialization branch January 19, 2024 03:24
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.

3 participants