Mirascope / mirascope

LLM abstractions that aren't obstructions
http://mirascope.com/
MIT License
757 stars 50 forks source link

docs: improve types in blog writing agent notebook #626

Closed jd-solanki closed 3 weeks ago

jd-solanki commented 4 weeks ago

Hi πŸ‘‹πŸ»

I was playing with writing agent notebook after my issue #625 and realized there are some type issues: image

Type of "append" is partially unknown
Argument type is "list[Unknown]"

This is because we don't explicitly provides type for list item of tools_and_outputs.

I've made fix that explicitly adds the type for tools_and_outputs.


I follow strict typing in all of my projects to get type hints everywhere, I recommend we add following in vscode settings.json

"python.analysis.typeCheckingMode": "strict",

This is because even with 30 sec tutorial we're not 100% type safe: image

Thanks ❀️

willbakst commented 4 weeks ago

Hmm, we've put a lot of effort into type safety, and we ensure every PR that gets merged passes Pyright, so I'm not sure what is going on here, particularly for the call decorator.

What type checker are you using here?

willbakst commented 4 weeks ago

Looks like there is an issue from a previous PR. We are going to work to resolve those type errors that should never have been the case. Thanks for pointing this out!

codecov[bot] commented 4 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (d19a791) to head (3d3b19f).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #626 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 352 352 Lines 10459 10459 ========================================= Hits 10459 10459 ``` | [Flag](https://app.codecov.io/gh/Mirascope/mirascope/pull/626/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Mirascope) | Coverage Ξ” | | |---|---|---| | [tests](https://app.codecov.io/gh/Mirascope/mirascope/pull/626/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Mirascope) | `100.00% <ΓΈ> (ΓΈ)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Mirascope#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jd-solanki commented 4 weeks ago

What type checker are you using here?

I'm using VSCode + Pylance + mypy

If you create new project & use vscode + pylance + mypy you'll get the same error with 30 sec tutorial.

Most of the time these errors are from missed types of list item, forgot to pass down generic, etc.

Moreover, as you can see in my first screenshot, implicit type of tools_and_outputs is list[Any] which is not ideal and technically we should write type for list item as well.

This is my first PR and if you want I'll submit more PRs regarding types as I get aware of it. I really love the simplicity of mirascope and will be using from now on.

Awaiting your response.

willbakst commented 4 weeks ago

So for maintainability we've created a call_factory method for creating the call decorator for each provider. Originally we were relying heavily on nested typing overloads to dynamically generate the type based on variable types, but apparently the new version of Pyright (which Pylance uses) no longer supports this (or I guess considered support a bug and removed support 🀦 )

We've implemented a fix to use protocols instead so that the correct typing returns in this newer version. Once we merge in that fix and release it, I would love to see if you're still having the same typing issues here. I'm hopeful that this fix will return the proper type hints we expect in the examples.

As for PRs / contributions, they are always welcome with open arms! This particular PR may no longer be necessary with the fix I mentioned, but definitely keep the PRs coming!

willbakst commented 4 weeks ago

We've also labeled a bunch of issues with the "good first issue" and "hacktoberfest" labels, so if you're looking for a well-defined PR to work on, those are a great place to start!

We're available to answer any questions you may have along the way :)

jd-solanki commented 4 weeks ago

This particular PR may no longer be necessary with the fix I mentioned

Actually this PR adds types to the example which I guess will be still required after you merge fix for .call(). But if you feel that this is not necessary or impactful change, feel free to close the PR πŸ˜‡

Finally, Can you let me know where should I track the fix you mentioned so I can check it once landed in official release.

Thanks.

willbakst commented 4 weeks ago

This PR contains the fix, which I just released in v1.5

As you mentioned, while this fixes the issue with the call decorator, it still doesn't change anything on the tools_and_outputs list (since the type there is inferred and not explicit).

If we're going to add this additional typing, we should add it for every single example, not just this tutorial. Namely, all of the files in examples/learn/agents/ would need updating as well as the files in examples/learn/tools/tool_message_params, as well as any of the other agent tutorials that use the tools_and_outputs + tool_message_params method combo

jd-solanki commented 4 weeks ago

we should add it for every single example

I left this decision up to you. If you want I can update this PR because it's simple but lengthy.

I tried v1.5.0 and I'm no longer getting error on .call() however while exploring more I found missing type arg for generic error on tool. I guess we should open seperate issue for dicussing this type issues instead of in this PR. WDYS? image

Awaiting your response πŸ˜‡

willbakst commented 4 weeks ago

Glad the call decorator type errors are fixed! PR for updating these examples with improved types on tools_and_outputs are welcome if you believe they will add value. One reason we've left some type hints out is for brevity/readability of examples, but you're welcome to add more clarity here!

As for the BaseTool typing, this is an interesting point. As a user, you'll never actually use or instantiate a BaseTool instance because it's really just for defining the tool. We've included the generic for the tool schema type for each specific provider (e.g. OpenAITool), which is what will get returned when making a call with a tool definition.

You could use Any as the type in your definition (e.g. class GetBookAuthor(BaseTool[Any])) but using Any is generally bad practice.

One reason we haven't marked the tool_schema method as abstract is that the user should not need to define it. Curious for your thoughts here. Feel free to open a separate issue for us to discuss this separately from this PR as you've mentioned.

jd-solanki commented 3 weeks ago

Hi,

Thanks for your patience. I reflected on your thoughts and realized I should first understand the codebase and then make large PRs.

For the rest of the comments I'll open new issue if anything found.

Thanks, Closing this for now.