[Feature] Add extra support to JSON string schema #927

riedgar-ms · 2024-06-25T18:27:29Z

Looking to add support for pattern, minLength and maxLength to strings in JSON schema. This should address half of #925

This has lead to the creation of a few new library functions to help out, and consolidation into a new _sequences.py library file. It is also hooked into _regex.py.

hudson-ai · 2024-06-25T19:12:40Z

Thanks for getting this started! Just a few comments for you as you're working on this...

I'm a little bit wary about exposing direct regex generation via pattern as it will be difficult to enforce that a user's regex is properly escaped and will lead to something that is JSON loads-able... Although maybe all we have to do is document that using this functionality is "at your own risk"
In the same vein, it may be impossible to enforce both a pattern and min/max length...
I think you can do away with the direct select call inside _gen_json_string and just use a regex (optionally adding {min,max})... Take a look at the regex I sent on the related issue -- may be helpful

riedgar-ms · 2024-06-25T19:48:24Z

Thanks for getting this started! Just a few comments for you as you're working on this...

I'm a little bit wary about exposing direct regex generation via pattern as it will be difficult to enforce that a user's regex is properly escaped and will lead to something that is JSON loads-able... Although maybe all we have to do is document that using this functionality is "at your own risk"

In the same vein, it may be impossible to enforce both a pattern and min/max length...

I think you can do away with the direct select call inside _gen_json_string and just use a regex (optionally adding {min,max})... Take a look at the regex I sent on the related issue -- may be helpful

For (1), I'd absolutely say that it's "at your own risk" functionality.

For (2), I'm quite confident that we cannot simultaneously enforce both a pattern and a min/maxLength. Imagine something like "pattern": "aaaa", "maxLength": 2 which is impossible to satisfy.

Of more immediate concern is that my negative test cases are failing really weirdly :-/

hudson-ai · 2024-06-25T23:31:26Z

Ah, the {b'"'} != {b'"'} isn't as weird as it looks -- it's a repr problem. Annoyingly valid_next_bytes doesn't actually return a set of bytes; it returns a set of Bytes and ByteRanges (which look like bytes in their reprs).

riedgar-ms · 2024-06-26T00:02:54Z

Ah, the {b'"'} != {b'"'} isn't as weird as it looks -- it's a repr problem. Annoyingly valid_next_bytes doesn't actually return a set of bytes; it returns a set of Bytes and ByteRanges (which look like bytes in their reprs).

That was on my "to investigate" list; was suspecting something like that.

Of more immediate concern: if you look closely at the failing tests, I believe you'll find that they ought to be passing.

hudson-ai · 2024-06-26T02:33:55Z

guidance/library/_json.py

+from ._at_most_n_repeats import at_most_n_repeats
+from ._exactly_n_repeats import exactly_n_repeats


Could be nice to use/reuse these to reduce redundant code in the regex implementation :)

guidance/library/_json.py

hudson-ai · 2024-06-26T02:37:25Z

guidance/library/_json.py

+        lm += exactly_n_repeats(value=select(STRING_CHARS), n_repeats=min_length)
+        lm += at_most_n_repeats(value=select(STRING_CHARS), n_repeats=(max_length - min_length))
+    else:
+        lm += select(
+            STRING_CHARS,
+            recurse=True,
+        )


This functionality feels reusable! I.e. repeat(value, min, max) with sane behaviors when min and max are unset. This logic happens in the regex code too, so you should be able to consolidate

tests/unit/library/test_json.py

codecov-commenter · 2024-06-26T14:51:13Z

Codecov Report

Attention: Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.

Project coverage is 59.18%. Comparing base (ba86754) to head (bd30725).

Files	Patch %	Lines
guidance/library/_json.py	96.00%	1 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #927      +/-   ##
==========================================
+ Coverage   57.18%   59.18%   +1.99%     
==========================================
  Files          64       63       -1     
  Lines        4711     4733      +22     
==========================================
+ Hits         2694     2801     +107     
+ Misses       2017     1932      -85

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

riedgar-ms · 2024-06-26T15:03:18Z

tests/unit/library/test_json.py

