JSON Schema Rewrite

[riedgar-ms]

Rewriting the JSON Schema generator to be more remote from the actual grammar. @hudson-ai provided the code which allowed recursive structures (such as linked lists) to be created.

[riedgar-ms]

Tagging @hudson-ai for interest

[hudson-ai]

Are you open to including enum types and objects with additionalProperties in this PR, or should those be saved for the next one?

[hudson-ai]

May I also suggest changing collections.abc.Mapping[str, any], to collections.abc.Mapping[str, Any]? any is not a type, but Any from typing is :)

[hudson-ai]

Also for 3.8, Mapping and MutableMapping should be imported from typing if you want to subscript them.

[riedgar-ms]

@hudson-ai thanks for the Python 3.8 warning :-/

[Harsha-Nori]

We can pull these imports straight from the guidance.library namespace instead of pulling through the private files (https://github.com/guidance-ai/guidance/blob/main/guidance/library/__init__.py)

[riedgar-ms]

Tweaked a bit. The _grammar import isn't working, though

[hudson-ai]

A bit annoying, but I recommend something like this for the Mapping and MutableMapping interfaces:

import sys
if sys.version_info >= (3, 9, 0):
    from collections.abc import Mapping, MutableMapping
else:
    from typing import Mapping, MutableMapping

This is because as of 3.9, typing's versions are deprecated aliases to the abc ones and will be removed in future versions of python -- see https://docs.python.org/3/library/typing.html#typing.Mapping

[riedgar-ms]

@slundberg do you have thoughts on this? @hudson-ai has submitted a PR to my fork which avoids the problem by using a Python closure to do lazy evaluation. However, I suspect that that is not going to serialise well. I have a feeling that the previous tweak proposed for Placeholder is going to be what is needed.

Can you comment on this?

Also, do you want to wait for that to be sorted before merging this? I could make my test case be an XFAIL while the exact update for Placeholder is figured out. If you are concerned about people picking up and using my wrong initial implementation, then this does need to merge reasonably quickly.

[hudson-ai]

While closures can be a pain for serialization, there is no issue serializing the grammars generated using the approach in my PR (as far as I can tell). The code that generates the grammars has closures, but the grammars themselves don't.

That being said, the closure approach feels like a "workaround" to play nicely with how the library currently uses Placeholders, and caching guidance-decorated functions with a nonzero number of args would remove the need for the workaround.

[riedgar-ms]

@hudson-ai I've just tested your change with the example server in guidance, and it does serialise correctly. I've added a test which shows this

[slundberg]

I feel I was mistaken thinking I ever got this working...

@slundberg can you advise on how to make the following pattern not infinitely recurse? This is a huge simplification of what we're trying to do here, but it has the same failure mode as far as I can tell.

@guidance(stateless=True)
def repeat(lm, arg):
   return lm + arg + select([repeat(arg), ''])

The following works just fine because of the placeholder business when a function has no args, but with args above, we get infinite recursion.

@guidance(stateless=True)
def repeat_x(lm):
   return lm + 'x' + select([repeat_x(), ''])

There is a cache argument in the decorator, but it doesn't seem to help. Any ideas?

Sorry to be slow here. Infinite recursion is avoided by caching the function calls, however as you point out we just avoid caching all together if the function has an argument. We do this because it is challenging to correctly (and efficiently) check for equality over arbitrary input arguments. Good idea with closures. The alternative would be to add some basic argument equality checking to the guidance function caching (this would make writing these kinds of grammars easier in the future).

[hudson-ai]

Sorry to be slow here. Infinite recursion is avoided by caching the function calls, however as you point out we just avoid caching all together if the function has an argument. We do this because it is challenging to correctly (and efficiently) check for equality over arbitrary input arguments. Good idea with closures. The alternative would be to add some basic argument equality checking to the guidance function caching (this would make writing these kinds of grammars easier in the future).

@slundberg I think that adding more caching would be great, although this particular use-case wouldn't be very easy -- simply because the input is an un-hashable dictionary 😅

Off-topic to this PR -- will make an issue when I get the chance:
Maybe in the meantime, it would be nice to catch un-cached recursive calls and give users a nicer error that explains they are infinitely recursing because the current version of guidance only supports no-arg recursion? I only figured this out because I looked closely at the source code of the decorator. I doubt we want users to have to do that ;)

[riedgar-ms]

@slundberg were there any other changes you wanted to this PR? I think it's ready to merge.

[riedgar-ms]

Ping @slundberg .....

[slundberg]

Okay took a pass through this and am set to merge once the tests run. Thank you again @riedgar-ms and sorry it took a bit to review. The only change I made was to shorten the name of the library function from gen_json to just json. We could put "gen" in front of every library function if we wanted since they all result in generation...so we just leave them off normally. In this case that makes is easier for a library import conflict, but I think it is better for the user to pick what they want (either guidance.json or from guidance import json as my_gen_json)

Thanks!

[slundberg]

@riedgar-ms I went ahead and merged this, but there seems to be an unrelated build error on the new macOS tests....might be worth looking into.