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

Enabling absolute checkpoint dir #1283

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lade-odoo
Copy link

Sometimes, we don't want the checkpoints to be saved into our working directory. Instead, we may want to recreate the project structure inside a separate folder (e.g.: /home/{user}/.cache) like some software are doing.

@welcome
Copy link

welcome bot commented May 26, 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! 🎉

@kevin-bates
Copy link
Member

Hi @lade-odoo - thanks for opening this pull request. I have a couple of questions that come to mind...

  1. Won't this approach encounter duplicate checkpoint files otherwise protected by directory/file semantics? For example, if I have two notebooks named Untitled.ipynb residing in separate directories (dir1 and dir2), won't this lead to the same checkpoint path for each file (e.g., Untitled-checkpoint.ipynb)?
  2. Should there be any validation for where this single checkpoint directory can reside? I.e., should it reside within the root_dir (aka notebook_dir)?

@lade-odoo
Copy link
Author

@kevin-bates Thank you for you questions:

  1. No, that's the point, there won't be any duplicate. The directory structure will be reconstructed inside the checkpoint_dir folder. So, following your scenario, there will be two files created: dir1/Untitled-checkpoint.ipynb and dir2/Untitled-checkpoint.ipynb.
  2. From my point of view, I like the idea of being able to put it anywhere we want (we leave this freedom to the user).

@kevin-bates
Copy link
Member

No, that's the point, there won't be any duplicate. The directory structure will be reconstructed inside the checkpoint_dir folder.

Shoot - I'm sorry, I missed that piece.

I like the idea of being able to put it anywhere we want (we leave this freedom to the user).

I tend to agree. One caveat with an absolute checkpoint-dir would occur if users change their root_dir from one invocation to the next. In those instances, the checkpoint file's location will vary within the single checkpoint directory structure. I wouldn't expect that to commonly occur and could probably be addressed via the help string stating that an absolute checkpoint dir essentially requires consistent root_dir values between invocations.

Hmm - I suppose for users that change their root_dir locations entirely (i.e., disjoint hierarchies), you could have similar sub-directory structures in place that could lead to the duplicates issue. But, as you point out, perhaps absolute checkpoint directories are not for them.

@lade-odoo lade-odoo force-pushed the main-enable_absolute_checkpoint_dir branch from f6c0668 to 424d086 Compare June 22, 2023 07:12
Sometimes, we don't want the checkpoints to be saved into our working
directory. Instead, we may want to recreate the project structure inside
a separate folder (e.g.: /home/{user}/.cache) like some software are
doing.
@lade-odoo lade-odoo force-pushed the main-enable_absolute_checkpoint_dir branch from 424d086 to f7dc7e9 Compare September 12, 2023 14:40
@lade-odoo
Copy link
Author

@kevin-bates Is there any-news here ?

@kevin-bates
Copy link
Member

Hi @lade-odoo - thank you for the ping and I apologize for my inactivity (new job outside of Jupyter OSS).

I'm going to approve this PR, but I'm hoping someone closer to the Contents Service will take look prior to its merge. I'm just not sure if this might side-affect things in any non-obvious ways.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me - thank you @lade-odoo. I'm hoping someone closer to the Contents Service can take a look at this prior to its merge.

@blink1073
Copy link
Contributor

From a security standpoint, we only ever write to files that are contained withing the startup directory. This would have to be an opt-in feature, as a new traitlet on FileCheckpoints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants