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

Handle application/pdf mimetypes #136

Merged
merged 1 commit into from
May 24, 2024
Merged

Handle application/pdf mimetypes #136

merged 1 commit into from
May 24, 2024

Conversation

MichaelHatherly
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 56.30%. Comparing base (224d278) to head (21bb813).

Files Patch % Lines
...NotebookWorker/ext/QuartoNotebookWorkerPlotsExt.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #136      +/-   ##
==========================================
+ Coverage   55.98%   56.30%   +0.32%     
==========================================
  Files          25       25              
  Lines        1145     1142       -3     
==========================================
+ Hits          641      643       +2     
+ Misses        504      499       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

else
Plots.gr(; size_kwargs..., fmt = fm.fig_format, dpi_kwargs...)
end
Plots.gr(; size_kwargs..., dpi_kwargs...)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format wasn't really relevant to pass here, since we call show on each valid mimetype rather than relying on the default format set here. The addition of application/pdf support seems to resolve this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's what I gathered from the linked Plots code as well. I'm not sure what backends would actually use pdf assets if we offer them, maybe latex-based ones.

@MichaelHatherly MichaelHatherly linked an issue May 23, 2024 that may be closed by this pull request
@MichaelHatherly MichaelHatherly marked this pull request as ready for review May 23, 2024 14:41
@MichaelHatherly
Copy link
Collaborator Author

@lrnv if you have a chance to confirm this resolves it for you that would be good.

To point quarto render at this branch you can

  • clone this branch
  • instantiate and precompile it
  • set QUARTO_JULIA_PROJECT environment variable to that path of the clone so that it uses that runner rather than the quarto-managed one
  • run quarto render file.qmd

@lrnv
Copy link

lrnv commented May 23, 2024

I was able to install QNR, dev it and move the deved verion to this branch, but not to tell quarto to use it. Could you guide me a bit more into the environement thing ? I tried using a _enrivonement file but that did not work.

@MichaelHatherly
Copy link
Collaborator Author

The below should hopefully work for you. QUARTO_JULIA_PROJECT=/Users/mike/personal/QuartoNotebookRunner.jl/ should be set to whatever path you cloned this repo to.

~/personal 
❯ git clone --branch mh/plots-pdf https://github.com/PumasAI/QuartoNotebookRunner.jl^C

~/personal 
❯ cd QuartoNotebookRunner.jl/

QuartoNotebookRunner.jl on  mh/plots-pdf [?] is 📦 v0.10.2 via ஃ v1.10.3 via ⨁ v1.5.39 took 4s 
❯ julia --project -e 'import Pkg; Pkg.instantiate(); Pkg.precompile()'

QuartoNotebookRunner.jl on  mh/plots-pdf [?] is 📦 v0.10.2 via ஃ v1.10.3 via ⨁ v1.5.39 
❯ QUARTO_JULIA_PROJECT=/Users/mike/personal/QuartoNotebookRunner.jl/ quarto render PATH/TO/FILE.qmd
Running [1/1] at line 17:  using Random, Plots
pandoc 
  to: beamer
  output-file: file.tex
  standalone: true
  pdf-engine: xelatex
  variables:
    graphics: true
    tables: true
  default-image-extension: pdf
  
metadata
  block-headings: true
  title: title
  julia:
    exeflags:
      - '--project=plots'
  

Rendering PDF
running xelatex - 1
  This is XeTeX, Version 3.141592653-2.6-0.999995 (TeX Live 2023) (preloaded format=xelatex)
   restricted \write18 enabled.
  entering extended mode
  
running xelatex - 2
  This is XeTeX, Version 3.141592653-2.6-0.999995 (TeX Live 2023) (preloaded format=xelatex)
   restricted \write18 enabled.
  entering extended mode
  

Output created: file.pdf

@lrnv
Copy link

lrnv commented May 23, 2024

i'm sorry I was still not able to reproduce my previous result, now it complains about not having Plots.jl in its environement although it is (in the local env of the qmd)..

@MichaelHatherly
Copy link
Collaborator Author

MichaelHatherly commented May 23, 2024

Ah, the file.qmd I was using had explicitly set the project, see https://github.com/PumasAI/QuartoNotebookRunner.jl/pull/136/files#diff-e002873e0c60e777197ce755872fc8a5d32041b7dd3b21062807548c4ef1f85dR7-R8 for how to do that. Set it to whatever environment has Plots installed.

