feat & bugfix: Added metadata features and fixed naming bug #11
Conversation
|
@masci should we disable more lint errors? I am not sure if these are valid errors, wdyt? |
src/banks/registries/directory.py
Outdated
| msg = f"Prompt {name}.{version}.jinja not found in the index. Cannot set meta for a non-existing prompt." | ||
| raise ValueError(msg) | ||
|
|
||
| if f"{name}:{version}.json" in self._meta_path.glob("*.json"): |
There was a problem hiding this comment.
glob returns a generator so this should be something like
if f"{name}:{version}.json" in list(self._meta_path.glob("*.json"))
is it working?
There was a problem hiding this comment.
You are right, it wasn't. I fixed and added unit tests as well. :)
Thanks for catching this one 🙌
|
Thanks a lot @masci 🙏 What do you think of the PR? |
src/banks/registries/directory.py
Outdated
| if not self._index_path.exists() or force_reindex: | ||
| self._scan() | ||
| else: | ||
| self._load() | ||
| self._meta_path = self._path / DEFAULT_META_PATH |
There was a problem hiding this comment.
Any reason why we want to use a separate index for the metadata? If we extend the PromptFile model to include meta: dict[str, Any] we would piggy back on the existing get/set infraastructure
There was a problem hiding this comment.
I agree with this, I would make the changes.
src/banks/registries/directory.py
Outdated
| raise FileNotFoundError(msg) | ||
| return json.loads(open(meta_path, encoding="utf-8").read()) | ||
|
|
||
| def set_meta(self, *, meta: dict, name: str, version: str | None = None, overwrite: bool = False): |
There was a problem hiding this comment.
I would try to pass meta to the regular set() (see comment above)
There was a problem hiding this comment.
I would do that but I will also provide set_meta separately because there are many use cases (i.e., adding accuracy, adding failed stats) that require setting meta after running the prompt. We also need such things per prompt so I will still keep the different folders for storing the meta and the prompt/index.
| @@ -26,7 +32,7 @@ def __init__(self, directory_path: Path, *, force_reindex: bool = False): | |||
| raise ValueError(msg) | |||
|
|
|||
| self._path = directory_path | |||
| self._index_path = self._path / "index.json" | |||
| self._index_path = self._path / DEFAULT_INDEX_NAME | |||
src/banks/registries/directory.py
Outdated
| self._index.files.append(pf) | ||
| self._index_path.write_text(self._index.model_dump_json()) | ||
|
|
||
| def get(self, *, name: str, version: str | None = None) -> "Prompt": | ||
| version = version or "0" | ||
| def get_prompt(self, *, name: str, version: str | None = None) -> "Prompt": |
There was a problem hiding this comment.
Why changing the method name? Its counterpart is still set
There was a problem hiding this comment.
Because with set we can set both meta and prompt. I thought being explicit might be better?
There was a problem hiding this comment.
Ah now I remember the convo, yeah let's scratch this. But I think get_meta will end up being just an utility method calling get_prompt under the hood? See comment below.
src/banks/registries/directory.py
Outdated
| pf = PromptFile(name=name, version=version, path=new_prompt_file, meta=meta) | ||
| return pf | ||
|
|
||
| def set( # pylint: disable=too-many-arguments |
There was a problem hiding this comment.
I think we can relax this rule to 10 args per method
There was a problem hiding this comment.
Cool will do that.
src/banks/registries/directory.py
Outdated
| name: str, | ||
| prompt: Prompt, | ||
| meta: dict | None = None, | ||
| version: str | None = None, |
There was a problem hiding this comment.
Since strings are immutable, why not defaulting here directly? version: str = DEFAULT_VERSION,
There was a problem hiding this comment.
Makes sense, will do it.
src/banks/registries/directory.py
Outdated
| for pf in self._index.files: | ||
| if pf.name == name and pf.version == version: | ||
| return pf.meta | ||
| raise MetaNotFoundError |
There was a problem hiding this comment.
Not sure about this, the error will be triggered by the fact the requested pf was not found in the index... Also what do you want to signal with MetaNotFound, the absence of the dict or an emty dict?
There was a problem hiding this comment.
It was like that before. I changed it to be consistent with get_prompt, it also raises TemplateNotFoundError when the prompt is not found.
The idea was that if someone makes a mistake in the name, they should get the error instead of silently returning the empty dict.
There was a problem hiding this comment.
What about
for pf in self._index.files:
if pf.name == name and pf.version == version:
if not pf.meta:
raise MetaNotFoundError
return pf
raise TemplateNotFoundErrorThere was a problem hiding this comment.
Makes sense 🙌
There was a problem hiding this comment.
Actually, why not just return self.get_prompt(...).meta?
Allowing users to store metadata for each prompt
First create
metasubdir inside the root template dir and then create prompt metadata.