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

include shortcode in computation code cell are not resolved for julia engine #10008

Closed
cderv opened this issue Jun 14, 2024 · 19 comments · Fixed by PumasAI/QuartoNotebookRunner.jl#154 or #10110
Assignees
Labels
enhancement New feature or request includes julia
Milestone

Comments

@cderv
Copy link
Collaborator

cderv commented Jun 14, 2024

Following up on #9984

Sorry, but I think I was confused by all the different things I tried.
Actually there seems to be a bug when using includes in the julia engine executable code block.

test.qmd

---
title: test
engine: julia
format:
  gfm: default
---

````{julia}
#| eval: false
{{< include foo.jl >}}
````

foo.jl

"""
```python
1 + 1
```
"""

output:

# test


``` julia
"""
```python
1 + 1
```

““”

\`\`\`

If it is not an executable code block, it works fine.

# test


```` julia
#| eval: false
"""
```python
1 + 1
```
"""
````

So I suppose this is a bug of QuartoNotebookRunner.jl?
(The fact that the number of backticks varies from engine to engine was quite an unexpected behavior for me, though.)

Originally posted by @eitsupi in #9984 (comment)

@cderv
Copy link
Collaborator Author

cderv commented Jun 14, 2024

I believe this is a cell source output backticks problem. Here is the intermediate outputs:

---
title: test
engine: julia
keep-md: true
format:
  gfm: default
---


::: {.cell execution_count=0}
``` {.julia .cell-code}
"""
```python
1 + 1
```
"""

```
:::

We can see the the fenced code cell for source output does use only ```` backticks when initially it was 4.

I guess not all engine handle nested backticks for code cells the same.

I know knitr handles it for R by computing the number of backticks. No problem with engine: knitr

and it seems for Jupyter engine we do handle it also as I can't reproduce with this

---
title: test
keep-md: true
format:
  gfm: default
---

````{python}
#| eval: false
{{< include foo.jl >}}
````

So this is linked to new Julia engine that needs to do the same as with Jupyter engine. It may be Quarto and not QuartoNotebookRunner.jl directly.

@cderv cderv added the julia label Jun 14, 2024
@cderv
Copy link
Collaborator Author

cderv commented Jun 14, 2024

Yes we do count backticks at

const fenced = echoFenced(cell, options);
const ticks = "`".repeat(
Math.max(countTicks(jupyterCellSrcAsLines(cell)) + 1, fenced ? 4 : 3),
);
md.push(ticks + " {");

@cderv
Copy link
Collaborator Author

cderv commented Jun 14, 2024

It seems this issue does relate to shortcode because this has no issues

---
title: test
engine: julia
keep-md: true
format:
  gfm: default
---

````{julia}
#| eval: false
"""
```python
1 + 1
```
"""
````

and produced correct intermediate md

---
title: test
engine: julia
keep-md: true
format:
  gfm: default
---


::: {.cell execution_count=0}
```` {.julia .cell-code}
"""
```python
1 + 1
```
"""
````
:::

So just maybe something about timing for resolving shortcodes ... 🤔

@eitsupi
Copy link
Contributor

eitsupi commented Jun 14, 2024

Thank you for your investigation!
The issue seems to be more complicated than I imagined...

@cderv cderv self-assigned this Jun 14, 2024
@cderv
Copy link
Collaborator Author

cderv commented Jun 14, 2024

OK so looking more into it, I do think this is not supported at the moment.

@jkrumbiegel @cscheid as you were the one working on it in

It seems that Julia engine will use the input source file

const execOptions = {
...options,
target: {
...options.target,
input: normalizePath(options.target.input),
},
};

However, our internal processing does create a processed Markdown version of a .qmd file to resolve different things. And this includes shortcode. We resolves includes (more generally cell handler) in a pre-engine phase.

if (!isJupyterNotebook(context.target.source)) {
// this is not a jupyter notebook input,
// so we can run pre-engine handlers
const preEngineCellHandlerOptions: LanguageCellHandlerOptions = {
name: "", // this gets filled out by handleLanguageCells later.
temp: options.services.temp,
format: context.format,
markdown: context.target.markdown,
context,
flags: options.flags || {} as RenderFlags,
stage: "pre-engine",
};
const { markdown, results } = await handleLanguageCells(
preEngineCellHandlerOptions,
);
context.target.markdown = markdown;
if (results) {
context.target.preEngineExecuteResults = results;
}
}

