{"action":"created","issue":{"url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/1919","repository_url":"https://api.github.com/repos/pyinstaller/pyinstaller","labels_url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/1919/labels{/name}","comments_url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/1919/comments","events_url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/1919/events","html_url":"https://github.com/pyinstaller/pyinstaller/issues/1919","id":145315380,"number":1919,"title":"Something Wicked This Way Came to \"modulegraph\"","user":{"login":"leycec","id":217028,"avatar_url":"https://avatars.githubusercontent.com/u/217028?v=3","gravatar_id":"","url":"https://api.github.com/users/leycec","html_url":"https://github.com/leycec","followers_url":"https://api.github.com/users/leycec/followers","following_url":"https://api.github.com/users/leycec/following{/other_user}","gists_url":"https://api.github.com/users/leycec/gists{/gist_id}","starred_url":"https://api.github.com/users/leycec/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/leycec/subscriptions","organizations_url":"https://api.github.com/users/leycec/orgs","repos_url":"https://api.github.com/users/leycec/repos","events_url":"https://api.github.com/users/leycec/events{/privacy}","received_events_url":"https://api.github.com/users/leycec/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":6,"created_at":"2016-04-02T00:13:53Z","updated_at":"2016-05-01T07:05:10Z","closed_at":null,"body":"Bleary-eyed midnight salutations, fellow code spelunkers. All may not be well in the cavernous depths of the PyInstaller codebase. I am possibly the sad-faced pallbearer of ultrabad news.\r\n\r\n**PyInstaller might be a little b0rked.** I know, right? leycec crazy talk. But hear this angsty diatribe out.\r\n\r\n## How Bad Could It Possibly Be?\r\n\r\nUltrabad. Here's a minimal-length example (MLE):\r\n\r\n```\r\nimport scipy.special\r\n```\r\n\r\nThat's it. A one-liner noop only importing a single package. For:\r\n\r\n- SciPy < 0.15.0, PyInstaller should already freeze this example without modification.\r\n- SciPy >= 0.15.0, PyInstaller should theoretically freeze this example after merging #1909.\r\n\r\n**It doesn't do either.** To demonstrate this, #1909 adds a new [`test_scipy_special()`](https://github.com/pyinstaller/pyinstaller/pull/1909/commits/ef680f6c97b7c74b5c809bb30429b4cfce3284cc) test trivially exercising the above example as:\r\n\r\n```\r\ndef test_scipy_special(pyi_builder):\r\n    pyi_builder.test_source(\"\"\"import scipy.special\"\"\")\r\n```\r\n\r\nFor SciPy < 0.15.0, this test should succeed. For SciPy >= 0.15.0, this test should succeed after merging #1909 and otherwise fail with the traceback given at #1908 – a long-standing issue of an undetectable import in a SciPy C extension trivially fixed with a [new one-liner hook](https://github.com/pyinstaller/pyinstaller/pull/1909/commits/65a4873f0883d6b6f674b7131cc1443434e367d8). _Annnyway._\r\n\r\nThis test currently does none of those things. If you're still with me, you might be thinking: _\"Ah-ha! He's gonna complain about hooks, C extensions, shared libraries, or Windows. Those always break.\"_\r\n\r\n**Nope.** Disturbingly, this test does _not_ fail with a trivially fixable low-level platform- or package-specific error. It fails with a [high-level import error](https://travis-ci.org/pyinstaller/pyinstaller/jobs/119444786) causally implicating `ModuleGraph`. That's _never_ trivially fixable:\r\n\r\n```\r\nLOADER: Running pyiboot01_bootstrap.py\r\nLOADER: Running pyi_rth_pkgres.py\r\nLOADER: Running test_scipy_special.py\r\nFailed to execute script test_scipy_special\r\nLOADER: OK.\r\nLOADER: Cleaning up Python interpreter.\r\nTraceback (most recent call last):\r\n  File \"/tmp/pytest-of-travis/pytest-0/popen-gw2/test_scipy_special_onedir_0/test_scipy_special.py\", line 2, in <module>\r\n    import scipy.special\r\n  File \"/home/travis/build/pyinstaller/pyinstaller/PyInstaller/loader/pyimod03_importers.py\", line 389, in load_module\r\n    exec(bytecode, module.__dict__)\r\n  File \"scipy/special/__init__.py\", line 629, in <module>\r\n  File \"/home/travis/build/pyinstaller/pyinstaller/PyInstaller/loader/pyimod03_importers.py\", line 389, in load_module\r\n    exec(bytecode, module.__dict__)\r\n  File \"scipy/special/basic.py\", line 18, in <module>\r\n  File \"/home/travis/build/pyinstaller/pyinstaller/PyInstaller/loader/pyimod03_importers.py\", line 389, in load_module\r\n    exec(bytecode, module.__dict__)\r\n  File \"scipy/special/orthogonal.py\", line 101, in <module>\r\nImportError: cannot import name 'linalg'\r\nLOADER: Back to parent (RC: 255)\r\nLOADER: Doing cleanup\r\nLOADER: Freeing archive status for /tmp/pytest-of-travis/pytest-0/popen-gw2/test_scipy_special_onedir_0/dist/test_scipy_special/test_scipy_special\r\n```\r\n\r\n## I Don't See a Problem Here\r\n\r\nOkay, everybody. Put on your corpse-handling gloves. The stacktrace above implies the import chain below:\r\n\r\n1. The pure-Python `test_scipy_special()` test attempts to `import scipy.special`.\r\n1. The pure-Python `scipy.special.__init__` module attempts to `from .basic import *`.\r\n1. The pure-Python `scipy.special.basic` module attempts to `from . import orthogonal`.\r\n1. The pure-Python `scipy.special.orthogonal` module attempts to `from scipy import linalg`.\r\n1. The pure-Python `scipy.linalg` package was _not_ frozen by PyInstaller, resulting in the destruction of all that is good and pure.\r\n\r\n**This import chain should be trivially parsable by `ModuleGraph`.** No dynamic `__import__()` calls or package `__path__` modifications are performed by any of the modules or packages in this chain. No C extensions, shared libraries, Cython modules, or other unparsable shenanigans are imported within this chain. It's all static pure-Python `import` statements. <sup>_I could be wrong. I'm often wrong. I hope I'm wrong._</sup>\r\n\r\nTo muddy the waters even more, this import chain _is_ parsable by `ModuleGraph` when the `import scipy.special` statement is moved into the existing `test_scipy()` test. It only fails when this import is performed in a stand-alone test. _wat?_\r\n\r\n`ModuleGraph` is the stable bedrock on which all PyInstaller behaviour depends. It no longer appears to be capable of parsing a simple import chain under certain edge cases. Maybe it never was.\r\n\r\n## Should I Be Concerned Yet?\r\n\r\nI'm sweating bullets myself. (_Kinda hurts._)\r\n\r\nI'm hoping this test passed without modification under older versions of PyInstaller – at least for Python <= 2.7 and SciPy < 0.15.0. Since we didn't have a test exercising this failure [until now](https://github.com/pyinstaller/pyinstaller/pull/1909/commits/ef680f6c97b7c74b5c809bb30429b4cfce3284cc), it's all conjecture without manually going back in time or investing time in testing older releases. Sadly, I'm out of flux capacitors.\r\n\r\nIf this used to work, then PyInstaller's capacity to freeze applications has significantly degraded. The above example is trivial. PyInstaller no longer freezes it. What else can PyInstaller no longer freeze?\r\n\r\nIf this never worked, then PyInstaller's capacity to freeze at least trivial applications is in question. Which is also kind-of bad. Just not quite as bad.\r\n\r\nIn either case, badness follows.\r\n\r\n## How Do You Know It Was Us? Maybe It Was Ron!\r\n\r\nBlaming [Ron](https://bitbucket.org/ronaldoussoren/modulegraph) is my modus operandi. For better or worse, Ron probably didn't do this. In fact, Ron appears to have abandoned `modulegraph`. Which makes me feel bad.\r\n\r\nI could be wrong. It's feasible that `ModuleGraph` has always been broken in this respect. That would be great. But also not, because I have no idea how to repair that. Which makes me feel bad yet again.\r\n\r\n## How Do You Know Anything?\r\n\r\nI don't, O.K.? That's the trouble with code tribbles.\r\n\r\nTo decipher this dessicated dungheap, I first enabled `ModuleGraph` debugging via `MODULEGRAPH_DEBUG=4`. A suspicious line reared up on its creepy hindlegs:\r\n\r\n```\r\n                   Global name found: scipy linalg\r\n```\r\n\r\n_Heywaitaminute!_ That's the name of the erroneously omitted package: `scipy.linalg`. That's also debug output recently added by us, which I'd never seen before... **until now.** The lugubrious plot thickens.\r\n\r\nThe logic responsible for this output lives in two related private `ModuleGraph` methods – `_ensure_fromlist()` and `_safe_import_hook()`. Note the suspicious strings `'Global name found:'` in this code snippet:\r\n\r\n```\r\nclass ModuleGraph(Graph):\r\n    ...\r\n\r\n    def _ensure_fromlist(self, m, fromlist):\r\n        ...\r\n\r\n        for sub in fromlist:\r\n            submod = m.get(sub)\r\n            if submod is None:\r\n                if sub in m.globalnames:\r\n                    # Name is a global in the module\r\n                    self.msg(4, 'Global name found:', m.identifier, sub)\r\n                    continue\r\n        ...\r\n\r\n    def _safe_import_hook(self, name, caller, fromlist, level=DEFAULT_IMPORT_LEVEL, attr=None):\r\n        ...\r\n\r\n        for sub in (fromlist or ()):\r\n            # If this name is in the module namespace already,\r\n            # then add the entry to the list of substitutions\r\n            if sub in m:\r\n                ...\r\n\r\n            elif sub in m.globalnames:\r\n                # Global variable in the module, ignore\r\n                self.msg(4, 'Global name found:', m.identifier, sub)\r\n                continue\r\n        ...\r\n```\r\n\r\nThe `Node.globalnames` set appears to be implicated. Plot thickens more. I grepped about for other instances of `globalnames` and discovered this:\r\n\r\n```\r\nclass ModuleGraph(Graph):\r\n    ...\r\n\r\n    def _scan_code(self, m, co, co_ast=None):\r\n        ...\r\n\r\n        # Actually import the modules collected while scanning.\r\n        # We need to suspend the globalnames as otherwise\r\n        # `_safe_import_hook()` would take identfiers imported via\r\n        # `from . import abc` as existing global names and not try to\r\n        # import them. Please note: This only effects this form of\r\n        # import where relative_module is \".\" For all other cases, the\r\n        # imported module is already processed and all global names\r\n        # are in place anyway.\r\n        globalnames = m.globalnames\r\n        m.globalnames = set()\r\n        self._process_imports(m)\r\n        m.globalnames = globalnames\r\n```\r\n\r\n_Hey! **Hey!**_ What in the two-headed hydra of Hades is going on here? Bar none, that's the most suspiciously hacky code I've seen in months. And I've personally authored several decades worth of hacky code in the past month alone. <sup>_Don't tell my wife._</sup>\r\n\r\nAgain, the shameless hackage above has been recently added by us. I've never seen it before.\r\n\r\n## Explain It To Me Like I'm Intellectually Five\r\n\r\nI'm intellectually five, too. I can't explain anything.\r\n\r\nAlso, `ModuleGraph` is a classic [wicked problem](https://en.wikipedia.org/wiki/Wicked_problem). Between Ron's undocumented older functionality, the undocumented newer functionality we've added, and the sad passage of Time, I no longer have a firm grip on `ModuleGraph` behaviour. <sup>...or my own, thanks to this issue.</sup>\r\n\r\nI do have unsubstantiated suppositions, however. Let's take those one non-Euclidean dimension at a time.\r\n\r\n### `ModuleGraph`: The Teenage Years\r\n\r\nA ripe cornucopia of new functionality I've never seen before has been added to `ModuleGraph`, including:\r\n\r\n* A new `BaseModule._imported_modules` list.\r\n* A new `_Visitor._collect_import()` method.\r\n* A new `ModuleGraph._process_imports()` method.\r\n* A new `ModuleGraph.createNode()` method.\r\n\r\nThe new `BaseModule._imported_modules` list appears to be the linchpin tying this new functionality together, which appears to be trying to defer imports performed by `ModuleGraph._scan_*()` methods until _after_ those methods return, which may not behave as expected under all edge cases. Like this one.\r\n\r\n### `ModuleGraph` Invariants Have Been B0rked (_Maybe_)\r\n\r\n**`ModuleGraph` invariants.** Remember those? Neither do I.\r\n\r\nThe idea is that all recursively called functions in a recursive algorithm have formal, contractual invariants that should _never_ be broken. We might've broken them. Maybe.\r\n\r\n> **Caveat emptor:** Everything that follows is pure conjecture based on the seemingly reasonable assumption that `ModuleGraph` used to behave sanely. I did actually try testing the above example under older versions of PyInstaller, but... I failed and decided go to sleep. It was the smart move.\r\n\r\nMy assumption is that older versions of `ModuleGraph` did _not_ add attributes imported from other modules and packages to the graph. Imported attributes were simply ignored. This makes sense. Imported attributes are module-scope types and variables. Nobody using `ModuleGraph` cares about module-scope types and variables. Everybody only cares about imports between modules and packages.\r\n\r\nIgnoring imported attributes is exactly what the `if sub in m.globalnames:` branches in the `_ensure_fromlist()` and `_safe_import_hook()` methods detailed above were attempting to do. Hence, this assumption appears reasonable.\r\n\r\nIf this assumption used to hold, it doesn't anymore. Most (but _not_ all) imported attributes are now added to the graph as `MissingModule` nodes rather than being ignored. This is unspeakably bad – but I shall speak of it. Since attributes are _not_ modules, adding attributes to the graph:\r\n\r\n* **Breaks `ModuleGraph` semantics.** What would an outgoing edge from an attribute node even mean, for example? Attributes aren't namespaces. They can't import anything. Moreover, `MissingModule` nodes are supposed to inform PyInstaller about... missing modules, _not_ ignorable attributes that actually exist.\r\n* **Increases average graph size** with predictable decreases in performance. I haven't profiled anything, because lazy – but it's easy to see how a graph obesity epidemic would be bad for space and time complexity. Most modules import multiple attributes, most of which are now encapsulated as `MissingModule` nodes unidirectionally connected to their parent module nodes.\r\n* **Invites namespace conflicts** between ignorable attributes and non-ignorable submodules and subpackages of the same name. What happens, for example, when a module `foo` imports a submodule `bar` and then reassigns `bar` to be a variable? Python 2 and 3 both permissively permit this. What's `bar`, in this case? Is it a non-ignorable imported submodule _or_ an ignorable attribute? It's both at different times, of course, but what does `ModuleGraph` think it is? For example, suppose module `foo` contains:\r\n\r\n        from . import bar\r\n        bar = 3.1415\r\n\r\n* **Serves zero purpose.** PyInstaller doesn't care about imported attributes. Neither should `ModuleGraph`, except to explicitly ignore imported attributes. Which it no longer does.\r\n\r\n### `ModuleGraph` Attribute Detection Has Been B0rked, Too\r\n\r\nMore importantly, the `if sub in m.globalnames:` branches in the `_ensure_fromlist()` and `_safe_import_hook()` methods that I assume once conditionally ignored imported attributes now either:\r\n\r\n1. Always evaluate to `False` when those methods are called by `_scan_code()` for the currently scanned module. This is usually the case, so ignorable attribute imports are usually no longer ignored.\r\n1. Sometimes evaluate to `True` when those methods are called by `_scan_code()` for modules other than the currently scanned module. This is _not_ usually the case, but it sometimes is. So, non-ignorable module and package imports are occasionally misdetected as ignorable attribute imports now.\r\n\r\nThe latter case is what's happening in the `test_scipy_special()` test. The non-ignorable import of the `scipy.linalg` package is misdetected as an ignorable import of a non-existent `scipy.linalg` attribute. It's _not_ an attribute. It's a package. But `ModuleGraph` no longer knows the difference – if it ever did.\r\n\r\nWhy did this happen? Possibly because `_scan_code()` has been hacked to temporarily nullify the set of all attributes imported by the current module (i.e., `Node.globalnames`), preventing `_ensure_fromlist()` and `_safe_import_hook()` from ignoring imported attributes. My assumption is that, previously, those methods could reliably distinguish ignorable imported attributes from non-ignorable imported modules. That no longer happens – if it ever did.\r\n\r\n## Why? Oh Nordic Gods Below, _Why?_\r\n\r\nA myriad of ugly questions jostle for attention in my ADHD-addled brainpan. The most stunningly obvious is: **\"Why?\"**\r\n\r\nWhy are we trying to delay imports in `_scan_code()` and elsewhere? What's going on here? Is all of this really necessary? Because it may not work. Is this preparatory work for future work to be done – say, an attempt to refactor `ModuleGraph` to use an iterative rather than recursive algorithm for parsing imports? If so, I applaud such virtuous forward-thinking entrepeneurship. That whole maybe-non-working thing puts a damper on my enthusiam, however.\r\n\r\nIs delaying imports the cause of this issue? This superficially appears to be the cause, but I'm definitely open to being stupid and wrong. Maybe this never worked and `ModuleGraph` was always discouragingly broken.\r\n\r\nIf delaying imports _is_ the cause and we want to valiantly trek on with this new functionality, that's cool. Let's trek! But only if we get this to work without brute-force hackery attempting (...but failing) to circumvent our having broken `ModuleGraph` invariants. The `ModuleGraph._scan_code()` changes, which appear to only mask rather than solve underlying issues caused by this new functionality, are especially disconcerting.\r\n\r\nTwo obvious solutions present themselves.\r\n\r\n### Temporary Fix for Maximum Worky\r\n\r\nAs a merely temporary fix just getting this to work right now without any nauseating delay, we _could_:\r\n\r\n* Comment out the entire `if sub in m.globalnames:` branch in `_ensure_fromlist()`.\r\n* Comment out the entire `elif sub in m.globalnames:` branch in `_safe_import_hook()`.\r\n\r\nAfter disabling both of these branches, I can confirm that the `test_scipy_special()` test now locally passes. Both must be disabled. It does _not_ suffice to disable merely one.\r\n\r\nSince the `_ensure_fromlist()` and `_safe_import_hook()` methods can no longer reliably distinguish ignorable imported attributes from non-ignorable imported modules, the easiest hack-on-a-hack is to stop ignoring attributes altogether. That's what this fix does. It's a poor long-term solution, however. Ignorable imported attributes _should_ be ignored. <sup>_Assuming `ModuleGraph` could ever do that, that is. So many assumptions._</sup>\r\n\r\n### Ideal Fix for Maximum Happy\r\n\r\nIf delaying imports is the cause, my preference would – with a tip of the fedora and a sad apology to the original author – be to revert our new functionality. But maybe it's actually awesome and we should work hard to preserve somebody's hard work? I don't know. Convince me, convincingly!\r\n\r\n### No Fix for Maximum Confused\r\n\r\nIf delaying imports is _not_ the cause, `ModuleGraph` was always broken in this respect. That's hard to fathom. Detecting simple import chains is core functionality... but my bloodshot third eye is open to the possibility that everything we thought worked never _truly_ worked.\r\n\r\nI don't know what the optimum solution to this worst-case scenario would be. Maybe just rip out the `Node.globalnames` attribute and all functionality related to that attribute? If detecting attribute imports is actually infeasible (_...as it might be_), we need to stop trying entirely.\r\n\r\nI have no working code and I must scream. Help me, Odin. Help me.\r\n\r\n![McBain: \"MENDOZAAAAAAA!!!!\"](https://deadhomersociety.files.wordpress.com/2011/02/saturdaysofthunder6_thumb.png?w=512&h=384)"},"comment":{"url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/comments/216021150","html_url":"https://github.com/pyinstaller/pyinstaller/issues/1919#issuecomment-216021150","issue_url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/1919","id":216021150,"user":{"login":"leycec","id":217028,"avatar_url":"https://avatars.githubusercontent.com/u/217028?v=3","gravatar_id":"","url":"https://api.github.com/users/leycec","html_url":"https://github.com/leycec","followers_url":"https://api.github.com/users/leycec/followers","following_url":"https://api.github.com/users/leycec/following{/other_user}","gists_url":"https://api.github.com/users/leycec/gists{/gist_id}","starred_url":"https://api.github.com/users/leycec/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/leycec/subscriptions","organizations_url":"https://api.github.com/users/leycec/orgs","repos_url":"https://api.github.com/users/leycec/repos","events_url":"https://api.github.com/users/leycec/events{/privacy}","received_events_url":"https://api.github.com/users/leycec/received_events","type":"User","site_admin":false},"created_at":"2016-05-01T07:05:10Z","updated_at":"2016-05-01T07:05:10Z","body":"**OHGODSOHGODSOHGODS.** I just uncovered the core issue – and it truly is ultra-lame-o. Anyone recall where I [suggested above](https://github.com/pyinstaller/pyinstaller/issues/1919#issue-145315380) that our `ModuleGraph._scan_code()` kludge:\r\n\r\n> **Invites namespace conflicts** between ignorable attributes and non-ignorable submodules and subpackages of the same name. What happens, for example, when a module `foo` imports a submodule `bar` and then reassigns `bar` to be a variable? Python 2 and 3 both permissively permit this. What's `bar`, in this case? Is it a non-ignorable imported submodule or an ignorable attribute? It's both at different times, of course, but what does ModuleGraph think it is? For example, suppose module `foo` contains:\r\n\r\n```\r\nfrom . import bar\r\nbar = 3.1415\r\n```\r\n\r\nThe answer to my query \"...but what does ModuleGraph think it is?\" is worst-case horrible, of course: **ModuleGraph thinks `bar` is an ignorable attribute.** I never actually envisioned any module _actually_ doing this, but that's exactly what `scipy.__init__` is doing. <sup>I think, anyway.</sup>\r\n\r\nLine 68 of  `scipy.__init__` defines a global `linalg = None`. Catastrophically, `linalg` is _also_ a subpackage of `scipy` (i.e., `scipy.linalg`). ModuleGraph resolves this namespace conflict in the stupidest possible way, treating `linalg` as a global variable rather than a subpackage.\r\n\r\nTo resolve this, we absolutely _do_ need to:\r\n\r\n1. Patch the `ModuleGraph._ensure_fromlist()` method as [suggested above](https://github.com/pyinstaller/pyinstaller/issues/1919#issuecomment-216020047) to differentiate non-ignorable submodules from ignorable attributes.\r\n1. Patch the `ModuleGraph._safe_import_hook()` method in a similar way.\r\n1. Remove the `ModuleGraph._scan_code()` kludge.\r\n\r\nTune in next time as leycec possibly does something."}}