quarto-dev / quarto-cli

Open-source scientific and technical publishing system built on Pandoc.
https://quarto.org
Other
3.78k stars 309 forks source link

Code cells of different languages are wrongly merged when inside a layout #8179

Open mcanouil opened 8 months ago

mcanouil commented 8 months ago

Code cells of different languages are wrongly merged when inside a layout (independently of knitr/Jupyter being the engine)

Quarto documentv1.3.450v1.4
````qmd --- title: "Block layout" format: html engine: knitr --- # Code chunk block layout :::: {layout="[48,-4,48]"} ::: {} ### R ```{r} sqrt(2) ``` ::: ::: {} ### Python ```{python} import math math.sqrt(2) ``` ::: :::: ```` image image

Discussed in https://github.com/quarto-dev/quarto-cli/discussions/8178

Originally posted by **luifrancgom** January 9, 2024 ### Description I want to display code chunks side by side where I try the solution specify in #3423 and the instructions pointed out in the documentation [here](https://quarto.org/docs/authoring/figures.html#block-layout) but I did not obtain the expected behavior in relation to the code. The solution only works for the result of the code. ### Steps to reproduce ````qmd --- title: "Block layout" format: html: default pdf: default editor: visual --- # Code chunk block layout :::: {layout="[48,-4,48]"} ::: {} ### R ```{r} sqrt(2) ``` ::: ::: {} ### Python ```{python} import math math.sqrt(2) ``` ::: :::: ```` ### Expected behavior ![expected_behavior](https://github.com/quarto-dev/quarto-cli/assets/50002526/a639d2e8-6051-4723-b010-8917147ff679) ### Actual behavior ![actual_behavior](https://github.com/quarto-dev/quarto-cli/assets/50002526/80ed2b29-1ef4-434b-b49f-b6c85238409e) ### Question How can I obtain the expected behavior? ### Quarto and dependencies I am running ```bash Quarto 1.4.537 [>] Checking versions of quarto binary dependencies... Pandoc version 3.1.11: OK Dart Sass version 1.69.5: OK Deno version 1.37.2: OK [>] Checking versions of quarto dependencies......OK [>] Checking Quarto installation......OK Version: 1.4.537 Path: C:\Users\Usuario\AppData\Local\Programs\Quarto\bin CodePage: 1252 [>] Checking tools....................OK TinyTeX: v2024.01 Chromium: 869685 [>] Checking LaTeX....................OK Using: TinyTex Path: C:\Users\Usuario\AppData\Roaming\TinyTeX\bin\windows\ Version: 2023 [>] Checking basic markdown render....OK [>] Checking Python 3 installation....OK Version: 3.11.6 (Conda) Path: C:/Users/Usuario/anaconda3/python.exe Jupyter: 5.7.0 Kernels: ir, julia-1.9, python3 [>] Checking Jupyter engine render....OK [>] Checking R installation...........OK Version: 4.3.2 Path: C:/PROGRA~1/R/R-43~1.2 LibPaths: - C:/Users/Usuario/AppData/Local/R/win-library/4.3 - C:/Program Files/R/R-4.3.2/library knitr: 1.45 rmarkdown: 2.25 [>] Checking Knitr engine render......OK ```
cderv commented 8 months ago

FWIW this is the intermediate .md

# Code chunk block layout

:::: {layout="[48,-4,48]"}

::: {}

### R

::: {.cell}

```{.r .cell-code}
sqrt(2)

::: {.cell-output .cell-output-stdout}

[1] 1.414214

::: :::

:::

::: {}

Python

::: {.cell}

import math
math.sqrt(2)

::: {.cell-output .cell-output-stdout}

1.4142135623730951

::: :::

:::

::::


Also this is not related to **knitr** as this happens with Jupyter engine too 

````markdown
---
title: "Block layout"
format: html
keep-md: true
---

# Code chunk block layout

:::: {layout="[48,-4,48]"}

::: {}

### Python

```{python}
import math
math.sqrt(4)

:::

::: {}

Python

import math
math.sqrt(2)

:::

::::