+    @pytest.mark.parametrize(
+        ["bad_string", "good_bytes", "failure_byte", "allowed_bytes"],
+        [
+            ('""', b'"', b'"', None),


I had hoped to be able to use Select(STRING_CHARS)._values for the 'allowed_bytes' in this 'too short' case, but that wasn't quite getting it right. So I'm allowing None to be passed, to turn off that check

guidance/library/_json.py

hudson-ai · 2024-06-27T18:08:32Z

guidance/library/_sequences.py

+        model += at_most_n_repeats(value=value, n_repeats=(max_length - min_length))
+    elif min_length is not None:
+        model += exactly_n_repeats(value=value, n_repeats=min_length)
+        model += select([optional(value)], recurse=True)


Can you reuse zero_or_more here to reduce code duplication?

I had actually been planning on reimplementing those two functions in terms of sequence(). Of course, something isn't quite right, and the errors I'm getting are very Pythonic.

Oh fair enough. Sorry about your python 😂

There is one test involving the mocked model which is interfering with zero_or_more() being a synonym for sequence(). It's related to the max_tokens argument on gen, so not a core part of this PR, though.

Would you open an issue if it feels worthy? :)

Done. It could also be something strange in the way the mock model is working. But it would be good to run down.

guidance/library/_sequences.py

hudson-ai · 2024-06-27T18:13:51Z

Looking good! One ask -- could you maybe reuse the sequence code inside of _regex.py so we don't have two implementations of it?

guidance/library/_regex.py

hudson-ai

LGTM! Thanks @riedgar-ms

Trying to get some things together

981a55c

riedgar-ms added 2 commits June 25, 2024 15:40

Some more work

6166c5f

We have a problem with failure cases.....

73f5f65

hudson-ai reviewed Jun 26, 2024

View reviewed changes

guidance/library/_json.py Outdated Show resolved Hide resolved

hudson-ai reviewed Jun 26, 2024

View reviewed changes

tests/unit/library/test_json.py Outdated Show resolved Hide resolved

riedgar-ms added 5 commits June 26, 2024 09:38

Getting some stuff sorted

8fcea51

A few extra tests

a530bfc

Explanatory comments

7c1dc06

Tweak check_match_failure

ba49df2

minLength only

29cb9f5

And the last bit

d0c4e90

riedgar-ms commented Jun 26, 2024

View reviewed changes

riedgar-ms added 2 commits June 26, 2024 11:37

Refactor

4c5dd31

Sort imports

7dad6f3

riedgar-ms changed the title ~~[WIP] [Feature] Add extra support to JSON string schema~~ [Feature] Add extra support to JSON string schema Jun 26, 2024

riedgar-ms requested a review from hudson-ai June 27, 2024 11:43

riedgar-ms added 7 commits June 27, 2024 10:05

Working on more specific testing

4250c3e

More testing

65dc931

A little more testing

486d607

Start expanding testing

a4cc8dc

More testing

6533988

Linting

bdea9b6

More testing

a0e3ec8

riedgar-ms added 3 commits June 27, 2024 11:00

Some more test cases

406f196

Some negative tests

417ce6c

And a little more testing

32f0588

hudson-ai reviewed Jun 27, 2024

View reviewed changes

guidance/library/_json.py Outdated Show resolved Hide resolved

hudson-ai reviewed Jun 27, 2024

View reviewed changes

guidance/library/_sequences.py Outdated Show resolved Hide resolved

riedgar-ms added 5 commits June 27, 2024 14:19

Simplify logic

c7f9b7c

Better error message

63d2e73

Switch to sequence

8a5855d

Rewrite

bc4b19c

Use sequence in Regex

4b14315

riedgar-ms commented Jun 27, 2024

View reviewed changes

guidance/library/_regex.py Show resolved Hide resolved

Unused import

bd30725

riedgar-ms requested review from hudson-ai, Harsha-Nori and paulbkoch June 27, 2024 20:37

hudson-ai approved these changes Jun 27, 2024

View reviewed changes

riedgar-ms mentioned this pull request Jun 27, 2024

[Bug] sequence() not quite replacement for zero_or_more() #932

Open

riedgar-ms merged commit 137130f into guidance-ai:main Jun 27, 2024
100 checks passed

riedgar-ms deleted the riedgar-ms/json-string-length-01 branch June 27, 2024 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Add extra support to JSON string schema #927

[Feature] Add extra support to JSON string schema #927

riedgar-ms commented Jun 25, 2024 •

edited

Loading

hudson-ai commented Jun 25, 2024

riedgar-ms commented Jun 25, 2024

hudson-ai commented Jun 25, 2024

riedgar-ms commented Jun 26, 2024

hudson-ai Jun 26, 2024

hudson-ai Jun 26, 2024

codecov-commenter commented Jun 26, 2024 •

edited

Loading

riedgar-ms Jun 26, 2024

hudson-ai Jun 27, 2024

riedgar-ms Jun 27, 2024

hudson-ai Jun 27, 2024

riedgar-ms Jun 27, 2024

hudson-ai Jun 27, 2024

riedgar-ms Jun 27, 2024

hudson-ai commented Jun 27, 2024

hudson-ai left a comment

		from ._at_most_n_repeats import at_most_n_repeats
		from ._exactly_n_repeats import exactly_n_repeats

[Feature] Add extra support to JSON string schema #927

[Feature] Add extra support to JSON string schema #927

Conversation

riedgar-ms commented Jun 25, 2024 • edited Loading

hudson-ai commented Jun 25, 2024

riedgar-ms commented Jun 25, 2024

hudson-ai commented Jun 25, 2024

riedgar-ms commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 26, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

hudson-ai commented Jun 27, 2024

hudson-ai left a comment

Choose a reason for hiding this comment

riedgar-ms commented Jun 25, 2024 •

edited

Loading

codecov-commenter commented Jun 26, 2024 •

edited

Loading