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.

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: ChatPromptBuilder from_dict if template is None #8165

Merged

Conversation

faymarie
Copy link
Contributor

@faymarie faymarie commented Aug 6, 2024

Related Issues

When initialisizing ChatPromptBuilder component from_dict with value {"template": None},
we receive 'NoneType' object is not iterable.

Proposed Changes:

Fix from_dict method.

How did you test it?

Unit test

Notes for the reviewer

Checklist

@faymarie faymarie changed the title fix ChatPromptBuilder from dict if template=None fix: ChatPromptBuilder from dict if template=None Aug 6, 2024
@coveralls
Copy link
Collaborator

coveralls commented Aug 6, 2024

Pull Request Test Coverage Report for Build 10265959872

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 90.143%

Totals Coverage Status
Change from base Build 10247553761: 0.001%
Covered Lines: 6914
Relevant Lines: 7670

💛 - Coveralls

@faymarie faymarie marked this pull request as ready for review August 6, 2024 10:33
@faymarie faymarie requested review from a team as code owners August 6, 2024 10:33
@faymarie faymarie requested review from dfokina and julian-risch and removed request for a team August 6, 2024 10:33
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

Hi @faymarie thanks for bringing this issue to our attention and for opening a pull request at the same time! 👍
I suggest that we keep template set to None instead of replacing it with an empty list if the dict contains None. Something like:

        template = init_parameters.get("template")
        if template:
            init_parameters["template"] = [ChatMessage.from_dict(d) for d in template]
        else:
            init_parameters["template"] = None

or actually the following should be sufficient

        template = init_parameters.get("template")
        if template:
            init_parameters["template"] = [ChatMessage.from_dict(d) for d in template]

In that case the test case would also need to be adjusted to check for assert component.template == None
What do you think? If we can merge the PR today, it will make it into the 2.4.0 release.

@faymarie
Copy link
Contributor Author

faymarie commented Aug 6, 2024

        template = init_parameters.get("template")
        if template:
            init_parameters["template"] = [ChatMessage.from_dict(d) for d in template]

You are right. Better to keep it None! Pushed the change. We would be happy to have it in the upcoming release already! @julian-risch

@julian-risch julian-risch changed the title fix: ChatPromptBuilder from dict if template=None fix: ChatPromptBuilder from_dict if template is None Aug 6, 2024
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@julian-risch julian-risch merged commit 031b0bf into main Aug 6, 2024
17 checks passed
@julian-risch julian-risch deleted the fix/ChatPromptBuild-from-dict-with-empty-template branch August 6, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants