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 call engine julia commands #11803

Merged
merged 52 commits into from
Mar 21, 2025
Merged

Conversation

jkrumbiegel
Copy link
Contributor

@jkrumbiegel jkrumbiegel commented Jan 6, 2025

Description

This PR adds CLI commands to interact with or diagnose the native julia engine. It adds a new subcommand call engine julia for this purpose:

$ quarto call engine            

Usage:   quarto call engine
Version: 99.9.9       

Description:

  Access functionality specific to quarto's different rendering engines.

Options:

  -h, --help  - Show this help.  

Commands:

  help   [command]  - Show this help or the help of a sub-command.                
  julia             - Access functionality specific to the julia rendering engine.

Implementation

The PR adds a new optional field populateCommand to the ExecutionEngine interface which only the julia engine currently sets, but others could potentially as well. This field takes a function that adds its own cliffy subcommands under its key, in the julia engine's case julia.

Julia commands

The subcommands for a given engine can be inspected as well. In this PR I've added status, kill and log and close [--force].

$ quarto call engine julia

Usage:   quarto call engine julia
Version: 99.9.9             

Description:

  Access functionality specific to the julia rendering engine.

Options:

  -h, --help              - Show this help.                             
  --log         <file>    - Path to log file                            
  --log-level   <level>   - Log level (info, warning, error, critical)  
  --log-format  <format>  - Log format (plain, json-stream)             
  --quiet                 - Suppress console output.                    
  --profile               - Active project profile(s)                   

Commands:

  help    [command]  - Show this help or the help of a sub-command.                               
  status             - Get status information on the currently running Julia server process       
  kill               - Kills the control server if it is currently running.                       
  log                - Prints the julia server log file if it exists which can be used to diagnose
                       problems. 

The status command can be used to print out information about the currently running server process. For example, if quarto render scratch/test.qmd has just been started the output is:

quarto call engine julia status
Julia server is responding.
  port: 8003
  pid: 48443
  julia version: 1.10.5
  environment: /Users/krumbiegel/.julia/dev/QuartoNotebookRunner
  runner version: 0.11.7
  workers active: 1
    worker 1:
      path: /Users/krumbiegel/dev/quarto-cli/scratch/test.qmd      
      run started: 3:48:01 PM (6 seconds ago)    
      run finished: -    
      timeout: 1000 seconds 

or when some notebooks are idling and waiting to be rerun:

$ quarto call engine julia status
Julia server is responding.
  port: 8003
  pid: 48443
  julia version: 1.10.5
  environment: /Users/krumbiegel/.julia/dev/QuartoNotebookRunner
  runner version: 0.11.7
  workers active: 1
    worker 1:
      path: /Users/krumbiegel/dev/quarto-cli/scratch/test.qmd      
      run started: 3:48:01 PM (1 minute 2 seconds ago)    
      run finished: 3:48:11 PM (took 10 seconds)    
      timeout: 1000 seconds (15 minutes 48 seconds left)

Before, there was no way to check what had gone wrong if the server didn't respond anymore. Now, the julia process writes to a log file which can be inspected with the log command:

$ quarto call engine julia log
[ Info: Log started at 2025-01-06T15:44:19.491
ERROR: LoadError: something failed
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] top-level scope
   @ ~/dev/quarto-cli/src/resources/julia/quartonotebookrunner.jl:92
in expression starting at /Users/krumbiegel/dev/quarto-cli/src/resources/julia/quartonotebookrunner.jl:92

If nothing helps or if the server should be restarted for some other reason, the kill command sends SIGTERM to the pid that's saved in the transport file.

$ quarto call engine julia kill
Sent SIGTERM to server process

The close command can be used to close workers that are currently idle. To close workers that are currently computing something, the --force flag can be added which adds a way to recover from hanging or long-running worker processes.

Trying this PR

This PR depends on unreleased changes to QuartoNotebookRunner.jl. You can clone the repository and check out PR 229. Then you can run quarto with QUARTO_JULIA_PROJECT=path/to/cloned/repo.

Summary

This PR adds julia-engine specific commands to the quarto CLI interface. Other than creating the means for other engines to hook in their own commands, this PR does not add any functionality for non-julia engines.

At the current stage I would first like to get maintainer approval for the general mechanism I introduced here. Comments about the interface for the actual commands is appreciated as well. I will probably also need a little help once the PR goes into the test writing stage, as I have only interacted with the notebook rendering test infrastructure so far, not the general tests for the CLI.

@cscheid
Copy link
Collaborator

cscheid commented Jan 6, 2025

I want to give a little bit of further thought to the engine name for two reasons:

  • a top-level command will live as long as quarto itself
  • our public top-level commands are all supposed to be verbs

quarto configure engine is a bit a of handful to type, but it reads much better when changing settings. It doesn't make sense for the options here, but I think we need something like that.

@jkrumbiegel
Copy link
Contributor Author

a top-level command will live as long as quarto itself

I completely understand, which is why I wanted to get your feedback early :)

our public top-level commands are all supposed to be verbs