![image](https://github.com/quarto-dev/quarto-cli/assets/6791940/3972fa57-c807-49a4-87c6-10a040603f94)

So there seems to be indeed an issue with cell merging. Example can be simplified with 
````yaml
execute: 
  eval: false
cderv commented 8 months ago

This is the change that created the regression c4ab028a479ebb6834e1394b4ac223b441b3b851 from

I did a git bisect to confirm

mcanouil commented 8 months ago

@cderv thanks for the bisect (4am is not a good time to do it^^)

cderv commented 8 months ago

No need to justify. And no need to do triaging at 4AM too. 😉

mcanouil commented 8 months ago

@cderv Well, it was no time wasted 😉

cscheid commented 8 months ago

Let me start by the first comment. The "expected" vs "actual" behavior goes the opposite way. We've always wanted to merge the code from layout cells but weren't doing it in all situations. This is now working as expected, as isn't a bug.

cscheid commented 8 months ago

We might want to add an option to not to code hoisting, but this isn't a bug.

mcanouil commented 8 months ago

Well, the two code cells are of different languages. Only code cells sharing the same language should be merged at least. Currently, no matter the code cells languages, those are merged.

cscheid commented 8 months ago

I see. We need to edit the issue title and description.

cscheid commented 8 months ago

I'm honestly reluctant to change the behavior at this point in the release because that code is pretty fragile.

cderv commented 8 months ago

The "expected" vs "actual" behavior goes the opposite way. We've always wanted to merge the code from layout cells but weren't doing it in all situations. This is now working as expected, as isn't a bug.

I was not expecting this. I am probably missing the use case where merging two different code cells is required ... 🤔

If indeed we should not merged two cells of different languages, example given in my previous answer (https://github.com/quarto-dev/quarto-cli/issues/8179#issuecomment-1882663591) is mono language python, and it still feels very surprising to have the headers moved in column layout while code source cell is kept outside of columns and merged.

Example with no merging about this "hoisting" outside

---
format: html
---

# two columns

:::: {layout="[ 40, 60 ]"}

::: {#first-column}
```{r}
1 + 1

Some text that should be laid out below the code :::

::: {#second-column} :::

::::



![image](https://github.com/quarto-dev/quarto-cli/assets/6791940/4fd00d61-6009-4258-ae4d-cc7ad2b20c8a)

I don't think we have any way to put code cells in layout now. 

It seems to me we want to merge code cells when using in-cell options for layout (https://quarto.org/docs/authoring/figures.html#computations) , as this applies to the cell outputs. In that case currently, `layout*` attributes is put onon the `.cell` div in the intermediate markdown. This is different to me than wrapping in a div of attribute `layout`. 

Feels like a breaking change for 1.4 if this syntax was used in the while to layout in columns some cell code + output content. Let's see if we have more report with RC out.
mcanouil commented 8 months ago

I do agree with the breaking change/regression part of this. If it stays that way, it should really be emphasise.

mcanouil commented 8 months ago

What I am afraid of with this is for all users using the layout in Reveal.js for educational purposes. It's no longer usable.

Quarto documentHTML
````qmd --- title: "Block layout" format: revealjs engine: knitr execute: echo: true --- ## Code chunk block layout :::: {layout="[48,-4,48]"} ::: {#r-code1} ### R ```{r} sqrt(2) ``` ::: ::: {#python-code1} ### Python ```{python} import math math.sqrt(2) ``` ::: :::: ```` image
cscheid commented 8 months ago

I am probably missing the use case where merging two different code cells is required ... 🤔

This is a pretty clear case where merging is a good thing. The merging is not going away, I'm sorry. This has been a feature since 1.0.

We should do better in multi-language cases.

I believe that the revealjs case can be solved by using the revealjs columns feature instead of the panel layout.

mcanouil commented 8 months ago

Indeed, using columns/column does not merge the cells.

image

By the way, where this merging behaviour is documented? I cannot find any trace of this.

cscheid commented 8 months ago

By the way, where this merging behaviour is documented? I cannot find any trace of this.

This is an example of it in use - https://quarto.org/docs/authoring/figures.html#custom-layout

cscheid commented 8 months ago

Indeed, using columns/column does not merge the cells.

And this works in other formats as well (I don't love the padding but you see that the idea is sound)

---
title: columns
---

:::: {.columns}

::: {.column width="50%"}

```{r}
plot(1:10)

:::

::: {.column width="50%"}

from matplotlib import pyplot as plt
plt.plot([1,2,3,4])

:::

::::



<img width="711" alt="image" src="https://github.com/quarto-dev/quarto-cli/assets/285675/9c3c1d32-ec82-40d5-89ab-c915a989b846">

I do believe the issues are:

1. we shouldn't merge multiple-language cells in a single layout. But that's:
   - currently not trivial to detect and fix in the Lua filters, and layouts are a regression-prone part of the code
   - relatively rare (multi-language is not a common thing)
   - fixable with .columns  

   So we ought to do it in 1.5.
2. The layouts that are not being computed _are_ broken. We ought to do it in 1.4.
mcanouil commented 8 months ago

This merging behaviour needs to be better described, maybe with:

cscheid commented 8 months ago

I think the principle we should be obeying (which, to be clear, we currently are not obeying) is that when echo: true, code cells in the output should look as they do on the input. Specifically, there should exactly as many output code cells as there are input code cells. knitr breaks those apart, which requires us to do that merging. Quarto 1.3 didn't merge all of them successfully (although I can't seem to find an example of this now). But Quarto 1.4 is currently merging them too aggressively.

Again, this is something I would argue for a fix in 1.5, but I think it's good if we can agree on the reason.

mcanouil commented 8 months ago

I would go for 1.5 maybe an "early-in-release" mostly because:

  1. That's the first report
  2. "Columns" is nearly the only layout option appearing in search in the documentation so I wouldn't expect this to impact a lot of users.
  3. I am starting to think any changes regarding this would be at the cost of postponing further away the 1.4 release which is kind of a 2.0 actually 😅