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

Warning "cannot cache unpickable configuration value" when config contains a reference to a function #12300

Closed
theOehrly opened this issue Apr 17, 2024 · 12 comments
Labels
awaiting:response Waiting for a response from the author of this issue

Comments

@theOehrly
Copy link

theOehrly commented Apr 17, 2024

Describe the bug

When the configuration contains a reference to a function, the warning "cannot cache unpickable configuration value" is show. If warnings are treated as errors, this of course fails the configuration build.

This problem was introduced in v7.3.0. It seems to only occur on a clean build (at least in the case that I observed). This is still problematic because CI workflows commonly do clean builds and then fail.

For the reason why function references are used in the configuration, I'll refer to #11777 where there is a more detailed explanation of the reasons.
The steps for reproduction are the same as in that issue, albeit with a different outcome/error.

How to Reproduce

pip install sphinx sphinx-gallery
sphinx-quickstart --sep --project test --author me -l en -r 0.0.1
mkdir examples
echo "" > examples/readme.txt

Add the following content to the end of the default conf.py in the source directory:

extensions = [
    'sphinx_gallery.gen_gallery',
]


def example_func():
    return


sphinx_gallery_conf = {
   'reset_modules': (example_func, ),
}

Run make html and notice "WARNING: cannot cache unpickable configuration value: 'sphinx_gallery_conf'" in the output.
This problem only occurs on a clean build (at least in this case), so it is necessary to run make clean before reproducing the problem again.

Environment Information

Platform:              win32; (Windows-10-10.0.19044-SP0)
Python version:        3.12.1 (tags/v3.12.1:2305ca5, Dec  7 2023, 22:03:25) [MSC v.1937 64 bit (AMD64)])
Python implementation: CPython
Sphinx version:        7.3.0
Docutils version:      0.21.1
Jinja2 version:        3.1.3
Pygments version:      2.17.2

Sphinx extensions

extensions = [
    'sphinx_gallery.gen_gallery',
]

Additional context

@chrisjsewell
Copy link
Member

Heya, this is not a bug, it is intended: #12203

If you are happy to live with not having caching, then you simply add the config: suppress_warnings = ["config.cache"],

