Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
AI Representation #129
base: master
Are you sure you want to change the base?
AI Representation #129
Changes from 5 commits
8f8711c
b278d7f
90d20be
7e4fccf
f805b4e
1282304
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: Consumers of this protocol (e.g. Jupyter AI) will likely not be able to support every MIME type, nor do they need to. Jupyter AI should document which MIME types we read from. That way, people implementing these methods know how to define
_ai_repr_()
to provide usable reprs for Jupyter AI. Other consuming extensions should do the same.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth defining a type for the dictionary returned by
_ai_repr_()
instead of usingDict[str, Any]
? This may be important in the case of image reprs, since the same MIME type may be encoded in different ways. For example, it's ambiguous as to whetherimage/png
may return abytes
object or a base64-encodedstring
.MIME types do allow encodings/parameters to also be specified, so
image/png;base64
could refer to astring
object whileimage/png
could refer to abytes
object. It may be worth defining this in the same proposal to reduce ambiguity.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, one problem is that using a
TypedDict
doesn't allow extra keys to be defined. So if we define_ai_repr_() -> AiRepr
, implementers can only use the keys we define in theAiRepr
type. Another issue that this custom type would have to be provided by some dependency, which means every consumer & implementer needs 1 extra dependency.We should continue to think of ways to provide better type safety guarantees on the values in the returned dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be better to have the async version live in a different method, e.g.
_async_ai_repr_()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to allow dots in names ? it may be property and trigger side effects, but I think that is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's ok too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the
kwargs
structure needs to be documented somewhere. Would this be standardized by the kernel implementation when handlingai_repr_request
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my understanding is that they should be some consensus, but some free parameters that could depends on which repr understand the need of which models.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intent was not to be prescriptive here and let that evolve naturally. That is, given
jupyter-ai
popularity, I expect it to implicitly set some kwargs as a standard. No kwarg should be required is maybe a better statement here, but plugins are free to document things they support.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A registry can define AI representations for objects that lack a
_ai_repr_()
. It seems natural to also a registry to define a "fallback" AI representation, i.e. the method to be used when neither a_ai_repr_()
method or a registry entry exist for an object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is the registry is already the fallback, because the object does not have a
_ai_repr_
, Unless you see the registry as an override of_ai_repr_
?