Skip to content
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

replace c.ServerApp.root_dir by environment variable #327

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jhgoebbert
Copy link

jupyter-ai generates files in the directory set by c.ServerApp.root_dir which might not be writable.
This MR adds the ability to overwrite this location with an environment variable called JUPYTERAI_SAVEDIR

If JUPYTERAI_SAVEDIR it will fall back to c.ServerApp.root_dir but checks if that directory is writeable.
If this is not the case it will take the current working directory without any further checks.

Possible fix for issue #326

@welcome
Copy link

welcome bot commented Aug 9, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@JasonWeill JasonWeill added the bug Something isn't working label Aug 9, 2023
@dlqqq
Copy link
Collaborator

dlqqq commented Aug 9, 2023

@jhgoebbert Welcome to Jupyter AI! We’re excited to have you contribute.

Jupyter projects use a library called traitlets for static configuration instead of environment variables. This allows JupyterLab to read from both static configuration files and command line arguments, and is generally preferable over environment variables.

I would recommend that we make root_dir a configurable trait on both LearnChatHandler and GenerateChatHandler, and rename the current root_dir kwarg and instance attribute to server_root_dir as well. root_dir should default to server_root_dir if unspecified.

Let me know if you'd like to implement this change. I'm happy to help.

@krassowski
Copy link
Member

While not directly solving this problem, there is a pull requests to traitlets adding an option allowing to configure any traitlet via environment variable:

As per ipython/traitlets#99 (comment) it is waiting for reviews from interested users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants