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's CM uses parent CM's get and save methods to read text files #634

Merged
merged 7 commits into from
Sep 30, 2020

Conversation

mwouts
Copy link
Owner

@mwouts mwouts commented Sep 27, 2020

With this PR we stop using mock to replace the nbformat.reads and writes functions with Jupytext's ones. Instead we use the parent contents manager to load/write the text files and parse them with Jupytext.

As a side effect, we don't use any more private methods from the FileContentsManager or LargeFileManager, and Jupytext can work on top of other contents managers like Jupyter-FS.

This is a follow-up on #621 and will hopefully close #618

@codecov
Copy link

codecov bot commented Sep 27, 2020

Codecov Report

Merging #634 into master will increase coverage by 0.00%.
The diff coverage is 98.91%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #634   +/-   ##
=======================================
  Coverage   99.18%   99.18%           
=======================================
  Files          94       94           
  Lines        9307     9359   +52     
=======================================
+ Hits         9231     9283   +52     
  Misses         76       76           
Impacted Files Coverage Δ
tests/test_trust_notebook.py 98.98% <94.73%> (-1.02%) ⬇️
jupytext/contentsmanager.py 98.51% <98.07%> (+0.06%) ⬆️
tests/test_cli.py 100.00% <100.00%> (ø)
tests/test_cm_config.py 100.00% <100.00%> (ø)
tests/test_contentsmanager.py 100.00% <100.00%> (ø)
tests/test_escape_magics.py 100.00% <100.00%> (ø)
tests/test_read_simple_percent.py 100.00% <100.00%> (ø)
tests/test_save_multiple.py 100.00% <100.00%> (ø)
tests/utils.py 100.00% <100.00%> (ø)
jupytext/config.py 97.51% <0.00%> (+0.62%) ⬆️

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 aea84d9...5a21182. Read the comment docs.

@mwouts
Copy link
Owner Author

mwouts commented Sep 28, 2020

With this version I see a warning: 'dict' object has no attribute 'metadata' when I save paired notebooks.

@telamonian
Copy link
Contributor

@mwouts I'll take a look

@mwouts
Copy link
Owner Author

mwouts commented Sep 29, 2020

Thank you @telamonian , well sure it is a great idea if you want to have a look at this. With this PR the contents manager will be much cleaner - the implementation in Jupytext <= 1.6.0 works well in practice but is a bit hacky!

The tests on this PR pass but as mentioned above the contents manager does not work in the real world. I am a bit surprised that the tests don't catch this... I'll see tonight if I can reproduce the problem in the test framework, and report here.

@telamonian
Copy link
Contributor

telamonian commented Sep 29, 2020

@mwouts I tracked the problem down and fixed it. It actually seems like jupyter-fs and jupytext are working together well now. All the details are at #635.

I'm trying to put together some tutorials for how to debug Jupyter-related code. So as an experiment/rough draft I filmed myself working on this bug and uploaded it to youtube.

If you have a sec, check it out

@joequant
Copy link

Great job!!!!

Is there any place we can start a wiki to talk about debugging and testing techniques?

The tools that I use are Firefox developer and KernelSpy, and I find that a lot of bugs can be tracked down by just running these two programs and seeing where you have error messages, or weird behaviour.

Also, I have some hints as to guessing where the bugs often are.

@mwouts mwouts force-pushed the test_jupyterfs_metamanager branch from a63f59d to 5a21182 Compare September 30, 2020 05:17
@mwouts mwouts merged commit 4b89f60 into master Sep 30, 2020
@mwouts mwouts deleted the test_jupyterfs_metamanager branch September 30, 2020 05:29
@mwouts
Copy link
Owner Author

mwouts commented Sep 30, 2020

Hi Max, just wanted to thank you for spending time on this project. I've watched the two videos, they are very interesting! Nice way to spend time together 😄

A few quick comments:

  • I'm sure I will reuse your settings from the first one next time I have to debug the TypeScript extension 😄 Currently for Python I am very happy with PyCharm, but for the JavaScript / TypeScript part I was missing a better development environment than the browser...
  • Seeing you scroll among the code and the latest commits is a great incentive to write clearer code and commits !
  • Oh yes, using mock was a hack, sorry about this... I was just learning Python (and pytest, hence mock...) at that time.... and it worked too well - I mean it took two years before a real limitation appeared... But of course I'm glad to have this better implementation now.
  • Just like you, I love interactive debugging, but maybe I am even more fond of the tests - In my experience they help ensure that I won't need to debug another time. What do you think?

Looking forwards to listening to your talk at the next JupyterCon 👋

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

Successfully merging this pull request may close these issues.

Jupytext manager seems to fail with jupyterfs
3 participants