ENH: Modify execute option for jb page command.

[rossbar]

Modifies the meaning of the --execute flag for the jb page command.

Using the click.options --opt/--no-opt pattern for boolean flags,
modify the behavior of jb page so that notebook targets are executed
by default, and can be toggled off by passing in the --no-execute
flag.

Marking as draft until added tests + update corresponding docs

[choldgraf]

just a note that this is here well, it's checking for the execute config from this page, so check which one to use (again, apologies for the slightly confusing config setup)

[rossbar]

Thanks for the additional info - I need to spend a bit more time mapping out the different objects/containers that hold configuration information in the different layers (jb, myst-nb, etc.) and how they map to each other.

[rossbar]

Okay, I've added tests and a blurb to the documentation to update the feature. Note that there are now three tests for page in test_build that have a lot of duplication in them - I'd be happy to refactor to get rid of some of the duplication in test setup, but my inclination would be to wrap them up into a class where I think the preferred way of doing things in pytest is with fixtures. LMK if that's something you'd like me to take a stab at here!

[choldgraf]

@rossbar go for it! improving the test infra would be great

[rossbar]

Okay, I gave it a shot - I'm not sure it would be considered an improvement :). The changes eliminate a bit of code associated with setting up each individual jb page test. I'm not sure that patterns I used are particularly useful - LMK what you think! I'm happy to revert if this ends up being more trouble than it's worth.

[choldgraf]

Sounds good! I see some of the tests are failing, is that expected on your end? I am +1 on the change if we get the tests working

[rossbar]

Good point - they were failing for me on master, but it looks like a simple requirements issue - will fix now!

[rossbar]

Hmm - so one of the failing tests is from test_clean.py and is related to .jupyter_cache - it's tricky as the failure seems to be intermittent. I don't get a failure with a fresh clone of the repo but if I run the test suite again it fails (though not 100% of the time).

I'm not exactly sure what is happening there - seems related to the caching somehow? I'm not too familiar with that component.

[rossbar]

bisecting narrows it down to the config change I made in yaml.py

[rossbar]

I'm still having trouble chasing down the execution order of the configuration steps. Based on the comment above setup here:

jupyter-book/jupyter_book/__init__.py

Lines 23 to 35 in d76cdba

	# We connect this function to the step after the builder is initialized
	def setup(app):
	app.connect("config-inited", update_indexname)
	app.connect("source-read", add_toctree)
	app.add_config_value("globaltoc_path", "toc.yml", "env")
	# configuration for YAML metadata
	app.add_config_value("yaml_config_path", "", "html")
	app.connect("config-inited", add_yaml_config)
	app.connect("config-inited", add_static_files)

it seems like it might be possible that some of the default configuration parameters are overriding config values passed into build_sphinx. That might explain why the configuration done for page (e.g. setting jupyter_execute_notebooks to off via the --no-execute flag) isn't taking effect.

Does this seem like a reasonable hypothesis? Note also that for a single build, "auto", "force" and "cache" should all induce same behavior when it comes to executing notebooks, and their doesn't seem to be a test for building with "off", so the above might be slipping through the current test suite.

[rossbar]

I think I've figured out the root cause of the problems here. Essentially, it boils down to the fact that a config parameter (jupyter_execute_notebooks) was being modified via the command line, but the value is also specified in _config.yml. In this case, because of execution order (build_sphinx first, followed by add_config_yaml registered via app.connect), the value in the _config.yml would always override whatever is specified via the CLI flag.

In d895c1b I've illustrated a way to get around this issue, though in general I'm not very happy with it because I think it makes the configuration machinery even more complicated. I mostly added it in an attempt to better illustrate what I think the problem is.

As an aside - I wasn't familiar with the development pattern for sphinx extensions before trying to track down what was happening here. If it wasn't for your comment in the code I linked above, I would've floundered for even longer.

[choldgraf]

Hmmmm - I see...so I think there's a root problem here that you've identified, which is that CLI arguments will be over-ridden by the _config.yml arguments because of the order in which they are defined. Is that right?

If so, I feel like we should try to nail that bug down and come up with a more natural way for configuring to happen. Do you agree? Then it could be much more straightforward to configure etc.

[rossbar]

Hmmmm - I see...so I think there's a root problem here that you've identified, which is that CLI arguments will be over-ridden by the _config.yml arguments because of the order in which they are defined. Is that right?

Yes, this is correct.

If so, I feel like we should try to nail that bug down and come up with a more natural way for configuring to happen. Do you agree? Then it could be much more straightforward to configure etc.

I agree completely - as I mentioned, I only listed the "solution" in d895c1b to help illustrate the point. I do not think it's a good idea to add another *_override as I did - this would just make the configuration even more confusing.

What would make sense to me is closing this PR and linking the discussion in a corresponding issue that is more accurately titled to capture the larger issue about the execution-order of the configuration steps.

[choldgraf]

OK cool, lemme think about this a little bit and I'll try and whip up a PR that will improve the configuration stuff :-)

[rossbar]

Closing for now, will revisit after potential changes to the configuration process. Thanks for taking the time to follow up on the meandering discussion here!

[choldgraf]

Sounds good - I'm going to try to get to it this week :-) this was a really helpful challenge to highlight, thanks for doing so!