serializing nested ntbk metadata items #147

choldgraf · 2020-04-08T22:52:48Z

I think that this is the only thing that we need to do in order to get nested metadata working.

edit: I think I need to edit some of the tests because the metadata in the output document will be different...I will try and figure out to update the regressions stuff so that it works (though somebody comment if that's not the problem here). @chrisjsewell are we OK with just updating the regression templates so that they now include the JSON-serialized nested metadata?

closes #147

chrisjsewell · 2020-04-08T22:59:08Z

Yep the failed tests show that the metadata is being correctly propagated to the sphinx env, you just need to update them!

choldgraf · 2020-04-08T23:08:52Z

OK I think I've got it fixed (in a fairly inelegant way, but 🤷‍♂ ). I had to update the pre-commit flake8 test because the nested JSON lines are too long, but other than that tests were passing

choldgraf · 2020-04-08T23:10:09Z

tests/test_parser.py

+            "ipub": '{"bibliography": "example.bib", "biboptions": ["super", "sort"], "bibstyle": "unsrtnat", "language": "portuges", "listcode": true, "listfigures": true, "listtables": true, "titlepage": {"author": "Authors Name", "email": "authors@email.com", "institution": ["Institution1", "Institution2"], "logo": "logo_example.png", "subtitle": "Sub-Title", "supervisors": ["First Supervisor", "Second Supervisor"], "tagline": "A tagline for the report.", "title": "Main-Title"}, "toc": {"depth": 2}, "pandoc": {"use_numref": true, "at_notation": true}, "sphinx": {"bib_title": "My Bibliography"}}',
+            "jupytext": '{"metadata_filter": {"notebook": "ipub"}}',
+            "kernelspec": '{"display_name": "Python 3", "language": "python", "name": "python3"}',
+            "language_info": '{"codemirror_mode": {"name": "ipython", "version": 3}, "file_extension": ".py", "mimetype": "text/x-python", "name": "python", "nbconvert_exporter": "python", "pygments_lexer": "ipython3", "version": "3.6.2"}',


@chrisjsewell is there any reason for us to think any of this stuff will be brittle and break over time? (e.g. as libraries update etc?)

ugh I think the answer is yes, because the tests are erroring because the version of jupytext they've got is different from the one on my machine. hmmmmm

chrisjsewell · 2020-04-08T23:11:53Z

.pre-commit-config.yaml

+      tests/test_parser.py|
+      tests/test_text_based.py


Lazy 😜 ! Add # noqa: E501 to specific lines that are too long, not ignore the whole file

ahhh that's right, I knew there was a way to do that

chrisjsewell · 2020-04-08T23:12:40Z

tests/test_parser.py

@@ -23,7 +29,18 @@ def test_complex_outputs(sphinx_run, file_regression):
    # print(sphinx_run.status())
    assert sphinx_run.warnings() == ""
    assert sphinx_run.app.env.metadata == {


as this is pretty long now, probably easier to add a data_regression.check

same with the others

ok this is a good opportunity to learn about data_regression.check. I guess that we will just have to update the regression templates any time that the jupytext version is updated

Yeh, or if it's going to change to often, just do as a minimum assert set(sphinx_run.app.env.metadata.keys()) == {"celltoolbar", ...}

choldgraf · 2020-04-08T23:20:04Z

@chrisjsewell where does the jupytext dependency get defined? I didn't see it in there, but I think it's needed for myst text reading, no? Are we inheriting it from some other package? And if so then should I just add it in explicitly?

edit: oh weird, we're not using jupytext in the parsing, but the file metadata is still different somehow...

I am thinking this might be more sensible if we just check to make sure the keys are always the same, as well as the values of a few select keys

chrisjsewell · 2020-04-08T23:38:29Z

where does the jupytext dependency get defined? I didn't see it in there, but I think it's needed for myst text reading, no? Are we inheriting it from some other package? And if so then should I just add it in explicitly?

There's (at least currently) no specific dependency of myst-nb or jupytext, since the relevant code from jupytext is essentially copied into myst_nb/convert. It's an open question if, now that jupytext 1.4.2 is released, we delete this code and import from jupytext.

The versions you see in the notebook/myst-markdown metadata, are/were generated during jupytext --to myst notebook.ipynb or when you pair the notebook in jupyter notebook/lab.
They don't change dynamically and are not strictly required for reading in the current code (jupytext uses them to checks for compatible versions: https://github.com/mwouts/jupytext/blob/b8693695640efa0659d91ab6f5a2524ca9f75471/jupytext/formats.py#L365). So yeh they are static and won't change if dependencies change

choldgraf · 2020-04-08T23:45:35Z

OK take a look at the latest testing suite. I'm checking for all the keys in the metadata, as well as for the proper values for a few keys, including both "flat" and "nested" values. Think that works?

choldgraf · 2020-04-08T23:47:09Z

tests/test_parser.py

+    assert sphinx_run.app.env.metadata["basic_run"]["test_name"] == "notebook1"
+    assert (
+        sphinx_run.app.env.metadata["basic_run"]["kernelspec"]
+        == '{"display_name": "Python 3", "language": "python", "name": "python3"}'


note - black did this, and it seemed reasonable + removed the line too long issues, so I went with it

codecov · 2020-04-08T23:47:13Z

Codecov Report

Merging #147 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #147   +/-   ##
=======================================
  Coverage   83.85%   83.85%           
=======================================
  Files           9        9           
  Lines         805      805           
=======================================
  Hits          675      675           
  Misses        130      130

Flag	Coverage Δ
#pytests	`83.85% <ø> (ø)`

Impacted Files	Coverage Δ
myst_nb/parser.py	`91.15% <ø> (ø)`

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 e77e33c...c75beb1. Read the comment docs.

choldgraf · 2020-04-08T23:48:52Z

finally

chrisjsewell · 2020-04-08T23:50:20Z

Yep looks fine to me 👍

chrisjsewell · 2020-04-08T23:59:11Z

I don't think you realize how easy you have it here lol. For example, I just has this PR merged aiidateam/aiida-core#3613 after months of having to address revisions from 3 reviewers and a lot more stringent CI checks 😬

choldgraf · 2020-04-09T03:42:22Z

haha - my worst was in switching over the matplotlib documentation entirely to use sphinx-gallery. It took 7 months and more bikeshedding than I have ever experienced in my life :-) I also had this MNE-python PR that had 268 comments over 3 months before it finally got merged.

* serializing nested ntbk metadata items * updating tests for nested metadata

serializing nested ntbk metadata items

aec986b

choldgraf commented Apr 8, 2020

View reviewed changes

chrisjsewell requested changes Apr 8, 2020

View reviewed changes

choldgraf force-pushed the ntbk_metadata branch from 4847350 to 6ed8c7c Compare April 8, 2020 23:44

updating tests for nested metadata

c75beb1

choldgraf force-pushed the ntbk_metadata branch from 6ed8c7c to c75beb1 Compare April 8, 2020 23:46

choldgraf commented Apr 8, 2020

View reviewed changes

choldgraf merged commit 4d57fd8 into master Apr 8, 2020

choldgraf deleted the ntbk_metadata branch April 8, 2020 23:54

phaustin pushed a commit to phaustin/MyST-NB that referenced this pull request Apr 10, 2020

serializing nested ntbk metadata items (executablebooks#147)

90377b6

* serializing nested ntbk metadata items * updating tests for nested metadata

choldgraf mentioned this pull request Apr 17, 2020

Embed the notebook kernel information in page metadata #142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

serializing nested ntbk metadata items #147

serializing nested ntbk metadata items #147

choldgraf commented Apr 8, 2020 •

edited

chrisjsewell commented Apr 8, 2020

choldgraf commented Apr 8, 2020

choldgraf Apr 8, 2020

choldgraf Apr 8, 2020

chrisjsewell Apr 8, 2020

choldgraf Apr 8, 2020

chrisjsewell Apr 8, 2020

chrisjsewell Apr 8, 2020

choldgraf Apr 8, 2020

chrisjsewell Apr 8, 2020

choldgraf commented Apr 8, 2020 •

edited

chrisjsewell commented Apr 8, 2020

choldgraf commented Apr 8, 2020

choldgraf Apr 8, 2020

codecov bot commented Apr 8, 2020 •

edited

choldgraf commented Apr 8, 2020 •

edited

chrisjsewell commented Apr 8, 2020

chrisjsewell commented Apr 8, 2020

choldgraf commented Apr 9, 2020 •

edited

serializing nested ntbk metadata items #147

serializing nested ntbk metadata items #147

Conversation

choldgraf commented Apr 8, 2020 • edited

chrisjsewell commented Apr 8, 2020

choldgraf commented Apr 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choldgraf commented Apr 8, 2020 • edited

chrisjsewell commented Apr 8, 2020

choldgraf commented Apr 8, 2020

Choose a reason for hiding this comment

codecov bot commented Apr 8, 2020 • edited

Codecov Report

choldgraf commented Apr 8, 2020 • edited

chrisjsewell commented Apr 8, 2020

chrisjsewell commented Apr 8, 2020

choldgraf commented Apr 9, 2020 • edited

choldgraf commented Apr 8, 2020 •

edited

choldgraf commented Apr 8, 2020 •

edited

codecov bot commented Apr 8, 2020 •

edited

choldgraf commented Apr 8, 2020 •

edited

choldgraf commented Apr 9, 2020 •

edited