Refactor Intro #20
Refactor Intro #20
Conversation
There was a problem hiding this comment.
Good stuff @erikgaas ! Some minor suggestions inline.
The pattern of creating a function multiple times and gradually improving it actually turned out not to work well at all. So we're trying to get rid of it! It results in refactorings requiring replacing every version of the function, and it makes understanding the code more complicated. So try to redo your notebook without using this pattern. Happy to chat about it.
| def _details(resp): | ||
| ps = [f"id: `{resp['id']}`", f"model: `{resp['model']}`",f"finish_reason: `{_f_reason(resp)}`"] | ||
| if hasattr(resp, 'usage') and resp['usage']: ps.append(f"usage: `{resp['usage']}`") | ||
| return f'<details>\n\n{'\n- '.join(ps)}\n\n</details>' | ||
|
|
||
| @patch(as_prop=True) | ||
| def details(self:litellm.ModelResponse): return _details(self) |
There was a problem hiding this comment.
No need for two functions when 1 will do.
| def _details(resp): | |
| ps = [f"id: `{resp['id']}`", f"model: `{resp['model']}`",f"finish_reason: `{_f_reason(resp)}`"] | |
| if hasattr(resp, 'usage') and resp['usage']: ps.append(f"usage: `{resp['usage']}`") | |
| return f'<details>\n\n{'\n- '.join(ps)}\n\n</details>' | |
| @patch(as_prop=True) | |
| def details(self:litellm.ModelResponse): return _details(self) | |
| @patch(as_prop=True) | |
| def details(self:litellm.ModelResponse): | |
| ps = [f"id: `{resp['id']}`", f"model: `{resp['model']}`",f"finish_reason: `{_f_reason(resp)}`"] | |
| if hasattr(resp, 'usage') and resp['usage']: ps.append(f"usage: `{resp['usage']}`") | |
| return f'<details>\n\n{'\n- '.join(ps)}\n\n</details>' |
|
|
||
| - {det_str} | ||
| @patch(as_prop=True) | ||
| def content(self:litellm.ModelResponse): return _content(self) |
There was a problem hiding this comment.
Just one function needed here too.
| return {'index': idx, 'function': {'arguments': args, 'name': func}, 'id': id, 'type': 'function'} | ||
|
|
||
| # %% ../nbs/00_core.ipynb | ||
| def _wrap_choice(msg, finish_reason='stop', index=0): |
There was a problem hiding this comment.
This is only used once and is very short, so may as well inline it
| return "\n".join([f"\n\n🔧 {tc['function']['name']}({tc['function']['arguments']})\n" for tc in tool_calls]) | ||
|
|
||
| @patch(as_prop=True) | ||
| def show_tools(self:litellm.ModelResponse): return _show_tools(self) |
There was a problem hiding this comment.
No need for 2 functions here
|
@erikgaas lemme know when this is ready for re-review. Can you take a look at the conflicts too when you have a chance? |
I was going from top to bottom of the lisette core nb and while I was learning I did some refactoring and tried to add complete documentation to explain everything as I was going along.
Pros: The intro is more complete and incorporates some of the synthetic data creation from before. I moved most of that to the top fo the notebook using completion instead of Chat.
Cons: It is longer, there is more non exported code. It follows more of the pattern of building up some functions and then finally exporting them later when they are complete.