Pass additional configuration options to the `jupyter lite build` command

agriyakhetarpal commented 6 months ago

Description

This PR closes #168. It aims to implement additional options from the JupyterLite CLI, i.e., the jupyter lite build command that is currently wrapped in a subprocess call inside a Sphinx app. This enables the use of, say, custom environments from a file as requested in https://github.com/jupyterlite/jupyterlite-sphinx/issues/168#issuecomment-2079472156, allowing overrides in case of conflicts where environment.yml might refer to a file that is not being used for jupyterlite-sphinx but for setting up general environments with mamba or conda.

Changes made

Added jupyterlite_build_command_options as a configuration option to allow power users of jupyterlite-sphinx to customise their JupyterLite build procedure with a dictionary of options.

Task list

[x] Implement ways to add options from https://jupyterlite.readthedocs.io/en/latest/reference/cli.html#common-parameters
[x] Add documentation about these user-configurable options
[x] Some of the options are available to be overridden by environment variables, adjust the default values in the setup() function?
[x] Address potential conflicts with options, as noted by @steppi in #168, copying here for ease of access:

except we might want to document how to avoid having options that conflict with the ones that are currently included, and put in place safeguards to prevent conflicting options

Additional context

Since the jupyterlite-pyodide kernel doesn't support pre-installing packages right now, it's not possible to add requirements.txt files. Users shall have to use the emscripten-forge channel and the xeus kernel to get the packages they need.

steppi commented 6 months ago

Thanks @agriyakhetarpal. Is there any reason why we can't just let the users pass a dictionary of options with values in conf.py, and check that none of the options they've provided conflict with the existing ones? To me, this seems preferable to manually adding new options every time we discover something that would make sense to have.

Letting the user provide an entirely custom command seems like it might be too flexible, without much benefit over just allowing for extra options.

agriyakhetarpal commented 6 months ago

Thanks @agriyakhetarpal. Is there any reason why we can't just let the users pass a dictionary of options with values in conf.py, and check that none of the options they've provided conflict with the existing ones? To me, this seems preferable to manually adding new options every time we discover something that would make sense to have.

This is what I was thinking as well with allowing a custom jupyter lite build command – a dictionary would indeed serve that purpose better, sure. Do you have something in mind about checking for conflicts with existing options, i.e., do we plan to check against the current trio of --contents, --output-dir, and --lite-dir – and gracefully raise an error, and for additional options that we currently don't have but plan to add with this PR, let the jupyter lite CLI raise errors on conflicts on its own accord?

steppi commented 6 months ago

Do you have something in mind about checking for conflicts with existing options, i.e., do we plan to check against the current trio of --contents, --output-dir, and --lite-dir – and gracefully raise an error, and for additional options that we currently don't have but plan to add with this PR, let the jupyter lite CLI raise errors on conflicts on its own accord?

Yeah, checking for conflicts with the existing options, and if found raising, but leaving anything beyond that to the jupyter lite CLI sounds like the right approach. When we document this, we can show what the default command is and let users know there will be an error if they supply options that conflict with that, but beyond that, they are responsible for entering sensible options.

agriyakhetarpal commented 6 months ago

Thanks, here's a very crude implementation in 6eb6680664cb5096fba50d1a7e446409533197cd – I feel that snake_case is better than kebab-case, especially inside Python files like it is so here. I think it needs further refinement, and additions to the documentation of course. Open to suggestions.

Also, I un-silenced JupyterLite in 617a614831fd850b3462a320bd7d09f911bb413f, maybe it's good for us because we can inspect the logs both when checks pass and when they don't? Happy to revert the change if you disagree!

steppi commented 6 months ago

Thanks @agriyakhetarpal. This is starting to look good. I'll take a closer look later today.

Also, I un-silenced JupyterLite in 617a614, maybe it's good for us because we can inspect the logs both when checks pass and when they don't? Happy to revert the change if you disagree!

I see, what you've done is not to un-silence the output by default, but just override the default in the conf.py for jupyterlite-sphinx's docs. We might want to do that, but if so it should go into a separate PR and be discussed separately.

Can you add the example from https://github.com/jupyterlite/jupyterlite-sphinx/issues/168#issuecomment-2079472156 for changing the path to where xeus looks for the environment file to the docs?

agriyakhetarpal commented 6 months ago

I see, what you've done is not to un-silence the output by default, but just override the default in the conf.py for jupyterlite-sphinx's docs. We might want to do that, but if so it should go into a separate PR and be discussed separately.

Ah, apologies if I was too terse. I indeed mean to do it for jupyterlite-sphinx, i.e., here, and not for other repositories. However, I agree that it should go into another PR, not here – I'll revert the commit. Edit: reverted in 78b4a37ded2be308507625924401179f27473c6d.

agriyakhetarpal commented 6 months ago

Can you add the example from https://github.com/jupyterlite/jupyterlite-sphinx/issues/168#issuecomment-2079472156 for changing the path to where xeus looks for the environment file to the docs?

I have now done this in f6e6d3b2154fe42b22528d3bb1b108ad04d3ab84, happy to receive comments on the wording and if the location of this section is alright. I enabled some admonitions for this page by enabling the colon_fence MyST extension in 0b5e4996648f7363774d2f10b531a6d020ee856f, please let me know if that is out of scope for this PR.

steppi commented 6 months ago

@agriyakhetarpal. Do you still consider this to be a work in progress, and if so, what else are you planning to do?

agriyakhetarpal commented 6 months ago

I just merged main and I was editing the PR description just now :) I think this should be good to go very soon.

agriyakhetarpal commented 6 months ago

Edited the title and the description. I think this is the only remaining point that we can think to address:

Some of the options are available to be overridden by environment variables, adjust the default values in the setup() function?

If someone does not pass in a custom option via conf.py through this configuration value, they can alternatively do so using environment variables. However, it is possible that options set via environment variables can cause conflicts with those set via conf.py and it would be difficult for the user to debug that? I don't see anything in the CLI documentation about whether the environment variable takes priority or whether the option in the command does (it is the latter, I just confirmed, and it's usually so based on how most CLIs work – but should we document that, considering we would like to reduce irrelevant bug reports?)

steppi commented 6 months ago

but should we document that, considering we would like to reduce irrelevant bug reports?)

Yes. I think it would be good to add a sentence saying that options in the jupyter lite build command take precedence over options in the config file. Before the sentence about this being an advanced feature I think.

steppi commented 6 months ago

Yes. I think it would be good to add a sentence saying that options in the jupyter lite build command take precedence over options in the config file. Before the sentence about this being an advanced feature I think.

Or I guess there are environment variables, a json config file, and command line options?

agriyakhetarpal commented 6 months ago

Or I guess there are environment variables, a json config file, and command line options?

The documentation seems to be quite sparse about it – the JSON configuration file isn't mentioned on the CLI documentation page, and https://jupyterlite.readthedocs.io/en/stable/howto/configure/config_files.html explains which JSON files affect the build command, but there is no reference about the priority as such. I think the priority is CLI > environment variables > JSON configuration file, but I need to check for the second comparison.

agriyakhetarpal commented 6 months ago

Or, we can not state the comparison directly through the sentence added in a9ca8867f44bd56bfa4a82c206e947bf2dedcb25, considering this is a feature for power users and they can figure out things as they try it (and no one would, in realistic contexts, use a mixture of all three: conf.py, environment variables, and a configuration JSON file altogether – a combination of the first and the third options is more likely).

This is ready for re-review/merging on my end.

steppi commented 6 months ago

Thanks @agriyakhetarpal, this looks good to me now.

jupyterlite / jupyterlite-sphinx