nbdev icon indicating copy to clipboard operation
nbdev copied to clipboard

nbdev breaks subcaptions on figures

Open muellerzr opened this issue 3 years ago • 7 comments

See the example here, Quarto can do something like the following:

#| label: fig-charts
#| fig-cap: Charts
#| fig-subcap: 
#|   - "First"
#|   - "Second"
#| layout-ncol: 2

import matplotlib.pyplot as plt
plt.plot([1,23,2,4])
plt.show()

plt.plot([8,65,23,90])
plt.show()

And render carts with captions: image

Currently this will break nbdev with the following trace:

Traceback (most recent call last):
  File "/home/zach/.local/lib/python3.8/site-packages/nbdev/quarto.py", line 278, in _f
    try: serve_drv.main(res)
  File "/home/zach/.local/lib/python3.8/site-packages/nbdev/serve_drv.py", line 22, in main
    if src.suffix=='.ipynb': exec_nb(src, dst, x)
  File "/home/zach/.local/lib/python3.8/site-packages/nbdev/serve_drv.py", line 16, in exec_nb
    cb()(nb)
  File "/home/zach/.local/lib/python3.8/site-packages/nbdev/processors.py", line 243, in __call__
    def __call__(self, nb): return self.nb_proc(nb).process()
  File "/home/zach/.local/lib/python3.8/site-packages/nbdev/process.py", line 126, in process
    for proc in self.procs: self._proc(proc)
  File "/home/zach/.local/lib/python3.8/site-packages/nbdev/process.py", line 119, in _proc
    for cell in self.nb.cells: self._process_cell(proc, cell)
  File "/home/zach/.local/lib/python3.8/site-packages/nbdev/process.py", line 104, in _process_cell
    if f in cell.directives_: self._process_comment(proc, cell, f)
  File "/home/zach/.local/lib/python3.8/site-packages/nbdev/process.py", line 115, in _process_comment
    return proc(cell, *args)
TypeError: __call__() takes 2 positional arguments but 3 were given

cc @seeM

muellerzr avatar Nov 12 '22 20:11 muellerzr

Oof, looks like Quarto directives support a more extended version of YAML these days. Our parser is super simple and doesn't understand "multi-line" directives like this. An immediate workaround would be to use single line syntax, for example I tested this locally and it works:

#| label: fig-charts
#| fig-cap: Charts
#| fig-subcap: ["First", "Second"]
#| layout-ncol: 2

import matplotlib.pyplot as plt
plt.plot([1,23,2,4])
plt.show()

plt.plot([8,65,23,90])
plt.show()

Note: #| fig-subcap: ["First", "Second"] isn't actually parsed correctly by nbdev, but it doesn't need to be. It just needs to not lead to an error so it can pass-through to Quarto.

Perhaps a decent short-term fix would be for nbdev to not raise on directive "syntax errors", but instead print a warning and move on, since nbdev doesn't need to understand Quarto's full syntax.

seeM avatar Nov 13 '22 06:11 seeM

@jph00 @hamelsmu, curious to hear your thoughts too.

seeM avatar Nov 13 '22 06:11 seeM

It might be annoying for Quarto-compliant users to have to see a warning every time they preview their blog.

Do you think there might be a way to parse the directives as YAML? We already have pyyaml as a dependency. Since #|export isn't valid yaml, we would have to pre-process that kind of thing first to turn things into key: value (with a space after the colon IIRC YAML is strict about the space). for example we might have to turn #|export into #|export: true behind the scenes.

The upshot of that approach is that it would give users a shortcut to all quarto directives as well; for example people could just write #| echo and it will get turned into #| echo: true by nbdev and passed along to quarto. This way, the behavior is consistent, and we can declare that we have enabled a "shortcut" for all cell directives even quarto ones.

WDYT about this?

hamelsmu avatar Nov 13 '22 18:11 hamelsmu

Message ID: @.***>Yeah I was wondering about that too. I think #|export actually is valid YAML though - it's a YAML string. The issue AFAICT is that Quarto expects everything to be a yaml dictionary.

jph00 avatar Nov 13 '22 20:11 jph00

@jph00 just out of curiosity, when I put the following into any YAML validator, like this one it errors:

foo
bar: baz

Similarly, in python this errors:

In [5]: import yaml

In [6]: yml = "foo\nbar: baz"

In [7]: yaml.safe_load(yml)

ScannerError: mapping values are not allowed here
  in "<unicode string>", line 2, column 4:
    bar: baz
       ^

But this does not

In [8]: yml = "foo: hello\nbar: baz"

In [9]: yaml.safe_load(yml)
Out[9]: {'foo': 'hello', 'bar': 'baz'}

What am I missing?

hamelsmu avatar Nov 13 '22 21:11 hamelsmu

I think the gap is the multi-line yaml stuff. so

#|export
#|echo: true

Would not be valid yaml if taken together, but line-by-line separately it would be. 🤔

hamelsmu avatar Nov 13 '22 21:11 hamelsmu

I think the gap is the multi-line yaml stuff. so

#|export #|echo: true Would not be valid yaml if taken together, but line-by-line separately it would be. 🤔

Yes exactly!

jph00 avatar Nov 13 '22 21:11 jph00