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.

Sign up for GitHub

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

Jump to bottom

bug: Missing params in OpenAIGenerator's to_dict method #7903

Closed
sjrl opened this issue Jun 20, 2024 · 0 comments · Fixed by #7929
Closed

bug: Missing params in OpenAIGenerator's to_dict method #7903

sjrl opened this issue Jun 20, 2024 · 0 comments · Fixed by #7929
Assignees
@Amnah199
Labels
P1 High priority, add to the next sprint type:bug Something isn't working

Comments

@sjrl
Copy link
Contributor

sjrl commented Jun 20, 2024

Describe the bug
I noticed that the to_dict method of OpenAIGenerator

haystack/haystack/components/generators/openai.py

Lines 144 to 152 in 4c59000

return default_to_dict(
self,
model=self.model,
streaming_callback=callback_name,
api_base_url=self.api_base_url,
generation_kwargs=self.generation_kwargs,
system_prompt=self.system_prompt,
api_key=self.api_key.to_dict(),
)

looks to be missing

haystack/haystack/components/generators/openai.py

Line 61 in 4c59000

organization: Optional[str] = None,

and

haystack/haystack/components/generators/openai.py

Lines 64 to 65 in 4c59000

timeout: Optional[float] = None,
max_retries: Optional[int] = None,

from it's init method.

I can understand that max_retries and timeout might be better left as None so environment variables are always used, but I think organization was missed.

Expected behavior
For serialization and deser to return the component with the same settings.

To Reproduce
Initialize a OpenAIGenerator and run serder and see that the components are not equal.
@anakin87 anakin87 added the type:bug Something isn't working label Jun 21, 2024
@mrm1001 mrm1001 added the P1 High priority, add to the next sprint label Jun 21, 2024
@Amnah199 Amnah199 mentioned this issue Jun 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Amnah199 Amnah199

Labels
P1 High priority, add to the next sprint type:bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add the missing parameter for serialization deepset-ai/haystack
4 participants
@mrm1001 @sjrl @Amnah199 @anakin87