I see, that didn't occur to me before. Not sure about configure engine given status and log but might make sense in the bigger picture. Maybe it could be allowed to use configure julia directly as a shortcut, then only the internal engine names would have to be avoided if other configurable things are added over time. configure engine xyz could be a fallback for future extensions.

@jkrumbiegel
Copy link
Contributor Author

@cscheid do you want me to go ahead and switch this PR to configure engine julia?

@jkrumbiegel
Copy link
Contributor Author

@cscheid bump :)

@cderv cderv requested a review from cscheid January 29, 2025 15:51
@jkrumbiegel
Copy link
Contributor Author

One more bump :)

@jkrumbiegel
Copy link
Contributor Author

About this one, @cscheid, let me recap what my current thinking is here:

  • the idea to make engines configurable via CLI was accepted in general
  • the exact API to do it was not confirmed yet
    • quarto engine seemed to be rejected because engine is not a verb
    • maybe quarto configure engine julia or quarto configure julia would work -> need confirmation on that
  • I need help how to add tests for this. So far I've only written smoke tests that check rendering. This would need to test other CLI commands.
  • The counterpart of this in QuartoNotebookRunner.jl is not yet merged. I might be able to make CI run off a QNR branch so I can demonstrate things are working before we commit to anything.

@cscheid
Copy link
Collaborator

cscheid commented Mar 5, 2025

(Thanks for bumping) quarto configure engine julia sounds like it would change something about the engine, but this is about checking, or inspecting.

Proposal: I actually think quarto check engine julia is precisely what we would want. It's a bit verbose, but each additional word makes sense given the previous ones, quarto check -> quarto check engine -> quarto check engine julia.

Context:

  • Currently, quarto check is really about internal health self-checks. But we even have quarto check jupyter which starts getting close to what you're looking to do with quarto check engine julia.
  • In addition, we have quarto check all (which is the default for quarto check). That could translate easily into quarto check engine all being equivalent to quarto check engine
  • quarto check is implemented as a simple dispatch over words. Adding engine would be trivial.

So what about the following? We add a new quarto check engine subcommand, and then when someone writes quarto check engine julia, we dispatch this to the engines through a new method check(what: string[]).

The main engineering complication is that this potentially propagates the Cliffy dependency into check, and into the engine checks themselves. It would happen because in the same way that someone does quarto check --help, they could then want quarto check engine --help, quarto check engine julia --help, etc.

@jkrumbiegel
Copy link
Contributor Author

quarto configure engine julia sounds like it would change something about the engine, but this is about checking, or inspecting.

The status and log commands fit this description, but the kill one doesn't. And I would potentially add other ones that for example interact with the internal installation of QuartoNotebookRunner (update) or with the Julia project that quarto would consider the one belonging to a given notebook quarto ... engine julia resolve notebook.qmd or something. So these are definitely more changing than checking. For example, currently we are confined to a single QuartoNotebookRunner version, but that doesn't need to be the case if we have an update command.

@jkrumbiegel jkrumbiegel marked this pull request as ready for review March 14, 2025 10:24
@jkrumbiegel
Copy link
Contributor Author

I'm marking this "ready for review" now because I'm generally happy with the functionality. However, the CI script still pulls in this PR PumasAI/QuartoNotebookRunner.jl#229 which probably only makes sense to merge after this PR here was ok'ed, as they belong together.

I have changed the interface to quarto call engine julia ... as requested.

No docs have been written, yet, I will do so once this PR passes review.

@cscheid
Copy link
Collaborator

cscheid commented Mar 14, 2025

This is great, and we'll get a lot of mileage out of quarto call. Thanks for putting it together!

Could you add an entry to the changelog? Just use the number of this pull request as reference, and use the "Other Fixes and Improvements" section (you'll see examples)

@jkrumbiegel
Copy link
Contributor Author

All right, I have added two entries, one more focused on the julia commands and one on the new quarto call

Copy link
Collaborator

@cscheid cscheid left a comment

Choose a reason for hiding this comment

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

I'm going to approve this. With that said, unless I'm missing something, we should consider adding a description() to src/command/call/cmd.ts

engineSubcommand
.description(
`Access functionality specific to the ${name} rendering engine.`,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this. I think that we should let engines have a method to build a response, but that can happen in a different PR.

@jkrumbiegel
Copy link
Contributor Author

jkrumbiegel commented Mar 14, 2025

All right, I've released QNR 0.15 and changed compat to that version so that I could remove the special branch from CI again. I've also added a description to call and made some further small changes to help strings. With that, I think this PR is good to merge.

@cscheid
Copy link
Collaborator

cscheid commented Mar 21, 2025

🎉

@cscheid cscheid merged commit 8bd9c6e into quarto-dev:main Mar 21, 2025
49 checks passed
@cderv
Copy link
Collaborator

cderv commented Mar 21, 2025

Thank you @jkrumbiegel !

@jkrumbiegel
Copy link
Contributor Author

Thank you for merging, I'll write up some docs next week

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.

4 participants