chore: Remove migrated HiTL code #431
Conversation
Pull Request Test Coverage Report for Build 21397277559Details
💛 - Coveralls |
julian-risch
left a comment
julian-risch
left a comment
There was a problem hiding this comment.
Looks good to me! 👍
Some of the changes didn't seem directly related to migrating HiTL code but were smaller improvements. Generally nice to have those. Reviewing would have been a bit easier and faster if they were separated. I give three examples below to better explain what I mean, although I think you are already aware. 🙂
| ) | ||
| @pytest.mark.integration | ||
| def test_delete_memory(self, sample_messages): | ||
| def test_delete_memory(self, sample_messages, memory_store): |
There was a problem hiding this comment.
Code changes here don't seem directly related to HiTL code and it would be better to have them in a separate PR next time.
There was a problem hiding this comment.
I brought them in here since there were causing the integration tests to be flaky and fail sometimes in this PR. But I can make it a separate PR next time.
| init_params = data.get("init_parameters", {}) | ||
|
|
||
| deserialize_chatgenerator_inplace(init_params, key="chat_generator") | ||
| deserialize_component_inplace(init_params, key="chat_generator") |
There was a problem hiding this comment.
deserialize_chatgenerator_inplace directly calls deserialize_component_inplace internally so there is no change in functionality at all. However, it's a slight improvement and I understand that it is done in line with the changes in https://github.com/deepset-ai/haystack/pull/10403/changes
| retrieved_memories = self._memory_store.search_memories(query=messages[-1].text, **memory_store_kwargs) # type: ignore[arg-type] | ||
|
|
||
| retrieved_memories = self._memory_store.search_memories( | ||
| query=messages[-1].text, **memory_store_kwargs if memory_store_kwargs else {} |
There was a problem hiding this comment.
**memory_store_kwargs if memory_store_kwargs else {} instead of **memory_store_kwargs prevents a TypeError: search_memories() argument after ** must be a mapping, not NoneType. 👍
Related Issues
Proposed Changes:
Remove the HiTL code that was migrated to Haystack main.
How did you test it?
Remaining tests
Notes for the reviewer
Should not be merged until Haystack 2.23 release
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.