First stab at minimal mypy implementation #738

ryanpeach · 2024-04-02T16:51:30Z

Set up the structure of mypy installs, now to start typing things
fixed guidance/library/_regex.py:13: SyntaxWarning: invalid escape sequence '['
First stab at minimal mypy implementation

This is a non-strict implementation of mypy, with cicd integrated.

After this is approved, we can either slowly move towards a strict implementation, or each PR can simply
choose how much they want/need to contribute to the typing system to help themselves understand the codebase.

ryanpeach · 2024-04-02T16:54:08Z

guidance/_grammar.py

@@ -739,7 +739,7 @@ def select(options: List[_T], name=None, list_append=False, recurse=False, skip_
            assert not isinstance(value, RawFunction), "You cannot select between stateful functions in the current guidance implementation!"
            assert not isinstance(value, types.FunctionType), "Did you pass a function without calling it to select? You need to pass the results of a called guidance function to select."
            if isinstance(value, int) or isinstance(value, float):
-                options[i] = str(value)
+                options[i] = str(value)  # type: ignore[assignment]


I didn't understand this, because options is type List[_T] but this is inserting a string.

I may have gotten the type wrong when I was trying to figure it out. Something like Union[List[_T], str] might be correct.

ryanpeach · 2024-04-02T16:54:46Z

guidance/_parser.py

@@ -380,7 +382,7 @@ def __repr__(self, state_sets=None) -> str:
                            rs += "•"
                else:
                    assert False
-                s += f"{rs:40} ({state.start}) {'nullable' if state.node.nullable else ''}\n"
+                s += f"{rs:40} ({state.start}) {'nullable' if state.node.nullable else ''}\n"  # type: ignore[attr-defined]


It thinks that node doesn't have a nullable. I couldn't find out what state was. Next person to touch this function should type it.

guidance/library/_block.py

ryanpeach · 2024-04-02T16:55:27Z

guidance/library/_regex.py

@@ -10,7 +10,7 @@
 @guidance(stateless=True, dedent=False)
 def regex(lm, pattern):
    # find all of the brackets we'll need to negate later
-    nots = re.findall('\[\^(.*?)\]', pattern)
+    nots = re.findall(r'\[\^(.*?)\]', pattern)


This was resulting in an actual error

ryanpeach · 2024-04-02T16:55:42Z

guidance/models/_model.py

@@ -1,17 +1,19 @@
 try:
    from IPython.display import clear_output, display, HTML
+    ipython_is_imported = True


This is a much cleaner way to do dynamic import.

ryanpeach · 2024-04-02T16:56:25Z

guidance/models/_model.py

+                clear_output(wait=True)
+                display(HTML(self._html()))
+            else:
+                pprint(self._state)


pprint is the best I could think of. Should fix #708

.github/workflows/ci_tests.yml

ryanpeach · 2024-04-03T02:27:42Z

E TypeError: 'module' object is not callable

I don't think that's me.

riedgar-ms · 2024-04-08T13:28:38Z

Thank you for starting on this. I've authorised this for running in GH Actions.

ryanpeach · 2024-04-09T15:36:08Z

@riedgar-ms can you understand why it's giving:

TypeError: 'module' object is not callable

mypy is passing and I didn't change anything like that

riedgar-ms · 2024-04-09T15:46:20Z

guidance/py.typed

What's the purpose of this file?

This indicates to people who are installing this library that it can be analyzed by mypy.

https://peps.python.org/pep-0561/

riedgar-ms · 2024-04-09T15:56:25Z

I'm not quite sure why that is happening. The magic that is supposed to make the module callable is:

guidance/guidance/__init__.py

Line 16 in 03d2b97

# This makes the guidance module callable

but you haven't edited that file.

Are you able to run the tests on your own machine?

ryanpeach · 2024-04-09T22:20:55Z

Do you have local testing instructions anywhere?

riedgar-ms · 2024-04-09T23:59:32Z

I'm not sure they're written out. You'll need the [test] extras packages, plus transformers. Then you should be able to run:

python -m pytest -m "not (needs_credentials or use_gpu or server)" tests/

ryanpeach · 2024-04-10T16:41:07Z

Ok so it does run on main and not in this branch, so it must be something in my pr. I'll try to fix it.

ryanpeach · 2024-04-10T17:12:36Z

@riedgar-ms ok fixed it. Importing from ..library._block import ContextBlock from _model creates a circular reference because __init__.py imports library. This is mostly fine except I think it makes the guidance class edits load later than the call in zero_or_more. Or something like that.

riedgar-ms · 2024-04-10T17:58:25Z

@riedgar-ms ok fixed it. Importing from ..library._block import ContextBlock from _model creates a circular reference because __init__.py imports library. This is mostly fine except I think it makes the guidance class edits load later than the call in zero_or_more. Or something like that.

I would not be averse to such a change (although perhaps not for this PR). Thoughts @Harsha-Nori ?

ryanpeach · 2024-04-10T18:02:14Z

@riedgar-ms ok fixed it. Importing from ..library._block import ContextBlock from _model creates a circular reference because __init__.py imports library. This is mostly fine except I think it makes the guidance class edits load later than the call in zero_or_more. Or something like that.

I would not be averse to such a change (although perhaps not for this PR). Thoughts @Harsha-Nori ?

Oh I edited the comment to not include the suggestion about the from guidance import guidance api change. But I've also already got a PR ready after this one for consideration to that end. It should be backwards compatible.

codecov-commenter · 2024-04-10T18:04:46Z

Codecov Report

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

Project coverage is 65.55%. Comparing base (c366498) to head (2106a87).

Files	Patch %	Lines
guidance/models/_model.py	68.18%	7 Missing ⚠️
guidance/_parser.py	0.00%	4 Missing ⚠️
guidance/_grammar.py	87.50%	1 Missing ⚠️
guidance/_utils.py	0.00%	1 Missing ⚠️
guidance/models/_azure_openai.py	50.00%	1 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #738      +/-   ##
==========================================
- Coverage   69.69%   65.55%   -4.15%     
==========================================
  Files          54       54              
  Lines        4029     4038       +9     
==========================================
- Hits         2808     2647     -161     
- Misses       1221     1391     +170

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

Harsha-Nori · 2024-04-10T18:33:43Z

@riedgar-ms ok fixed it. Importing from ..library._block import ContextBlock from _model creates a circular reference because __init__.py imports library. This is mostly fine except I think it makes the guidance class edits load later than the call in zero_or_more. Or something like that.

I would not be averse to such a change (although perhaps not for this PR). Thoughts @Harsha-Nori ?

Oh I edited the comment to not include the suggestion about the from guidance import guidance api change. But I've also already got a PR ready after this one for consideration to that end. It should be backwards compatible.

I generally like of how simple/clean "import guidance" being used for the decorator looks, but I appreciate that it causes some complexity in the developer onboarding here. I could definitely be persuaded against it but agree that we should discuss it in a separate issue/PR in more detail.

The mypy typing infrastructure looks great, and I appreciate the incremental approach you've taken here @ryanpeach! Happy to do a full review once the tests finish running (which it seems like they will). Thanks for the great contribution!

riedgar-ms

I'm OK with this.... @Harsha-Nori ?

.github/workflows/qa_tests.yml

riedgar-ms · 2024-04-11T12:57:39Z

.github/workflows/qa_tests.yml

+          python-version: 3.8
+      - name: Install mypy
+        run: pip install mypy
+      - uses: tsuyoshicho/action-mypy@v4


Should this be producing a comment on the PR? Or does that only happen if there's an error?

It should if there's an error, but I realize it's hard to test prior to release

ryanpeach · 2024-04-11T18:25:56Z

Anyway you can just run the qa_tests to see if my latest commit fixes the last commits problem?

ryanpeach · 2024-04-12T14:57:48Z

I assume the reason you manually run workflows is some of them cost money? We should make it so some workflows run on approval/workflow_dispatch and others run on push.

riedgar-ms · 2024-04-12T16:01:12Z

Cost is a reason; as is keeping secrets secret. There is some GH security setting we have on that means that external contributions need a member of the repo to approve before things run.

ryanpeach · 2024-04-12T16:12:23Z

Ok let me work on this at ryanpeach#1 since this custom action is behaving oddly

…quence '\['

…version will always be same as setup.py this way.

… --version before running its setup commands

ryanpeach · 2024-04-12T17:12:10Z

@riedgar-ms ok now it works in my PR to my own repo.

ryanpeach · 2024-04-12T17:20:05Z

Ready. Also rebased so that it should have a linear history.

ryanpeach · 2024-04-12T19:47:48Z

Passed, except one got canceled somehow, resulting in a red X.

guidance/_utils.py

guidance/models/_model.py

pyproject.toml

ryanpeach commented Apr 2, 2024

View reviewed changes

riedgar-ms requested review from riedgar-ms, paulbkoch and Harsha-Nori April 2, 2024 18:44

riedgar-ms reviewed Apr 2, 2024

View reviewed changes

.github/workflows/ci_tests.yml Outdated Show resolved Hide resolved

riedgar-ms reviewed Apr 9, 2024

View reviewed changes

riedgar-ms approved these changes Apr 11, 2024

View reviewed changes

riedgar-ms reviewed Apr 11, 2024

View reviewed changes

ryanpeach and others added 6 commits April 12, 2024 12:49

Set up the structure of mypy installs, now to start typing things

29a033f

fixed guidance/library/_regex.py:13: SyntaxWarning: invalid escape se…

fc2dc17

…quence '\['

First stab at minimal mypy implementation

be8bd18

The old version was not running on PR. Also this uses the action.

e9ef3f0

More actively maintained action. Test at lowest python version. Mypy …

cf93ef9

…version will always be same as setup.py this way.

Fixed circular reference and brought back dictionary

8a0c9f8

ryanpeach added 3 commits April 12, 2024 12:50

We dont need another pip install here

b398880

I think the last instance failed because the action tries to run mypy…

3fee464

… --version before running its setup commands

Simpler is better, just don't use an action

8c1a9e7

ryanpeach force-pushed the feature/mypy branch from cdbc54b to 8c1a9e7 Compare April 12, 2024 16:50

ryanpeach added 2 commits April 12, 2024 13:03

Name json_schema is not defined

32dc649

Library stubs not installed for 'jsonschema.validators'

8240901

ryanpeach force-pushed the feature/mypy branch from 44a12c3 to 8240901 Compare April 12, 2024 17:15

Fixed the typing of string

2106a87

Harsha-Nori reviewed Apr 12, 2024

View reviewed changes

guidance/_utils.py Show resolved Hide resolved

Harsha-Nori reviewed Apr 12, 2024

View reviewed changes

guidance/models/_model.py Outdated Show resolved Hide resolved

Harsha-Nori reviewed Apr 12, 2024

View reviewed changes

pyproject.toml Show resolved Hide resolved

Fixed whitespace

74240cf

Harsha-Nori merged commit 5c9776e into guidance-ai:main Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First stab at minimal mypy implementation #738

First stab at minimal mypy implementation #738

ryanpeach commented Apr 2, 2024 •

edited

ryanpeach Apr 2, 2024

riedgar-ms Apr 11, 2024

ryanpeach Apr 2, 2024

ryanpeach Apr 2, 2024

ryanpeach Apr 2, 2024

ryanpeach Apr 2, 2024

ryanpeach commented Apr 3, 2024

riedgar-ms commented Apr 8, 2024

ryanpeach commented Apr 9, 2024

riedgar-ms Apr 9, 2024

ryanpeach Apr 10, 2024

riedgar-ms commented Apr 9, 2024

ryanpeach commented Apr 9, 2024

riedgar-ms commented Apr 9, 2024

ryanpeach commented Apr 10, 2024

ryanpeach commented Apr 10, 2024 •

edited

riedgar-ms commented Apr 10, 2024

ryanpeach commented Apr 10, 2024

codecov-commenter commented Apr 10, 2024 •

edited

Harsha-Nori commented Apr 10, 2024

riedgar-ms left a comment

riedgar-ms Apr 11, 2024

ryanpeach Apr 11, 2024

ryanpeach commented Apr 11, 2024

ryanpeach commented Apr 12, 2024 •

edited

riedgar-ms commented Apr 12, 2024

ryanpeach commented Apr 12, 2024

ryanpeach commented Apr 12, 2024

ryanpeach commented Apr 12, 2024

ryanpeach commented Apr 12, 2024

First stab at minimal mypy implementation #738

First stab at minimal mypy implementation #738

Conversation

ryanpeach commented Apr 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanpeach commented Apr 3, 2024

riedgar-ms commented Apr 8, 2024

ryanpeach commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riedgar-ms commented Apr 9, 2024

ryanpeach commented Apr 9, 2024

riedgar-ms commented Apr 9, 2024

ryanpeach commented Apr 10, 2024

ryanpeach commented Apr 10, 2024 • edited

riedgar-ms commented Apr 10, 2024

ryanpeach commented Apr 10, 2024

codecov-commenter commented Apr 10, 2024 • edited

Codecov Report

Harsha-Nori commented Apr 10, 2024

riedgar-ms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanpeach commented Apr 11, 2024

ryanpeach commented Apr 12, 2024 • edited

riedgar-ms commented Apr 12, 2024

ryanpeach commented Apr 12, 2024

ryanpeach commented Apr 12, 2024

ryanpeach commented Apr 12, 2024

ryanpeach commented Apr 12, 2024

ryanpeach commented Apr 2, 2024 •

edited

ryanpeach commented Apr 10, 2024 •

edited

codecov-commenter commented Apr 10, 2024 •

edited

ryanpeach commented Apr 12, 2024 •

edited