Document how to create completions using full notebook content #777
Conversation
docs/source/developers/index.md
Outdated
| prefix = request.prefix | ||
| suffix = request.suffix.strip() | ||
|
|
||
| server_ydoc = self.settings.get("jupyter_server_ydoc", None) |
There was a problem hiding this comment.
The problem is that the provider does not have access to settings. We would need to expose the jupyter_server_ydoc either:
- on provider instances as an optional attribute,
- as an argument to
generate_inline_completionsandstream_inline_completions
if this is a use-case we want to support. Any thoughts?
There was a problem hiding this comment.
Alternatively, jupyter-ai could take the responsibility of extracting the document (YNotebook) and pass it to generate_inline_completions/stream_inline_completions. This way the providers would be restricted to accessing collaborative model of the document they were invoked on (rather than of any document user has open).
There was a problem hiding this comment.
Checking back on this one :)
There was a problem hiding this comment.
If both solutions are fine and either would be accepted, that would be helpful to know too - more than radio silence. I just do not want to work on a PR to learn that the maintainers would not accept it anyways.
There was a problem hiding this comment.
Some thoughts as I started an implementation (see the new commits below):
- should we call this
server_settingsto avoid confusion? - should it be a class attribute? It seems easier from implementation perspective because then we can just set it once on
BaseProviderlevel and then we do not need to update it ever; otherwise we have 4 places where providers get initialized so it would seem very repetitive - using
MappingProxyTypeoverDictwould narrow down the public API - ideally though I think we may want to filter which settings get exposed; this is not trivial - we cannot filter it at initialization time because extensions may load in different order so
jupyter_server_ydocmay not yet be in the dictionary; instead it would need to be filtered on access (e.g. in a custom__getitem__(self, key: str) -> Any
There was a problem hiding this comment.
@krassowski Hey Mike, I just got back from vacation. Let me take a look at this either this afternoon or tomorrow morning.
There was a problem hiding this comment.
I hope you had a good vacation!
There was a problem hiding this comment.
@krassowski Got sick yesterday but feeling better.
- I agree with using the
server_settingsattribute name. - A class attribute is fine since this attribute should only be set by
jupyter_aionce per Python process; I agree that using an instance attribute can be tedious because of how many placesBaseProvideris being instantiated in.
using MappingProxyType over Dict would narrow down the public API
Does this just serve to hint that this dictionary is read-only? I'm not familiar with when this type should be used.
ideally though I think we may want to filter which settings get exposed; this is not trivial
I think it makes more sense to include the entire server_settings dictionary. I don't see an obvious benefit to only allow-listing some set of keys to BaseProvider. Is this motivated by a data privacy/security concern?
There was a problem hiding this comment.
Does this just serve to hint that this dictionary is read-only?
MappingProxyType is in fact read-only. Assigning to it would raise:
TypeError: 'mappingproxy' object does not support item assignment
Is this motivated by a data privacy/security concern?
More that once we add things to API then we should not remove them without making a major release.
krassowski
force-pushed
the
document-full-notebook-extraction
branch
from
09eb901 to
ecb6616
Compare
dlqqq
force-pushed
the
document-full-notebook-extraction
branch
from
ecb6616 to
3ce9931
Compare
| @@ -265,6 +266,13 @@ class Config: | |||
| provider is selected. | |||
| """ | |||
|
|
|||
| server_settings: ClassVar[Optional[MappingProxyType[str, Any]]] = None | |||
There was a problem hiding this comment.
Can you check if there's a way to raise an exception when this class attr more than once? This is not a PR blocker, but it would be a good sanity check to include for developers that are accidentally assigning to this class attr again in their own server extension.
There was a problem hiding this comment.
I agree that this would be nice to have. It would be trivial for an instance attribute with a property descriptor. I think it is feasible for a class instance too by using a metaclass. Taking a look
There was a problem hiding this comment.
I will merge this PR and open a separate follow-up PR to improve this.
dlqqq
force-pushed
the
document-full-notebook-extraction
branch
from
3ce9931 to
ae23db8
Compare
Depending on the order in which extensions are initialized `jupyter_server_ydoc` may or may not be there; instead we may want to define a custom proxy which would restrict access on access to `__getitem__()`
dlqqq
force-pushed
the
document-full-notebook-extraction
branch
from
ae23db8 to
aa3436f
Compare
…erlab#777) * Document how to create completions using full notebook * Pass a subset of server-settings down to providers * Do not filter the settings object just yet Depending on the order in which extensions are initialized `jupyter_server_ydoc` may or may not be there; instead we may want to define a custom proxy which would restrict access on access to `__getitem__()` * `jupyter_server_ydoc` is accessible in web_app.settings * Add missing async/await
Closes #708