feat: update ContentComposer to work well with json #42
Conversation
| @@ -37,7 +37,7 @@ def compose(self, prompt: str) -> Dict: | |||
| # prompts: ['["John",40]'] | |||
| # result: '{"data":["John",40],"names":["Name","Age"]}' | |||
| try: | |||
| return json.loads(Composer.compose(self, prompt)) | |||
| return json.loads(self.vanilla_template.substitute(**{self.keyword: json.dumps(prompt)})) | |||
There was a problem hiding this comment.
can we do Composer.compose(self, json.dumps(prompt)) instead? that keeps the substitution logic at one place.
There was a problem hiding this comment.
Please add a comment too, with example, or update example in docstring, it's not that intuitive looking at the code.
There was a problem hiding this comment.
Prompt template does not require this update - prompt doesn't have to escape. Only content composer has to escape. I will add some comments.
There was a problem hiding this comment.
i did not understand your point. Below is what happens in Composer.compose:
self.vanilla_template.substitute(**{self.keyword: prompt})
Earlier code in JSONContentComposer:
json.loads(Composer.compose(self, prompt))
Your code is basically just using json.dumps(prompt) instead of prompt, which can be solved by below instead:
return json.loads(Composer.compose(self, json.dumps(prompt))) in compose method here. Why not do it this way? Keeps the core substitution logic common. Am i missing anything here?
There was a problem hiding this comment.
Can you also add a unit test of the specific case that was failing earlier? i don't see any update in UT suite.
There was a problem hiding this comment.
Okay updated. Unit tests were updated but probably it's not clear because python allow use of single quotes too. Maybe now it is more clear.
| @@ -37,7 +37,7 @@ def compose(self, prompt: str) -> Dict: | |||
| # prompts: ['["John",40]'] | |||
| # result: '{"data":["John",40],"names":["Name","Age"]}' | |||
| try: | |||
| return json.loads(Composer.compose(self, prompt)) | |||
| return json.loads(self.vanilla_template.substitute(**{self.keyword: json.dumps(prompt)})) | |||
There was a problem hiding this comment.
Can you also add a unit test of the specific case that was failing earlier? i don't see any update in UT suite.
Description of changes:
update ContentComposer to work well with json
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.