Fixes #634 bug: no metadata attribute

[telamonian]

Fixes the bug mentioned in #634 (comment). Previously, the monkey patch here:

jupytext/jupytext/contentsmanager.py

Lines 172 to 173 in aea84d9

	with mock.patch("nbformat.writes", _jupytext_writes(fmt)):
	return super(JupytextContentsManager, self).save(model, path)

called the parent save method, which would then call nbformat.from_dict:

            if model['type'] == 'notebook':
                nb = nbformat.from_dict(model['content'])
                self.check_and_sign(nb, path)
                self._save_notebook(os_path, nb)
                # One checkpoint should always exist for notebooks.
                if not self.checkpoints.list_checkpoints(path):
                    self.create_checkpoint(path)

before eventually calling jupytext.writes (via the patched nbformat.writes) as part of the _save_notebook call.

In the current code, jupytext.writes (which expects a NotebookNode, not just a notebook dict) is called before the parent save method:

jupytext/jupytext/contentsmanager.py

Lines 155 to 161 in af2b15b

	text_model = dict(
	type="file",
	format="text",
	content=jupytext.writes(model["content"], fmt=fmt),
	)
	return self.super.save(text_model, path)

so we need to explicitly add a conversion from dict to NotebookNode somewhere.

@mwouts Not sure if I put the nbformat.from_dict call in the ideal spot (I assume you know more about this than I do), but it does seem to fix the problem.

[codecov]

Codecov Report

Merging #635 into test_jupyterfs_metamanager will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                     Coverage Diff                     @@
##           test_jupyterfs_metamanager     #635   +/-   ##
===========================================================
  Coverage                       99.20%   99.20%           
===========================================================
  Files                              94       94           
  Lines                            9333     9334    +1     
===========================================================
+ Hits                             9259     9260    +1     
  Misses                             74       74           

Impacted Files	Coverage Δ
jupytext/contentsmanager.py	98.51% <100.00%> (+<0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af2b15b...a007c67. Read the comment docs.

[telamonian]

To my surprise, the fixes in #634 and #635 seem to be sufficient to fix all of the issues causing problems between jupytext and jupyter-fs.

I still think it's kind of a problem that

jupytext.cm wraps jupyterfs.cm wraps the normal contents managers

instead of

jupyterfs.cm wraps jupytext.cm wraps (each individually) the normal contents managers

But apparently jupytext.cm being at the top level still works fine, since it doesn't do anything to change the paths that jupyterfs.cm needs to correctly dispatch file operations to a given contents manager

[mwouts]

Thank you @telamonian , that is very helpful! I agree with the two commits: it is important to give more details when an exception occurs, and then you're right, I missed the conversion step that is performed in the parent save method.

I think I also see why the tests missed the problem... Many of the tests call cm.save, but they call the method on a notebook object (not a dict like in the real life!). I'll change this in a few tests to make sure we don't get the issue twice 😄

Also I will double check that we can still trust text notebooks... Your research shows that we won't call the self.check_and_sign(nb, path) method, I'd like to be sure that this will not cause problems...

[mwouts]

To my surprise, the fixes in #634 and #635 seem to be sufficient to fix all of the issues causing problems between jupytext and jupyter-fs

Well, that makes a total of three PR on the subject 😄
And already in #634 we had a test that tests Jupytext's CM on top on a MetaManager on top of a osfs file system - the only problem with the test was that it uses a notebook object rather than a dict:

jupytext/tests/test_contentsmanager.py

Lines 1735 to 1821 in af2b15b

	def fs_meta_manager(tmpdir):
	try:
	from jupyterfs.metamanager import MetaManager
	except ImportError:
	pytest.skip("jupyterfs is not available")
	cm_class = jupytext.build_jupytext_contents_manager_class(MetaManager)
	logger = logging.getLogger("jupyter-fs")
	cm = cm_class(parent=None, log=logger)
	cm.initResource(
	{
	"url": "osfs://{}".format(tmpdir),
	}
	)
	return cm
	def test_jupytext_jupyter_fs_metamanager(tmpdir):
	"""Test the basic get/save functions of Jupytext with a fs manager
	https://github.com/mwouts/jupytext/issues/618"""
	cm = fs_meta_manager(tmpdir)
	# the hash that corresponds to the osfs
	osfs = [h for h in cm._managers if h != ""][0]
	# save a few files
	text = "some text\n"
	cm.save(dict(type="file", content=text, format="text"), path=osfs + ":text.md")
	nb = new_notebook(
	cells=[new_markdown_cell("A markdown cell"), new_code_cell("1 + 1")]
	)
	cm.save(dict(type="notebook", content=nb), osfs + ":notebook.ipynb")
	cm.save(dict(type="notebook", content=nb), osfs + ":text_notebook.md")
	# list the directory
	directory = cm.get(osfs + ":/")
	assert set(file["name"] for file in directory["content"]) == {
	"text.md",
	"text_notebook.md",
	"notebook.ipynb",
	}
	# get the files
	model = cm.get(osfs + ":/text.md", type="file")
	assert model["type"] == "file"
	assert model["content"] == text
	model = cm.get(osfs + ":text.md", type="notebook")
	assert model["type"] == "notebook"
	# We only compare the cells, as kernelspecs are added to the notebook metadata
	compare(model["content"].cells, [new_markdown_cell(text.strip())])
	for nb_file in ["notebook.ipynb", "text_notebook.md"]:
	model = cm.get(osfs + ":" + nb_file)
	assert model["type"] == "notebook"
	actual_cells = model["content"].cells
	# saving adds 'trusted=True' to the code cell metadata
	for cell in actual_cells:
	cell.metadata = {}
	compare(actual_cells, nb.cells)
	def test_config_jupytext_jupyter_fs_meta_manager(tmpdir):
	"""Test the configuration of Jupytext with a fs manager"""
	tmpdir.join("jupytext.toml").write('default_jupytext_formats = "ipynb,py"')
	cm = fs_meta_manager(tmpdir)
	# the hash that corresponds to the osfs
	osfs = [h for h in cm._managers if h != ""][0]
	# save a few files
	nb = new_notebook()
	cm.save(dict(type="file", content="text", format="text"), path=osfs + ":text.md")
	cm.save(dict(type="notebook", content=nb), osfs + ":script.py")
	cm.save(dict(type="notebook", content=nb), osfs + ":text_notebook.md")
	cm.save(dict(type="notebook", content=nb), osfs + ":notebook.ipynb")
	# list the directory
	directory = cm.get(osfs + ":/")
	assert set(file["name"] for file in directory["content"]) == {
	"jupytext.toml",
	"text.md",
	"text_notebook.md",
	"notebook.ipynb",
	"notebook.py",
	"script.py",
	"script.ipynb",
	}

I still think it's kind of a problem that (...)
But apparently jupytext.cm being at the top level still works fine, since it doesn't do anything to change the paths that jupyterfs.cm needs to correctly dispatch file operations to a given contents manager

I quite agree with you... the other way around seems simpler... but, in the end, the purpose of #634 and #635 is to make Jupytext work on top of any contents manager, and a MetaManager is still a contents manager, isn't it? And also maybe it is simpler to wrap Jupytext around a single contents manager rather than around each of the sub managers?

[telamonian]

I agree with your ambivalence. The current way in this PR seem to work, so let's roll with it 👍

If it does cause problems down the line, I'll add a cm_wrapper hook (or something) to jupyter-fs that jupytext can use to pass along build_jupytext_contents_manager_class, which jupyter-fs could then use whenever it creates a new cm