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

Add test_pr for testing PRs #137

Merged
merged 2 commits into from
May 24, 2024
Merged

Add test_pr for testing PRs #137

merged 2 commits into from
May 24, 2024

Conversation

MichaelHatherly
Copy link
Collaborator

From #136 it's clearly difficult to correctly setup quarto to correctly run the backend using a particular PR's branch so that we can more easily have people verify that a fix is correct. This helper aims to make that much simpler.

@jkrumbiegel I'm not 100% sure that --no-execute-daemon is having the intended effect on reruns, see the below outputs. In particular the differences between the Transport file logs, the first of which I'd expect, but the second one is saying it is reusing the server, even though --no-execute-daemon flag is passed. Any idea what's happening there?

julia> QuartoNotebookRunner.test_pr(`quarto render test/examples/integrations/Plots.qmd`; rev = "mh/plots-pdf")
    Updating git-repo `https://github.com/PumasAI/QuartoNotebookRunner.jl`
   Resolving package versions...
    Updating `/private/var/folders/09/t9pc5r114yg9pfrf28lb2d440000gn/T/jl_yDNrHE/Project.toml`
  [4c0109c6] + QuartoNotebookRunner v0.10.2 `https://github.com/PumasAI/QuartoNotebookRunner.jl#mh/plots-pdf`
...
- Transport file /Users/mike/Library/Caches/quarto/julia/julia_transport.txt doesn't exist
Starting julia control server process. This might take a while...
- Custom julia project set via QUARTO_JULIA_PROJECT="/var/folders/09/t9pc5r114yg9pfrf28lb2d440000gn/T/jl_yDNrHE". Checking if QuartoNotebookRunner can be loaded.
- QuartoNotebookRunner could be loaded successfully.
- Starting detached julia server through julia, once transport file exists, server should be running.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file read successfully.
Julia server process started.
- Connecting to server at port 8002, pid 59472
- write command "isready" to socket server
- received server response
- Transport file read successfully.
- write command "isopen" to socket server
- received server response
- write command "run" to socket server
- received server response
- received progress update response, listening for further responses
Running [1/3] at line 9:  Pkg.activate("Plots")
- received server response
- received progress update response, listening for further responses
Running [2/3] at line 14:  import Plots
- received server response
- received progress update response, listening for further responses
Running [3/3] at line 18:  Plots.plot(Plots.fakedata(50, 5), w = 3)
- received server response
- write command "close" to socket server
- received server response
pandoc 
  to: html
  output-file: Plots.html
  standalone: true
  section-divs: true
  html-math-method: mathjax
  wrap: none
  default-image-extension: png
  
metadata
  document-css: false
  link-citations: true
  date-format: long
  lang: en
  title: Plots integration
  
Output created: Plots.html

julia> QuartoNotebookRunner.test_pr(`quarto render test/examples/integrations/Plots.qmd`; rev = "mh/plots-pdf")
    Updating git-repo `https://github.com/PumasAI/QuartoNotebookRunner.jl`
   Resolving package versions...
    Updating `/private/var/folders/09/t9pc5r114yg9pfrf28lb2d440000gn/T/jl_sJqWJp/Project.toml`
  [4c0109c6] + QuartoNotebookRunner v0.10.2 `https://github.com/PumasAI/QuartoNotebookRunner.jl#mh/plots-pdf`
...
- Transport file /Users/mike/Library/Caches/quarto/julia/julia_transport.txt exists, reusing server.
- Transport file read successfully.
- Connecting to server at port 8002, pid 59472
- write command "isready" to socket server
- received server response
- Transport file read successfully.
- write command "isopen" to socket server
- received server response
- write command "run" to socket server
- received server response
- received progress update response, listening for further responses
Running [1/3] at line 9:  Pkg.activate("Plots")
- received server response
- received progress update response, listening for further responses
Running [2/3] at line 14:  import Plots
- received server response
- received progress update response, listening for further responses
Running [3/3] at line 18:  Plots.plot(Plots.fakedata(50, 5), w = 3)
- received server response
- write command "close" to socket server
- received server response
pandoc 
  to: html
  output-file: Plots.html
  standalone: true
  section-divs: true
  html-math-method: mathjax
  wrap: none
  default-image-extension: png
  
metadata
  document-css: false
  link-citations: true
  date-format: long
  lang: en
  title: Plots integration
  
Output created: Plots.html

Copy link

codecov bot commented May 23, 2024

Codecov Report

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

Project coverage is 53.46%. Comparing base (224d278) to head (2a4bed1).
Report is 1 commits behind head on main.

Files Patch % Lines
src/utilities.jl 0.00% 57 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
- Coverage   55.98%   53.46%   -2.53%     
==========================================
  Files          25       26       +1     
  Lines        1145     1199      +54     
==========================================
  Hits          641      641              
- Misses        504      558      +54     

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

@jkrumbiegel
Copy link
Collaborator

jkrumbiegel commented May 24, 2024

--no-execute-daemon relates to the worker process for the notebook. The server process governing the workers is not affected by that, there was no existing option in quarto that I could have reused for restarting this server process. The only way that I currently have to force the server process to restart is by killing it manually (grabbing the pid from the verbose log). Or by being the owner of the server process in the first place, by not letting quarto start it but starting it myself using the julia file stored inside quarto. But that's not a good general solution because it could break when something about that script is changed on the quarto side.

Another idea worth thinking about is adding more information about the server process to the transport file, so that quarto can automatically restart the process if this information doesn't match. Currently, the transport file is "dumb" as it just has the port, pid and key, so it will be reused without question (a new one just will be started if the existing one doesn't answer in reasonable time).

https://github.com/quarto-dev/quarto-cli/blob/main/src/resources/julia/quartonotebookrunner.jl#L15-L17

@MichaelHatherly MichaelHatherly marked this pull request as ready for review May 24, 2024 09:39
@MichaelHatherly
Copy link
Collaborator Author

Obviously kind of brittle if anything upstream changes, but should make it a bit more straightforward to be able confirm fixes. I've just matched the cache paths for each platform based on what quarto itself is doing.

@jkrumbiegel
Copy link
Collaborator

jkrumbiegel commented May 24, 2024

At least it's likely that if anybody breaks that specific code upstream, it's going to be us.

@MichaelHatherly MichaelHatherly merged commit 63c8a52 into main May 24, 2024
8 of 10 checks passed
@MichaelHatherly MichaelHatherly deleted the mh/test_pr branch May 24, 2024 10:43
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.

2 participants