| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-19 | |||
| 16:27:11 | sean-k-mooney | ya that said i know the docs team wanted to get away from installing proejct deps and loading project in the doc env a while ago. i dont know if that effort did or not but its part of the reason why we created a seperate doc/requirement.txt | |
| 16:27:25 | mdbooth | No, we weren't doing it before | |
| 16:27:40 | sean-k-mooney | monkeypatching? | |
| 16:28:07 | mdbooth | Previously we monkey patched in nova/test.py and nova/tests/functional/__init__.py and sphinx didn't hit that | |
| 16:28:16 | sean-k-mooney | right | |
| 16:28:24 | mdbooth | I moved the test monkey patching to nova/tests/__init__.py, and now it does hit that | |
| 16:28:35 | mdbooth | Go figure | |
| 16:28:53 | mdbooth | If anybody can explain that interface to me I'd be interested | |
| 16:28:56 | sean-k-mooney | does that fix it our recreate teh breakage? | |
| 16:29:11 | mdbooth | I can't reproduce the failure locally, even with monkey patching | |
| 16:29:34 | mdbooth | Ok, I'm just going to move the monkey patching points and add comments not to move them again | |
| 16:30:02 | mdbooth | I have no idea how you're supposed to understand what sphinx imports | |
| 16:30:31 | stephenfin | mdbooth: The whole thing is basically a hack, but there's no easy way to do it | |
| 16:33:40 | sean-k-mooney | stephenfin: the sane thing to do would be not import the nova source when building docs but that is an issue for generationg config docs if we want to do that with sphinx i guess | |
| 16:34:28 | stephenfin | sean-k-mooney: We either do it dynamically, like we do, or we have a tool to export config to a machine readable format (YAML) and parse that | |
| 16:34:50 | stephenfin | and if we go with the latter, updating those definitions now becomes a manual step meaning we need to add tests to ensure it doesn't get out of date | |
| 16:35:16 | mdbooth | stephenfin: But even then presumably the tool which generated the yaml would import the source with a python interpreter | |
| 16:35:21 | jangutter | sean-k-mooney: http://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html "Warning: autodoc imports the modules to be documented. If any modules have side effects on import, these will be executed by autodoc when sphinx-build is run." | |
| 16:35:25 | sean-k-mooney | there is prably an option 3 | |
| 16:35:28 | stephenfin | autodocing APIs is even more complicated since you do need a full environment to be able to do introspection | |
| 16:35:33 | mdbooth | stephenfin: So we'd just be moving the problem elsewhere | |
| 16:36:00 | stephenfin | mdbooth: Correct, but that's sometimes a reasonable thing to do | |
| 16:36:19 | sean-k-mooney | jangutter: ya im aware of how autodocs works. | |
| 16:36:44 | sean-k-mooney | jangutter: its both good and bad | |
| 16:37:07 | sean-k-mooney | jangutter: we have some module level stuff we caluate on import that is slow | |
| 16:37:19 | stephenfin | mdbooth: We can either store this stuff as code and deal with these dependency issues or we store it separately and then deal with keeping it up-to-date. Choose one | |
| 16:38:08 | stephenfin | (I'm very much in favour of the former, as noted by my work on (cliff|oslo_config|oslo_policy).sphinxext | |
| 16:38:11 | stephenfin | *) | |
| 16:38:12 | sean-k-mooney | mdbooth: did you determin if the issue is with monkey patching twice or not monkey patch at all or didnt find the root cause | |
| 16:38:14 | jangutter | sean-k-mooney: and the current theory is that autodoc and monkeypatching are fighting? | |
| 16:38:37 | mdbooth | sean-k-mooney: I couldn't reproduce, so I have no idea. | |
| 16:38:37 | sean-k-mooney | jangutter: i think we are possibly monkeypatching twice | |
| 16:38:47 | sean-k-mooney | mdbooth: i can repoduce locally | |
| 16:38:55 | mdbooth | sean-k-mooney: On Ubuntu? | |
| 16:38:59 | stephenfin | jangutter: Not necessarily autodoc. Probably oslo_config.sphinxext | |
| 16:39:04 | sean-k-mooney | yes.. | |
| 16:39:06 | stephenfin | Don't think we do autodoc | |
| 16:39:07 | jangutter | mdbooth: I reproduced on a CentOS 7 container. | |
| 16:39:28 | sean-k-mooney | ok so it cross distro | |
| 16:39:30 | jangutter | stephenfin: ah! | |
| 16:39:47 | stephenfin | mdbooth: Good reading, fwiw http://python-notes.curiousefficiency.org/en/latest/python_concepts/import_traps.html | |
| 16:39:49 | sean-k-mooney | mdbooth: we should proably open a bug and fix it later | |
| 16:39:56 | mdbooth | That seems to avoid whatever sphinx is importing | |
| 16:40:58 | mdbooth | sean-k-mooney: For what? | |
| 16:41:49 | stephenfin | mdbooth: It's loading the module. I suspect you'd get the same issue if you left it in 'nova/test.py' and added 'import .test' to 'nova/tests/__init__.py' | |
| 16:41:57 | stephenfin | *'import nova.test' | |
| 16:42:45 | mdbooth | stephenfin: Which module is it loading? | |
| 16:43:16 | stephenfin | It's loading 'nova', which is causing it to load 'nova.tests', I would imagine | |
| 16:43:26 | sean-k-mooney | mdbooth: https://review.openstack.org/#/c/626952/8/nova/monkey_patch.py@55 | |
| 16:43:38 | sean-k-mooney | mdbooth: does that comment make sense. | |
| 16:43:59 | sean-k-mooney | actull i test it locally | |
| 16:45:29 | stephenfin | and that's happening because of https://github.com/openstack/nova/blob/master/doc/source/conf.py#L27 | |
| 16:45:33 | mdbooth | sean-k-mooney: Right, if the problem is importing twice that would work round it. However, if we previously weren't monkey patching at all by happy coincidence, I'm not sure if that's not just a dirtier hack. | |
| 16:45:49 | stephenfin | though that's clearly not necessary https://github.com/openstack/nova/blob/master/doc/source/conf.py#L22 | |
| 16:46:55 | mdbooth | stephenfin: “Never add a package directory, or any directory inside a package, directly to the Python path”. | |
| 16:47:17 | mdbooth | From that traps doc you sent earlier | |
| 16:48:08 | stephenfin | yuuuup | |
| 16:48:23 | stephenfin | In general, messing with the Python path is a bad idea | |
| 16:48:39 | stephenfin | but the alternative is making all these things installable | |
| 16:48:44 | mdbooth | s/(the|path) // | |
| 16:49:01 | mdbooth | g | |
| 16:49:17 | stephenfin | :) | |
| 16:49:24 | stephenfin | I like to live on the edge, personally | |
| 16:50:46 | mdbooth | Ok, I'm going to do both | |
| 16:51:12 | mdbooth | I'm going to move the monkey patching points back to where they were, *and* add a guard to explicitly avoid double monkey patching | |
| 16:52:29 | stephenfin | mdbooth: I think you might also be interested in some of Armin Ronacher's most recent presentations on all things Python http://lucumr.pocoo.org/talks/ | |
| 16:52:39 | sean-k-mooney | mdbooth: i tryed the guard locally and it did not seam to help | |
| 16:52:40 | stephenfin | he of Flask fame | |
| 16:54:09 | stephenfin | ...openstack/nova/doc/source/reference/notifications.rst:142:Unknown directive type "automodule". | |
| 16:54:14 | stephenfin | So we are using autodoc | |
| 16:58:00 | stephenfin | mdbooth: Try adding this to doc/source/conf.py | |
| 16:58:07 | stephenfin | autodoc_mock_imports = ['nova.tests'] | |
| 16:59:21 | mdbooth | stephenfin: That's still really flaky, though, because it doesn't explain why we don't import the nova.cmd or nova.api.openstack.wsgi_app entry points | |
| 16:59:39 | mdbooth | stephenfin: I'd prefer to either go from flaky to non-flaky, or just leave well alone | |
| 17:00:09 | mdbooth | flaky->different flaky isn't worth the churn, I think | |
| 17:00:45 | sean-k-mooney | mdbooth: im flipped the condition on my local gard to never monkey patch and that fixes it | |
| 17:00:59 | sean-k-mooney | so the issue is the docs only build if we never monkeypatch | |
| 17:01:05 | stephenfin | Oh, I didn't realize we did that there | |
| 17:01:34 | sean-k-mooney | stephenfin: we monkey patch on the __init__.py in nova/cmd | |
| 17:01:40 | stephenfin | Yup, see that now | |
| 17:01:45 | stephenfin | weirdddd | |
| 17:02:03 | stephenfin | mdbooth: Can you share the debug logs from Sphinx? | |
| 17:02:19 | sean-k-mooney | ya anywya as i said above the issue is the sphinx only works if we do not do eventlet.monkeypatch | |
| 17:02:24 | mdbooth | stephenfin: In what configuration? | |
| 17:02:39 | mdbooth | stephenfin: FWIW, this doesn't fail for me on F29, even when monkey patched | |
| 17:03:02 | mdbooth | Although apparently it does fail on Centos7 and Ubuntu, so I'm guessing it's some library version | |
| 17:03:02 | stephenfin | Oh, wherever it's failing | |
| 17:03:11 | stephenfin | I assume that's only on Ubuntu so? | |
| 17:03:12 | stephenfin | Ack | |
| 17:03:36 | sean-k-mooney | stephenfin: jangutter mentioned its broken on centos 7 too | |
| 17:03:56 | sean-k-mooney | mdbooth: are you running on py36 on fedora 29 or py37 | |
| 17:04:07 | sean-k-mooney | ubuntu and centos 7 use py36 | |
| 17:04:16 | stephenfin | Yeah, we need debug logs for this stuff | |
| 17:04:19 | mdbooth | 37 | |
| 17:04:39 | sean-k-mooney | ok so it could be a 36 vs 37 thing | |
| 17:05:08 | openstackgerrit | Lance Bragstad proposed openstack/nova master: Give the policy vision document a facelift https://review.openstack.org/644615 | |
| 17:05:36 | sean-k-mooney | ill install 3.7 and check | |
| 17:05:54 | lbragstad | mriedem lumped it all in there ^ lemme know if you want that broken up | |
| 17:06:00 | lbragstad | cc efried ^ | |
| 17:07:19 | openstackgerrit | Stephen Finucane proposed openstack/nova master: docs: Misc cleanups https://review.openstack.org/644616 | |