{"action":"created","issue":{"url":"https://api.github.com/repos/ipython/ipython/issues/7324","labels_url":"https://api.github.com/repos/ipython/ipython/issues/7324/labels{/name}","comments_url":"https://api.github.com/repos/ipython/ipython/issues/7324/comments","events_url":"https://api.github.com/repos/ipython/ipython/issues/7324/events","html_url":"https://github.com/ipython/ipython/pull/7324","id":52949005,"number":7324,"title":"DEV: Refactor checkpoint logic out of FileContentsManager into a separate class.","user":{"login":"ssanderson","id":1588357,"avatar_url":"https://avatars.githubusercontent.com/u/1588357?v=3","gravatar_id":"","url":"https://api.github.com/users/ssanderson","html_url":"https://github.com/ssanderson","followers_url":"https://api.github.com/users/ssanderson/followers","following_url":"https://api.github.com/users/ssanderson/following{/other_user}","gists_url":"https://api.github.com/users/ssanderson/gists{/gist_id}","starred_url":"https://api.github.com/users/ssanderson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ssanderson/subscriptions","organizations_url":"https://api.github.com/users/ssanderson/orgs","repos_url":"https://api.github.com/users/ssanderson/repos","events_url":"https://api.github.com/users/ssanderson/events{/privacy}","received_events_url":"https://api.github.com/users/ssanderson/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/ipython/ipython/labels/-Needs+review","name":"-Needs review","color":"bfe5bf"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":27,"created_at":"2014-12-27T05:41:13Z","updated_at":"2015-01-07T19:28:44Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ipython/ipython/pulls/7324","html_url":"https://github.com/ipython/ipython/pull/7324","diff_url":"https://github.com/ipython/ipython/pull/7324.diff","patch_url":"https://github.com/ipython/ipython/pull/7324.patch"},"body":"The purpose of this change is to provide an API for users to replace just the checkpointing behavior of a ContentsManager.  This is achieved by delegating checkpoint management to a dedicated `CheckpointManager`.\r\n\r\n The impetus for this change came after I completed a full Contents API implementation in [pgcontents](https://github.com/quantopian/pgcontents) and realized that in many cases I actually wanted the files managed by my ContentsManager to be present in the filesystem.  (This was most notably true when uploading data files.)  After playing around with alternatives for a while, I realized that the \"Checkpoints API\" can be implemented independently of the rest of the Contents API.  This is valuable because it makes it possible to separate the storage backend for the current working version of a file from the storage of previous versions.  This significantly lowers the barrier to entry for doing remote storage of checkpoints as well as things like managing checkpoints through version control.\r\n\r\n**Changes:**\r\n\r\n- Add a `CheckpointManager` base class and infrastructure for creating a\r\n  `checkpoint_manager` instance attribute on `ContentsManager`.\r\n\r\n- Provide default implementations of `delete` and `rename` in the base\r\n  `ContentsManager` class.  **`ContentsManager` subclasses are now\r\n  required to implement `delete_file` and `rename_file`.**  These methods\r\n  no longer need to manage checkpoints.\r\n\r\n- Move checkpoint-related functionality from `FileContentsManager` to a\r\n  dedicated `FileCheckpointManager` subclass.\r\n\r\n- Move shared filesystem interaction logic into `FileManagerMixin`\r\n  used by both `FileContentsManager` and `FileCheckpointManager`.\r\n\r\n- Minor tweaks to ContentsManager tests to get methods from the right\r\n  object."},"comment":{"url":"https://api.github.com/repos/ipython/ipython/issues/comments/69076883","html_url":"https://github.com/ipython/ipython/pull/7324#issuecomment-69076883","issue_url":"https://api.github.com/repos/ipython/ipython/issues/7324","id":69076883,"user":{"login":"minrk","id":151929,"avatar_url":"https://avatars.githubusercontent.com/u/151929?v=3","gravatar_id":"","url":"https://api.github.com/users/minrk","html_url":"https://github.com/minrk","followers_url":"https://api.github.com/users/minrk/followers","following_url":"https://api.github.com/users/minrk/following{/other_user}","gists_url":"https://api.github.com/users/minrk/gists{/gist_id}","starred_url":"https://api.github.com/users/minrk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/minrk/subscriptions","organizations_url":"https://api.github.com/users/minrk/orgs","repos_url":"https://api.github.com/users/minrk/repos","events_url":"https://api.github.com/users/minrk/events{/privacy}","received_events_url":"https://api.github.com/users/minrk/received_events","type":"User","site_admin":false},"created_at":"2015-01-07T19:28:44Z","updated_at":"2015-01-07T19:28:44Z","body":"@ssanderson the issue with shutil vs model seems pretty generic to abstraction - using higher abstractions, it can be correct and reusable in more situations, but it eliminates the ability to do the simpler or preferable thing when the two storage mechanisms are the same - e.g. both on the filesystem or both in the same database. You have to read the whole notebook even when you could have created the checkpoint in an entirely remote operation without any potentially expensive reads.  This is why I think we *should* make assumptions about the ContentsManager for a given checkpoint mechanism.\r\n\r\nFor that reason, I think maybe changing the API to take just a path, and letting the CheckpointsManager decide how to get the data from ContentsManager will allow Contents and Checkpoints to be implemented in a more intimate way, which I expect to be the way to go most of the time. I would expect the 'get the generic model' route to be takenl only when the CheckpointsManager is a poor choice for use with a given ContentsManager.\r\n\r\nAs for walk, when we ship stuff we don't use, it tends to go stale and break without us knowing about it (same applies to your point about special-casing FileContentsManager with one branch taken in ~all cases, so I'm arguing against myself a bit here). This happens all the time (in-process kernel, pure Python kernel, etc.), and I would like to avoid it when possible. So it would be great if you shipped walk with pgcontents where you actually use it, rather than here where we don't, even though it's a generically useful operation.\r\n"}}