Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-03-19
16:18:41 mdbooth stephenfin: I'd expect them to load the *whole* nova source though, right?
16:19:09 stephenfin mdbooth: What are they loading?
16:19:27 stephenfin eventlet monkeypatching?
16:19:32 stephenfin or monkeypatching of other things
16:19:43 sean-k-mooney mdbooth: we dont generate docs for the code directly so ingeneral i would have assuemd it should not load the nova code.
16:19:48 mdbooth stephenfin: eventlet. It's done just by loading certain modules.
16:20:04 mdbooth stephenfin: If we import everything, we do monkey patching eventually.
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

Earlier   Later