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

from guidance import guidance #759

Merged
merged 16 commits into from
Apr 17, 2024

Conversation

ryanpeach
Copy link
Contributor

@ryanpeach ryanpeach commented Apr 13, 2024

This is a backwards compatible implementation of from guidance import guidance that fixes the need for ignoring operator errors in mypy (which handle + - typing but also apparently can not call module errors) due to "can not call module" errors in static typing.

I do this by moving a lot of the code in __init__.py to a new _guidance.py and creating a duplicate object to Guidance from __init__.py called guidance in _guidance.py, which can then be relatively imported from within the package.

Outside the package, you can use either one. This also prevents some circular dependency errors from within the code that emerge when you import your own __init__.py.

@@ -26,10 +26,6 @@ no_implicit_optional = false # TODO: PEP484, set to true
strict = false # TODO: set to true
exclude = ["tests"]

[[tool.mypy.overrides]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the most relevant line, the one we are trying to delete to make mypy more comprehensive.

@@ -1,4 +1,4 @@
import guidance
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching to relative imports

@@ -0,0 +1,78 @@
import functools
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new file, but almost all this code is copy-paste from init.py


newline = "\n"

def guidance(f=None, *, stateless=False, cache=None, dedent=True, model=models.Model):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the new guidance function

@@ -863,7 +863,7 @@ def bos_token() -> ModelVariable:
# if len(low_bytes) > 1 or len(high_bytes) > 1:
# raise Exception("We don't yet support multi-byte character ranges!")
# return ByteRange(low_bytes + high_bytes)
def str_to_grammar(value: str) -> Union[str, bytes, Null, Byte, Join, Function]:
def str_to_grammar(value: str):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was wrong in the last PR. I have no idea what the types for this function should be.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Figuring that out has been on my ToDo list for a while. Similarly, I suspect that a few of the arguments to various functions only reflect a subset of the actual accepted types.

# This makes the guidance module callable
class Guidance(types.ModuleType):
class _Guidance(types.ModuleType):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this private so people don't from guidance import Guidance

@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2024

Codecov Report

Attention: Patch coverage is 98.46154% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 65.60%. Comparing base (5c9776e) to head (23e977a).

Files Patch % Lines
guidance/_guidance.py 97.36% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #759      +/-   ##
==========================================
- Coverage   69.53%   65.60%   -3.94%     
==========================================
  Files          54       55       +1     
  Lines        4038     4044       +6     
==========================================
- Hits         2808     2653     -155     
- Misses       1230     1391     +161     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Harsha-Nori
Copy link
Collaborator

Harsha-Nori commented Apr 17, 2024

Super cool that you were able to make this backwards compatible. I find the relative imports a bit more cumbersome -- it's always a pain remembering how many layers into the hierarchy you are... -- but I think the issue this solves around circular dependencies is worth the trouble.

Curious what @riedgar-ms and @slundberg think but from my POV, this LGTM

Copy link
Collaborator

@riedgar-ms riedgar-ms left a comment

Choose a reason for hiding this comment

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

Fine with me. Since we're practically creating our own DSL within Python with guidance some unpleasantness within the code is to be expected.

@@ -863,7 +863,7 @@ def bos_token() -> ModelVariable:
# if len(low_bytes) > 1 or len(high_bytes) > 1:
# raise Exception("We don't yet support multi-byte character ranges!")
# return ByteRange(low_bytes + high_bytes)
def str_to_grammar(value: str) -> Union[str, bytes, Null, Byte, Join, Function]:
def str_to_grammar(value: str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Figuring that out has been on my ToDo list for a while. Similarly, I suspect that a few of the arguments to various functions only reflect a subset of the actual accepted types.

@Harsha-Nori Harsha-Nori merged commit 4b45025 into guidance-ai:main Apr 17, 2024
81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants