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

Jupytext manager seems to fail with jupyterfs #618

Closed
joequant opened this issue Sep 8, 2020 · 11 comments · Fixed by #621 or #634
Closed

Jupytext manager seems to fail with jupyterfs #618

joequant opened this issue Sep 8, 2020 · 11 comments · Fixed by #621 or #634
Milestone

Comments

@joequant
Copy link

joequant commented Sep 8, 2020

The content manager for jupytext seems to want a call _get_os_path which seems to break with jupyter-fs metamanager
see jpmorganchase/jupyter-fs#53

@mwouts
Copy link
Owner

mwouts commented Sep 8, 2020

Hi @joequant , thank you for the report! Well that is interesting, indeed the Jupytext content manager does use _get_os_path. I agree that this may not make sense for every content manager... Later on in the week I'll have a look at this and see if we can do otherwise.

@joequant
Copy link
Author

joequant commented Sep 9, 2020

Thanks. What the jupyter-fs metamanager does is to take other managers and dispatch them, so the way to get it to work is to have jupyterfs wrapper jupytext inside an internal manager. Getting jupyterfs to work with jupytext looks straight foward. The hard part is to figure out some general mechanism.

Just thinking about this, one thing I'd suggest is maybe do the wrappering only if the content manager is subclass of something that's known to work. Otherwise look for some hook where the content manager can provide a monkey patched class.

@mwouts
Copy link
Owner

mwouts commented Sep 11, 2020

Hi @joequant , I think I have a fix for this. Do you think you could give a try to the branch jupytext_works_with_jupyterfs?

I still have to think about

  • how to explore the file system for the config file
  • how to create directories when paired files use a prefix

(e.g. there are still two try/except AttributeErrors, I'll see if I can remove them)

@mwouts
Copy link
Owner

mwouts commented Sep 12, 2020

In the latest version (on master), the contents manager uses the basic CM methods (get, save) for all the operations (including searching for the config file), so it should now work well with jupyter-fs.

I plan to ship a new version within a few days. Meanwhile you can give the dev version a try with:

pip install git+https://github.com/mwouts/jupytext.git

@joequant
Copy link
Author

Thanks!!!

@mwouts
Copy link
Owner

mwouts commented Sep 21, 2020

Reopening, cf. #621

No joy. Getting a missing _notebook_model error when I create a new notebook
The problem is that the toplevel contentmanager in jupyter-fs is metamanager.py and not fsmanager.py
Metamanager.py is the top level contentmanager and dispatches calls to a list of fsmanagers.

@joequant and myself will adapt the test test_jupytext_jupyter_fs_manager to make it more representative of how jupyter-fs is used in Jupyter.

@mwouts mwouts reopened this Sep 21, 2020
@mwouts mwouts added this to the 1.6.1 milestone Sep 22, 2020
@mwouts
Copy link
Owner

mwouts commented Sep 27, 2020

@joequant , I think I've solved the remaining issues. Would you mind giving a try to the below and let us know how it works for you?

pip install git+https://github.com/mwouts/jupytext.git@test_jupyterfs_metamanager

@mwouts
Copy link
Owner

mwouts commented Sep 28, 2020

@joequant when using this version I see a warning: 'dict' object has no attribute 'metadata' when I save paired notebooks. I'll see how to reproduce this and fix it with a test... stay tuned!

@joequant
Copy link
Author

FYI. Here is there current test harness. It doesn't quite work. Part of it is that when it runs notebook model, metamanager is missing a call to validate the notebook. also I'm trying to figure out how to initialize the metamanager

def test_config_jupytext_jupyter_fs_manager(tmpdir):
    """Test the configuration of Jupytext with a fs manager"""
    try:
        from jupyterfs.metamanager import MetaManager
    except ImportError:
        pytest.skip("jupyterfs is not available")

    import logging
    logger = logging.getLogger('example_logger')
    tmpdir.join("jupytext.toml").write('default_jupytext_formats = "ipynb,py"')

    cm_class = jupytext.build_jupytext_contents_manager_class(MetaManager)
    cm = cm_class(parent=None,log=logger)
    cm.initResource()
    cm.default_jupytext_formats = ""

    # save a few files                                                                                                                       
    nb = new_notebook()
    cm.save(dict(type="file", content="text", format="text"), path="text.md")
    cm.save(dict(type="notebook", content=nb), "script.py")
    cm.save(dict(type="notebook", content=nb), "text_notebook.md")
    cm.save(dict(type="notebook", content=nb), "notebook.ipynb")

    cm.get("notebook.ipynb")

@mwouts
Copy link
Owner

mwouts commented Sep 29, 2020

Oh, sorry, I should have let you know that now I am quite happy with the test involving the MetaManager, see here:

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",
}

@mwouts
Copy link
Owner

mwouts commented Oct 6, 2020

@joequant , @telamonian , I have prepared a release candidate with the fix:

pip install jupytext==1.7.0rc0

Would you mind giving it a try and confirm that it works as expected for you? Thanks!

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