_httpdomain.py: import both collections and collections.abc

[flokli]

While a1d4fca fixed the import for
collections.Mapping, using collections.defaultdict broke.

Import both packages, and use them where appropriate.

[qmonnet]

Hi and thanks for the report and PR!

I'm a bit surprised, because I didn't get any error when testing locally, or when we build the online docs for Cilium. I'm not a Python expert though so I'm ready to believe you, but could you please provide more details on the error you got? I'd like to understand better what's going on here

[flokli]

I got this error when trying to render docs using sphinx. It might be you didn't invoke that specific code path when building cilium docs?

This was with python 3.9, and which attributes exist, which don't, and which show deprecation warnings can be easily inspected from the console:

Python 3.9.13 (main, May 17 2022, 14:19:07) 
[GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import collections
>>> import collections.abc
>>> collections.defaultdict
<class 'collections.defaultdict'>
>>> collections.Mapping
<stdin>:1: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3, and in 3.10 it will stop working
<class 'collections.abc.Mapping'>
>>> collections.abc.Mapping
<class 'collections.abc.Mapping'>
>>> collections.abc.defaultdict
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'collections.abc' has no attribute 'defaultdict'

Python 3.10.6 (main, Aug  1 2022, 20:38:21) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import collections
>>> import collections.abc
>>> collections.defaultdict
<class 'collections.defaultdict'>
>>> collections.Mapping
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'collections' has no attribute 'Mapping'
>>> collections.abc.Mapping
<class 'collections.abc.Mapping'>
>>> collections.abc.defaultdict
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'collections.abc' has no attribute 'defaultdict'

As can be seen, collections.Mapping still exists, but is deprecated (and removed in 3.10).

collections.abc.defaultdict doesn't exist (and never did), which is why

openapi/sphinxcontrib/openapi/renderers/_httpdomain.py

Line 41 in a1d4fca

order_by = collections.defaultdict(

can't work if collections is in fact collections.abc (from your import above).

[qmonnet]

Interesting, I confirm I've not seen this before. We usually build the docs in a container, running make -C Documentation html. The base image is Alpine with Python 3.10, so this is surprising. What command do you use to build, if you don't mind me asking?

[flokli]

The makefile is super minimal and standard, I doubt there's anything interesting here:

# Minimal makefile for Sphinx documentation
#
 
# You can set these variables from the command line, and also
# from the environment for the first two.
SPHINXOPTS    ?=
SPHINXBUILD   ?= sphinx-build
SOURCEDIR     = source
BUILDDIR      = build
SOURCE_DATE_EPOCH ?= $(shell git log -1 --format=%ct)
 
# Put it first so that "make" without argument is like "make help".
help:
  @$(SPHINXBUILD) -M help "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)
 
.PHONY: help Makefile
 
# Catch-all target: route all unknown targets to Sphinx using the new
# "make mode" option.  $(O) is meant as a shortcut for $(SPHINXOPTS).
%: Makefile
  @$(SPHINXBUILD) -M $@ "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)

I essentially invoke make html, and the schema is in the source folder.

All this happens in an environment with python 3.9 (due to ikalnytskyi/picobox#55).

I have no idea _httpdomain.py didn't break when rendering docs for Cilium. My guess is that none of the schemas you asked it to render invoked the collections.defaultdict call, but that's just a guess… 🤷

I wasn't able to get the cilium Makefile to work, and I'm not really to involved enough to get it to work.

Can you trigger a new build with this code applied and check? I guess for quick testing, you could literally simply edit the Dockerfile to copy over the updated file to /usr/local/lib/python3.10/site-packages/sphinxcontrib/openapi/renderers/_httpdomain.py, and try rendering docs with it…

[qmonnet]

I wasn't able to get the cilium Makefile to work, and I'm not really to involved enough to get it to work.

OK, I looked at the PR from which you link this one, above. The bit of context I was missing is that you're using the fork for another project. I thought you were only trying to build Cilium's docs and somehow seeing failures that I didn't while doing so. So yeah, maybe we just don't go through the same paths in the code.

Can you trigger a new build with this code applied and check? I guess for quick testing, you could literally simply edit the Dockerfile to copy over the updated file to /usr/local/lib/python3.10/site-packages/sphinxcontrib/openapi/renderers/_httpdomain.py, and try rendering docs with it…

I can do better, I can pull directly your fork in the container by simply editing the requirements file:

sphinxcontrib-openapi @ git+https://github.com/flokli/sphinxcontrib-openapi.git@fix-collections

[qmonnet]

As a side note, I'd recommend being very careful if you use the Cilium fork for this project. We forked it because we needed the switch to mdinclude, but we don't particularly intend to maintain this fork over time, and it could as well be removed in the future if we find a better solution upstream. So do keeping a copy of a functional version of the fork just in case might be a good idea.

Ideally, I would like to contribute the changes in this branch to the upstream version of openapi. I haven't yet, because the test suite was not passing because mdinclude doesn't support docutils 0.19 (not sure what's the chain of failure but as far as I remember it's what it came down to), so I filed this issue in the meantime. [edit: issue now at https://github.com/omnilib/sphinx-mdinclude/issues/8).]

[flokli]

As a side note, I'd recommend being very careful if you use the Cilium fork for this project. We forked it because we needed the switch to mdinclude, but we don't particularly intend to maintain this fork over time, and it could as well be removed in the future if we find a better solution upstream. So do keeping a copy of a functional version of the fork just in case might be a good idea.

Ideally, I would like to contribute the changes in this branch to the upstream version of openapi. I haven't yet, because the test suite was not passing because mdinclude doesn't support docutils 0.19 (not sure what's the chain of failure but as far as I remember it's what it came down to), so I filed this issue in the meantime.

Understood - I'd also like to see upstream getting fixed. I'll try to keep an eye on the issue.

On the sphinx-mdinclude issue, I sent the patch I'm using to get it to work with more recent versions of docutils.

[qmonnet]

OK, this works for me when testing locally. Let me just give it a try with our CI tomorrow to make sure I don't break the build there (I don't see a reason why it would, but let's check anyway).

One nit, would you mind explaining why we can drop the Python 3.3 guard in your commit log, please?

On the sphinx-mdinclude issue, I sent the patch I'm using to get it to work with more recent versions of docutils.

Thanks a lot for that! Let's see if they pick it up. Might be worth opening a PR directly in a few days otherwise.

[flokli]

OK, this works for me when testing locally. Let me just give it a try with our CI tomorrow to make sure I don't break the build there (I don't see a reason why it would, but let's check anyway).

Thanks!

One nit, would you mind explaining why we can drop the Python 3.3 guard in your commit log, please?

Sure thing! Added to the commit log and pushed again!

Thanks a lot for that! Let's see if they pick it up. Might be worth opening a PR directly in a few days otherwise.

Yes! Same probably applies to ikalnytskyi/picobox#55, and I wonder why you're not running into the issues there, but that's definitely out of scope for this PR.

[qmonnet]

OK this worked fine, I'm merging. Thanks!

[flokli]

Thanks!

[qmonnet]

PR to upstream the changes: sphinx-contrib#127