Load personas dynamically from .jupyter dir
#1380
Conversation
| @@ -135,6 +139,10 @@ def _init_persona_classes(self) -> None: | |||
| "ERROR: Jupyter AI has no AI personas available. " | |||
| + "Please verify your server configuration and open a new issue on our GitHub repo if this warning persists." | |||
| ) | |||
|
|
|||
| # TODO: check whether in the end we need a full class for this, or if we can just use a simple function that returns a list of persona classes from the local filesystem. | |||
There was a problem hiding this comment.
Yeah, my sense so far is that a simple function will be simpler and cleaner. The only thing that I could imagine needing state for is for caching, but that can be done with a decorator.
| @@ -135,6 +139,10 @@ def _init_persona_classes(self) -> None: | |||
| "ERROR: Jupyter AI has no AI personas available. " | |||
| + "Please verify your server configuration and open a new issue on our GitHub repo if this warning persists." | |||
| ) | |||
|
|
|||
| # TODO: check whether in the end we need a full class for this, or if we can just use a simple function that returns a list of persona classes from the local filesystem. | |||
| persona_classes.extend(LocalPersonaLoader(self.root_dir).load_persona_classes()) | |||
There was a problem hiding this comment.
We might want to wrap this in a try/except once we get a sense of what exceptions might get triggered.
There was a problem hiding this comment.
One thing we will need to think about is if a user moves the chat file while it is open (which instantiates the PersonaManager) such that a new .jupyter directory is relevant. That and we also want to reload the modules.
| directory. | ||
| """ | ||
|
|
||
| def __init__( |
There was a problem hiding this comment.
This will all go away, but we usually avoid using an init method with LoggingConfigurable. If we need to keep the class we can share examples, but my sense is to switch to a function.
There was a problem hiding this comment.
Yup, gone to function now (though I later added back the logger, just not making a new one, simply passing it as an arg).
| def __init__( | ||
| self, | ||
| *args, | ||
| root_dir: str | None = None, |
There was a problem hiding this comment.
I will soon have #1376 ready with the right logic for the dir to pass here.
| return persona_classes | ||
|
|
||
| # Find all .py files in the root directory that contain "persona" in the name | ||
| all_py_files = glob(os.path.join(self.root_dir, "*.py")) |
There was a problem hiding this comment.
I think this could trigger a permission error.
There was a problem hiding this comment.
Wrapped in try/except now.
| for py_file in py_files: | ||
| try: | ||
| # Get module name from file path | ||
| module_name = Path(py_file).stem |
There was a problem hiding this comment.
Might want a private function that takes a py_file and returns the persona class or None. When we start to cache and enable reloading, we may want that encapsulation (this can wait until we really need it).
| module_name = Path(py_file).stem | ||
|
|
||
| # Skip if module name starts with underscore (private modules) | ||
| if module_name.startswith("_"): |
There was a problem hiding this comment.
Should we also include leading . in this?
There was a problem hiding this comment.
Done in commit 2517286, thx!
| ): | ||
| module_persona_classes.append(obj) | ||
|
|
||
| if module_persona_classes: |
There was a problem hiding this comment.
Nice that we can support multiple per file :-)
Only match (case insensitively) Python files with `persona` in the name.
for more information, see https://pre-commit.ci
… file. Also, rename local loader to a shorter name.
…r loop, clarify logging
dlqqq
changed the title
.jupyter dir
There was a problem hiding this comment.
Thank you Fernando, this is awesome! I pushed a commit fixing the mypy errors and opened a follow-up issue (#1395) that may help mitigate the performance impact of this PR. @ellisonbg @3coins Feel free to merge once you feel this is ready.
Draft PR to get a first look at the approach, will refine based on early feedback before proper review.
Implements a local persona loader that targets pure python files with
personain the name, and loads all personas in those files that subclassBasePersona.This is meant to be called by the main
PersonaManagerclass safely.Follow-up issues