| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-19 | |||
| 16:20:20 | stephenfin | We can monkeypatch the monkeypatch though... | |
| 16:20:32 | stephenfin | if that was actually what was happening | |
| 16:20:44 | stephenfin | stub out eventlet, so to speak | |
| 16:20:45 | sean-k-mooney | stephenfin: ew | |
| 16:21:09 | stephenfin | sean-k-mooney: That comment also applies to eventlet itself ;) | |
| 16:21:12 | mdbooth | stephenfin: If it was acceptable we could define an environment variable which, if set, would disable monkey patching | |
| 16:21:19 | mdbooth | However, I'd assumed that was not acceptable | |
| 16:21:28 | jangutter | it would be really funny if rendering the docs accidentally caused an entire Nova deployment to spring up. | |
| 16:21:28 | stephenfin | you'd have to load the os module to do that | |
| 16:21:44 | stephenfin | which I assume eventlet needs to monkeypatch | |
| 16:21:45 | sean-k-mooney | we should not be executing any of the code so we should not need to monkeypatch as monkeypatch should never change any of the api signiture and therefor should nto change the docs | |
| 16:21:51 | sean-k-mooney | in the eventlet case | |
| 16:22:05 | mdbooth | sean-k-mooney: coulda shoulda isn't relevant, though, as it's done at import time | |
| 16:22:09 | sean-k-mooney | mdbooth: we do that for debug right | |
| 16:22:27 | stephenfin | sean-k-mooney: Any code outside a function and not within a 'if __name__ == "__main__"' conditional is going to be executed tho | |
| 16:22:37 | mdbooth | sean-k-mooney: that's done with config | |
| 16:22:56 | mdbooth | sean-k-mooney: Actually by inspecting command line arguments | |
| 16:23:01 | sean-k-mooney | yes but you can now set config values via the environment | |
| 16:23:15 | sean-k-mooney | its new in a recent oslo config | |
| 16:23:34 | mdbooth | def enabled(): | |
| 16:23:34 | mdbooth | return ('--remote_debug-host' in sys.argv and | |
| 16:23:35 | mdbooth | '--remote_debug-port' in sys.argv) | |
| 16:23:42 | mdbooth | ^^^ is the guard | |
| 16:23:50 | sean-k-mooney | yes for debug | |
| 16:24:34 | sean-k-mooney | anyway stephenfin do you know what we use from nova beyond the version and config options for our docs. is that it or do we use other parts of the tree? | |
| 16:24:55 | stephenfin | I'm trying to figure out if we still generate API docs. I don't think we do | |
| 16:25:32 | mdbooth | It's safe to say that this interface is hairy AF, though | |
| 16:25:59 | mdbooth | That is, even if we weren't doing it, somebody only had to sneeze and we were. | |
| 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 | sean-k-mooney | jangutter: i think we are possibly monkeypatching twice | |
| 16:38:37 | mdbooth | sean-k-mooney: I couldn't reproduce, so I have no idea. | |
| 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'] | |