context.target.markdown is what will contains the resolved Markdown string that should be used for engine execution.

This transformed markdown is used in several places like engine resolution

// if we were passed a transformed markdown, use that for the text instead
// of the contents of the file.
if (kMdExtensions.includes(ext) || kQmdExtensions.includes(ext)) {
const markdown = await project.resolveFullMarkdownForFile(undefined, file);
// https://github.com/quarto-dev/quarto-cli/issues/6825
// In case the YAML _parsing_ fails, we need to annotate the error
// with the filename so that the user knows which file is the problem.
try {
return markdownExecutionEngine(
markdown ? markdown.value : Deno.readTextFileSync(file),
flags,
);
} catch (error) {
if (error.name === "YAMLError") {
error.message = `${file}:\n${error.message}`;
}
throw error;
}
} else {
return undefined;
}

and used in engine execution. For example for Jupyter engine, we create a .ipynb from a .qmd

execute: async (options: ExecuteOptions): Promise<ExecuteResult> => {
// create the target input if we need to (could have been removed
// by the cleanup step of another render in this invocation)
if (
(isQmdFile(options.target.source) ||
isJupyterPercentScript(options.target.source)) &&
!existsSync(options.target.input)
) {
await createNotebookforTarget(options.target);
}

and we use this target.markdown.value and not the input .qmd file
async function createNotebookforTarget(
target: ExecutionTarget,
project?: ProjectContext,
) {
const nb = await quartoMdToJupyter(target.markdown.value, true, project);
Deno.writeTextFileSync(target.input, JSON.stringify(nb, null, 2));
return nb;
}

So I believe julia engine should also use this markdown value for its engine execution. It should somehow be passed to QuartoNotebookRunner.jl

  • Either with an intermediate file
    • like we do for the notebook creation where we create a .quarto_ipynb intermediate for jupyter engine
      const notebook = join(fileDir, fileStem + ".quarto_ipynb");
      const target = {
      source: file,
      input: notebook,
      markdown: markdown!,
      metadata,
      data: { transient: true, kernelspec: {} },
      };
      nb = await createNotebookforTarget(target, project);
      target.data.kernelspec = nb.metadata.kernelspec;
      return target;
    • or with knitr engine where the markdown content from Quarto is passed to R and written to a file
      const result = await callR<ExecuteResult>(
      "execute",
      {
      ...options,
      target: undefined,
      input: options.target.input,
      markdown: resolveInlineExecute(options.target.markdown.value),
      },

      execute <- function(input, format, tempDir, libDir, dependencies, cwd, params, resourceDir, handledLanguages, markdown) {
      # calculate knit_root_dir (before we setwd below)
      knit_root_dir <- if (!is.null(cwd)) tools::file_path_as_absolute(cwd) else NULL
      # change to input dir and make input relative (matches
      # behavior/expectations of rmarkdown::render code)
      oldwd <- setwd(dirname(rmarkdown:::abs_path(input)))
      on.exit(setwd(oldwd), add = TRUE)
      input <- basename(input)
      # rmd input filename
      rmd_input <- paste0(xfun::sans_ext(input), ".rmarkdown")
      # swap out the input by reading then writing content.
      # This handles `\r\n` EOL on windows in `markdown` string
      # by spliting in lines
      xfun::write_utf8(
      xfun::read_utf8(textConnection(markdown, encoding = "UTF-8")),
      rmd_input
      )
      input <- rmd_input

I don't know if QuartoNotebookRunner.jl requires only a file a input, or could work also with a markdown passed a string.

but the same logic would need to be added.

Also I do think this would solve our embed support, which rely also on this transformed markdown content as it is in the handler category;

Hope this helps know what to do exactly, either in Quarto code base, or in QuartoNotebookRunner.jl

@cderv cderv added includes enhancement New feature or request triaged-to Issues that were not self-assigned, signals that an issue was assigned to someone. labels Jun 14, 2024
@cderv cderv removed their assignment Jun 14, 2024
@cderv cderv changed the title include shortcode in Julia code cell using julia engine has backticks problems include shortcode in computation code cell are not resolved for julia engine Jun 14, 2024
@jkrumbiegel
Copy link
Contributor

Probably I just didn't know that code for this had to be added, I built the julia engine code by copying and editing jupyter engine pieces until all that I needed worked. But I never tested shortcodes.

QuartoNotebookRunner takes a filepath as input, so we'd have to write out a temporary file I think.

@cderv
Copy link
Collaborator Author

cderv commented Jun 14, 2024

QuartoNotebookRunner takes a filepath as input, so we'd have to write out a temporary file I think.

I think that could be possible to write an intermediate file that would be removed after execution.

Is this is strong requirement that QuartoNotebookRunner takes a file ?

Asking because it could either be Quarto julia engine code writing it and removing it (like with Jupyter engine) , or otherwise this would be the QuartoNotebookRunner.jl side if we pass the information encoded as Json string (like with the knitr engine). It could even be base64 encode to be decode on Julia side for easier exchange of information.

Not sure what is best.

Do you want to work on this and send a PR for review ? Or should we add support for this (on the Quarto side in that case) ?

@jkrumbiegel
Copy link
Contributor

jkrumbiegel commented Jun 14, 2024

Let me pull in @MichaelHatherly. We already get the full options JSON object sent over from quarto to QuartoNotebookRunner, and I think this even contains the complete source as a string if I remember correctly. I assume some string in that object would change if shortcodes were executed correctly, and then we could pick it up from there?

@MichaelHatherly
Copy link
Contributor

We already get the full options JSON object sent over from quarto to QuartoNotebookRunner, and I think this even contains the complete source as a string if I remember correctly. I assume some string in that object would change if shortcodes were executed correctly, and then we could pick it up from there?

Yeah, we could just parse from that string in the JSON blob if it's available, otherwise fallback to the file.

@cderv
Copy link
Collaborator Author

cderv commented Jun 14, 2024

Let me pull in @MichaelHatherly. We already get the full options JSON object sent over from quarto to QuartoNotebookRunner, and I think this even contains the complete source as a string if I remember correctly. I assume some string in that object would change if shortcodes were executed correctly, and then we could pick it up from there?

Yes should have the target.markdown in there already on Quarto side. This is the markdown string where shortcodes like include and embed would have been expanded already. So if you pass everything to the Julia process already, you probably have access to it already.

Yeah, we could just parse from that string in the JSON blob if it's available, otherwise fallback to the file.

AFAIK target.markdown should always be set to the content of the file we read for all our processing.

@eitsupi
Copy link
Contributor

eitsupi commented Jun 27, 2024

@jkrumbiegel Thanks for the quick update, but I tried QuartoNotebookRunner 0.11.2 and it does not seem fixed.

@jkrumbiegel
Copy link
Contributor

@eitsupi do you have an MWE? QuartoNotebookRunner just uses the preprocessed markdown from quarto now if it's available, so the question would be what side a possible bug would be on.

@eitsupi
Copy link
Contributor

eitsupi commented Jun 27, 2024

@jkrumbiegel Thanks for your quick reply. What does "MWE" mean?
The reproducible example in the first post of this issue does not work.
I am not sure if this is a Quarto CLI issue or a QuartoNotebookRunner issue, sorry. (I thought that a fix on the engine side only would fix this, but I could be mistaken.)

@mcanouil
Copy link
Collaborator

MWE: Minimal Working Example

@jkrumbiegel
Copy link
Contributor

This is the output I get from the first post here, on QNB 0.11.2, with the latest prerelease 1.5.49. Seems correct to me?

# test


```` julia
"""
```python
1 + 1
```
"""
````

@eitsupi
Copy link
Contributor

eitsupi commented Jun 27, 2024

Thanks for confirming!
I just reinstalled Quarto and Julia and redid everything and confirmed it works (not sure why it failed last time even though I updated Manifest.toml and remove .quarto directory).
I think this issue can be closed.

@jkrumbiegel
Copy link
Contributor

Could be that an old quarto Julia server process was still running and it picked up that one after updating, so you only thought you'd tested on the correct QNB version. This is something I still want to improve, ideally the server would restart when a new QNB version is detected.

@cderv cderv linked a pull request Jun 27, 2024 that will close this issue
@cderv
Copy link
Collaborator Author

cderv commented Jun 27, 2024

Closed by #10110 following update of QuartoNotebookRunner

@cderv cderv closed this as completed Jun 27, 2024
@cderv cderv modified the milestones: v1.6, v1.5 Jun 27, 2024
@cderv cderv removed the triaged-to Issues that were not self-assigned, signals that an issue was assigned to someone. label Jun 27, 2024
@mcanouil
Copy link
Collaborator

Could be that an old quarto Julia server process was still running.

This indeed happens and is really hard to figure out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request includes julia
Projects
None yet
6 participants