(The entire error message you're seeing would be useful though.)

@lrnv
Copy link

lrnv commented May 23, 2024

Ok it finnaly worked, but it looks like I have the sam ebug as before :

lrnv@laptop slides_juliacon2024 $ QUARTO_JULIA_PROJECT="C:\\Users\\lrnv\\.julia\\dev\\QuartoNotebookRunner" quarto render examples.qmd
Running [1/1] at line 17:  using Random, Plots#|
ERROR: Julia server returned error after receiving "run" command:
Failed to run notebook: C:\Users\lrnv\Documents\slides_juliacon2024\examples.qmd       
ERROR: EvaluationError: Encountered 1 error during evaluation

Error 1 of 1
@ C:\Users\lrnv\Documents\slides_juliacon2024\examples.qmd:17
Error showing value of type Plots.Plot{Plots.GRBackend}
only png or svg allowed. got: :pdf
Stacktrace:
 [1] error(s::String)
   @ Base .\error.jl:35
 [2] _show(io::IOContext{IOBuffer}, ::MIME{Symbol("text/html")}, plt::Plots.Plot{Plots.GRBackend})
   @ Plots C:\Users\lrnv\.julia\packages\Plots\ju9dp\src\output.jl:203
 [3] #invokelatest#2
   @ .\essentials.jl:887 [inlined]
 [4] invokelatest
   @ .\essentials.jl:884 [inlined]
 [5] show(io::IOContext{IOBuffer}, m::MIME{Symbol("text/html")}, plt::Plots.Plot{Plots.GRBackend})
   @ Plots C:\Users\lrnv\.julia\packages\Plots\ju9dp\src\output.jl:232
 [6] show(io::IOContext{IOBuffer}, m::String, x::Plots.Plot{Plots.GRBackend})
   @ Base.Multimedia .\multimedia.jl:123

ERROR: Internal julia server error

Stack trace:
    at writeJuliaCommand (file:///C:/Users/lrnv/AppData/Local/Programs/Quarto/bin/quarto.js:41666:19)
    at eventLoopTick (ext:core/01_core.js:153:7)
    at async executeJulia (file:///C:/Users/lrnv/AppData/Local/Programs/Quarto/bin/quarto.js:41575:22)
    at async Object.execute (file:///C:/Users/lrnv/AppData/Local/Programs/Quarto/bin/quarto.js:41326:20)
    at async renderExecute (file:///C:/Users/lrnv/AppData/Local/Programs/Quarto/bin/quarto.js:77864:27)
    at async renderFileInternal (file:///C:/Users/lrnv/AppData/Local/Programs/Quarto/bin/quarto.js:78032:43)
    at async renderFiles (file:///C:/Users/lrnv/AppData/Local/Programs/Quarto/bin/quarto.js:77900:17)
    at async render (file:///C:/Users/lrnv/AppData/Local/Programs/Quarto/bin/quarto.js:82728:21)
    at async Command.actionHandler (file:///C:/Users/lrnv/AppData/Local/Programs/Quarto/bin/quarto.js:82876:32)
    at async Command.execute (file:///C:/Users/lrnv/AppData/Local/Programs/Quarto/bin/quarto.js:8015:13)
lrnv@laptop slides_juliacon2024 $ 

@MichaelHatherly
Copy link
Collaborator Author

Hmm, lets confirm that that is actually using this branch.

Can you swap out the contents of the notebook with

```{julia}
names(Main)
```

# Intro

## Slide working 
bla bla 

## Slide not working 

```{julia}
#| echo: false
#| fig-cap: "my caption"
# using Random, Plots
# x = rand(2, 1000)
# scatter(x[1, :], x[2, :])
```

I'm interested in what is printed out by the names(Main) cell.

@lrnv
Copy link

lrnv commented May 23, 2024

prints out

59-element Vector{Symbol}:
:BUFFER_SIZE
:Base
:Core
:IOCapture
:InlineDisplay
:MIMETYPE_WRAPPERS
:MSG_BOUNDARY
:Main
:MsgID
:MsgType
�
:main
:ojs_define
:pkg_hook_creator
:refresh!
:render
:render_mimetypes
:serve
:with_context
:with_inline_display

@lrnv
Copy link

lrnv commented May 23, 2024

All of them :

names(Main)[1:20]
20-element Vector{Symbol}:
:BUFFER_SIZE
:Base
:Core
:IOCapture
:InlineDisplay
:MIMETYPE_WRAPPERS
:MSG_BOUNDARY
:Main
:MsgID
:MsgType
:Notebook
:NotebookInclude
:OPTIONS
:PACKAGE_LOADING_HOOKS
:PACKAGE_REFRESH_HOOKS
:PKG_VERSIONS
:PNG
:POST_ERROR_HOOKS
:POST_EVAL_HOOKS
:PROJECT


names(Main)[21:40]
20-element Vector{Symbol}:
:PlotlyBasePlot
:PlotlyJSSyncPlot
:RCall_temp_files
:SVG
:WrapperType
:_CairoMakie_hook
:_Makie_hook
:_PlotlyBase_hook
:_PlotlyJS_hook
:_Plots_hook
:_RCall_hook
:_RCall_refresh_hook
:_buffer_writes
:_channel_cache
:_discard_until_boundary
:_figure_metadata
:_flatmap
:_getproperty
:_mimetype_wrapper
:_parseall


names(Main)[41:end]
19-element Vector{Symbol}:
:_pkg_version
:_register_wrapper
:_render_thunk
:_serialize_msg
:_transform_output
:clean_bt_str
:format_error
:handle
:include_str
:interrupt
:main
:ojs_define
:pkg_hook_creator
:refresh!
:render
:render_mimetypes
:serve
:with_context
:with_inline_display

@MichaelHatherly
Copy link
Collaborator Author

Thanks. quarto isn't using this branch, I'd expect that printout to not contain IOCapture, or many of the others. It appears to be somehow still using the quarto-managed environment...

What shell are you using? Setting the environment variable with the bash syntax might not be working.

@lrnv
Copy link

lrnv commented May 23, 2024

It's git-bash on windows, usually quite compliant with linux bash

@MichaelHatherly
Copy link
Collaborator Author

Let me see if I can find a more reliable way to make this kind of stuff testable. I'll ping you once I've worked that out :) probably won't be til tomorrow, thanks for taking the time to try this now though, much appreciated.

@lrnv
Copy link

lrnv commented May 23, 2024

Nevermind, thnaks for taking care of this -- this bug is basically making the whole engine unusable for me, and i have juliacon slides to write ;)

else
Plots.gr(; size_kwargs..., fmt = fm.fig_format, dpi_kwargs...)
end
Plots.gr(; size_kwargs..., dpi_kwargs...)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's what I gathered from the linked Plots code as well. I'm not sure what backends would actually use pdf assets if we offer them, maybe latex-based ones.

@MichaelHatherly MichaelHatherly merged commit 65ab142 into main May 24, 2024
9 of 10 checks passed
@MichaelHatherly MichaelHatherly deleted the mh/plots-pdf branch May 24, 2024 06:29
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.

Pdf plotting is broken (worked on iJulia)
3 participants