(note also the new config to show warning types: https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-show_warning_types

@chrisjsewell
Copy link
Member

With 959f73f,
potentially you could omit this warning; for config that only rebuilds html AND are un-cacheable because they are FunctionType.
But that is quite specific obviously

@chrisjsewell chrisjsewell added awaiting:response Waiting for a response from the author of this issue and removed type:bug labels Apr 17, 2024
@chrisjsewell
Copy link
Member

chrisjsewell commented Apr 17, 2024

I'd note that one of the reasons I added this is that I literally come across the issue when adding this configuration: https://myst-nb.readthedocs.io/en/latest/authoring/custom-formats.html#custom-formats,
originally this was given as a function, but because of the issue of caching, I changed the behaviour to simply have the config store the fully qualified name, then load that within the extension

I feel this may be the better route for extensions to go, rather than relying on Sphinx to have complex cache special cases

@theOehrly
Copy link
Author

@chrisjsewell thanks for the quick response. I have been trying to make sense of this for a bit now. I do understand why the warning is shown in the sense of what triggers the warning.

What I do not understand are the consequences that the warning is warning me about. I thought that issue with unintended full rebuilds was resolved with 959f73f
I read through the source code a bit more now. Am I understanding this correctly that during pickling these values are omitted? Then when reloading the environment, this is still not a problem, but only once these values are actually accessed in the configuration, they don't exist in the environment that was loaded with pickle? So a full rebuild is then only triggered if these values are actually accessed?

I changed the behaviour to simply have the config store the fully qualified name, then load that within the extension
I feel this may be the better route for extensions to go, rather than relying on Sphinx to have complex cache special cases

While I do agree that this is a better way, I want to state that I am not a fan of using a Python file as a configuration file and then imposing limitations like these. Simply because that behaviour is then very unexpected for the average user, in my opinion. Most people won't know about caching environments and configurations and so on. But I understand that this is not a simple problem, and this may still be the best solution.

My reaction to this warning was "it fails my CI, I don't really know what it wants to tell me and I don't know how to silence it".

With my current and still somewhat limited overview, I'd argue that this warning should be improved to better guide the user. Even when explicitly searching for it, I have a hard time finding information in the documentation about the process of pickling and caching environments and the benefits and consequences this has. I think this would underline my point that many users won't know about this.

@chrisjsewell
Copy link
Member

So a full rebuild is then only triggered if these values are actually accessed?

Not that is not the case:

for configuration variables that are set with rebuild='env' (the default), this is the relevant code:

for item in config.filter(frozenset({'env'})):
if self.config[item.name] != item.value:
self.config_status = CONFIG_CHANGED
self.config_status_extra = f' ({item.name!r})'
break

So for these configuration variables, if they do not get cached, then they will for sure trigger a "full" rebuild, i.e. re-parsing all documents and re-writing all output files (e.g. HTML files)

As per #12203, I certainly feel it is not a good user-experience, to have these re-builds triggered, without any indication that it is because of un-cacheable variables

for configuration variables, that are set with rebuild='html', then this is where 959f73f comes in:

if config:
values = {c.name: c.value for c in config.filter(config_categories)}
self.config_hash = _stable_hash(values)

If this hash is not equal, then it will trigger a "write" re-build, i.e. just re-writing all the HTML files

here, then yes there is now some "handling" of these un-cacheable values

I honestly feel that it is not ideal, that there are these two wildly different mechanisms for cache-busting 🤔

With my current and still somewhat limited overview, I'd argue that this warning should be improved to better guide the user.

Well, this is why I introduced #12131, and in the future would like to make it the default 😄,
because then you could do like rust, ruff, mypy, etc do and have a specific documentation URL to explain each (core) warning type and how you might fix them:

@larsoner
Copy link
Contributor

larsoner commented Apr 17, 2024

From the description of #12203 :

it can be a source of confusion when, on performing a sphinx-build on a cached project, an unchanged configuration variable is always marked as changed (which then triggers a full rebuild).

Agreed about this, it can be confusing! However, in sphinx-gallery we ensure that the __repr__ of our functions are always stable across invocations of sphinx-build (by making them the __call__ of a class with a stable __repr__), so the variable is never marked as changed, and everything works okay. On first build it builds the whole site, then subsequent calls are no-ops. At least that was the case on previous versions of sphinx for years, not sure if it's still true or not (I need to check!).

If it doesn't cause a rebuild in practice, maybe one improvement would be to make the message more verbose in saying that this inability to cache properly will cause a rebuild only if the object does not have a stable __repr__?

And maybe at the SG end we can figure out how to make the objects pass the is_serializable check (see also sphinx-gallery/sphinx-gallery#1286).

EDIT: Also I noticed that the config.cache option is missing from https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-suppress_warnings

@chrisjsewell
Copy link
Member

we ensure that the __repr__ of our functions are always stable across invocations of sphinx-build

I'd just re-emphasise: this only works (and has only ever worked) with config variables marked as rebuild="html" (well technically also epub), not the default rebuild="env"

This is the case for sphinx-gallery, but obviously not the case for others: https://github.com/sphinx-gallery/sphinx-gallery/blob/e22fc20463055199dee565db3c21c34c9d195c6e/sphinx_gallery/gen_gallery.py#L1550-L1552

@AA-Turner
Copy link
Member

Closing this now:

  1. The warning is new and intended to address a common failure mode of the build cache.
  2. The warning can be disabled with suppress_warnings = ["config.cache"]
  3. Documentation has been added at suppress_warnings.
  4. Extensions that rely on functions within setup.py should seek to add an alternate configration mechanism that doesn't rely on using such functions or classes.

A

@chrisjsewell
Copy link
Member

Thanks for mentioning about the caching discrepancy with html re-builds though @theOehrly

ru-fu added a commit to ru-fu/starter-pack that referenced this issue Apr 18, 2024
canonical#203
introduced a function into the `html_context` configuration.
In parallel, a Sphinx update added a warning message for such
cases (see sphinx-doc/sphinx#12300).

This commit suppresses the new warning for now.
We might be able to find a better solution later.

Signed-off-by: Ruth Fuchss <ruth.fuchss@canonical.com>
@jschueller
Copy link
Contributor

jschueller commented Apr 22, 2024

I get the same warning/error, but my sphinx_gallery_conf only contains strings contrary to what sphinx says:

extensions.append('sphinx_gallery.gen_gallery')
sphinx_gallery_conf = {
     'examples_dirs': ['examples'], # path to example scripts
     'gallery_dirs': ['auto_examples'], # path to where to save gallery gen. output
     'ignore_pattern': '/(torque_model)|(__init__.py)',
}

sphinx.errors.SphinxWarning: cannot cache unpickable configuration value: 'sphinx_gallery_conf' (because it contains a function, class, or module object)
Warning, treated as error:
cannot cache unpickable configuration value: 'sphinx_gallery_conf' (because it contains a function, class, or module object)

@larsoner
Copy link
Contributor

Pretty sure it's the same error because sphinx-gallery fills in entries in the conf when it loads, and some of those entries are classes

@jschueller
Copy link
Contributor

jschueller commented Apr 22, 2024

oh, we must wait #1289 then, got it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting:response Waiting for a response from the author of this issue
Projects
None yet
Development

No branches or pull requests

5 participants