Opened 4 years ago

Closed 4 years ago

#20729 closed defect (fixed)

doctest runner breaks loading modules from packages that use relative imports

Reported by: embray Owned by:
Priority: minor Milestone: sage-7.6
Component: doctest framework Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 13d4a62 (Commits) Commit: 13d4a62814d5048884553385412b4285f3fa5fbd
Dependencies: Stopgaps:

Description

I encountered this issue when trying to run the doctests in a new sub-package I added to sage_setup, in which I used relative imports between the submodules. Trying to run the doctests on these modules blows up with: ValueError: Attempted relative import in non-package

In this particular case I was able to work around this by fiddling with FileDocTestSource.in_lib to include all of sage_setup (previously it only contained sage_setup.docbuild--I can't tell you how much hair I pulled out trying to figure out why the relative imports in sage_setup.docbuild were not a problem :)

But in general this won't do if someone wants to test their own package using Sage's doctest runner, if it happens to use relative imports.

The cause of this error is an implementation detail of how relative imports work in Python. They are only meant to work in modules that belong to a package. This means the module must have either a correct __package__ or a __path__ attribute as it would if it were imported through the normal import system.

In the doctest runner, FileDocTestSource.create_doctests calls sage.repr.load.load on the module being tested if it is not in_lib, which in turn compiles and execs that module in a namespace with __name__ = '__main__'. This won't do when trying to exec a module that belongs to a package if it uses relative imports.

And even if it doesn't use relative imports, but uses absolute imports, it may not import other modules in the same package from the correct path. This statement deserves further explanation: Say I have a package:

foo/
    __init__.py
    a.py
    b.py

and __init__.py contains:

import foo.a
import foo.b

these are perfectly good absolute imports. Let's also say I have the foo package under mysrc/foo. If I then run ./sage -t mysrc/foo/__init__.py it will load() __init__.py as a stand-alone module. The imports in it might work if I happen to have mysrc/ in my sys.path, or if I have installed some version of the foo package into my site-packages. The latter case is especially bad because it means I'm testing mysrc/foo/__init__.py in my source tree, but it's importing foo.a and foo.b from an installed version.

The best workaround, though annoying to do, is when exec'ing a module that belongs to a package one needs to "set up" the environment it execs in as though it were imported as part of that package, through the normal import system. This can mean several things. Depending on what happens in the package's __init__.py (and the __init__.py of any super-packages if it is a sub-package) it will mostly likely be necessary to import those packages for the module to run properly at all.

So the thing to do is walk the entire package hierarchy for the module by looking up the directory tree for __init__.pys (this may fail to find PEP-420 namespace packages, but that's probably not a problem) and import each package in the hierarchy (making sure to add the package's path to sys.path).

Change History (10)

comment:1 Changed 4 years ago by jdemeyer

Python's doctest module seems to suffer from the same issue. From https://docs.python.org/2/library/doctest.html#simple-usage-checking-examples-in-docstrings:

python -m doctest -v example.py

This will import example.py as a standalone module and run testmod() on it. Note that this may not work correctly if the file is part of a package and imports other submodules from that package.

comment:2 follow-up: Changed 4 years ago by jdemeyer

Maybe we should just never load the module, i.e. behave as if in_lib is always True?

comment:3 in reply to: ↑ 2 Changed 4 years ago by jdemeyer

Replying to jdemeyer:

Maybe we should just never load the module, i.e. behave as if in_lib is always True?

Although that would break testing stand-alone single files, which we should probably support.

So a slightly better idea: only load the file if the file being tested is not in a package (i.e. __init__.py does not exist).

comment:4 Changed 4 years ago by embray

Another related problem is that all modules listed in in_lib make the assumption that the sage package has been imported, and that all global variables in sage are available (and don't need to be imported by the tests). This is not necessarily true for other packages, which is fine, but it might be unclear to someone writing doctests that this is only a special case for sage itself (or technically, anything listed in in_lib).

For packages one could, in principle, import the package, and then remove all of its modules from sys.modules (the end result is effectively the same as if you load()ed each module individually. Though I don't know what the advantage would be.

comment:5 Changed 4 years ago by jdemeyer

What do you think of 3? Load a file if and only if it is not part of a package (i.e. has no __init__.* file)

That would be very simple to implement and would probably do the right thing in most use cases.

comment:6 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/doctest_runner_breaks_loading_modules_from_packages_that_use_relative_imports

comment:7 Changed 4 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to 13d4a62814d5048884553385412b4285f3fa5fbd
  • Milestone changed from sage-7.3 to sage-7.6
  • Status changed from new to needs_review

I just implemented that.


New commits:

13d4a62Doctesting: never import files which are part of a package

comment:8 Changed 4 years ago by embray

  • Status changed from needs_review to positive_review

I'm honestly not sure how this will work on Python 3, where __init__.py is not required for packages. But I think in Python 3 relative imports are also changed so that loading a module directly (even if it's in a package) will still work properly. Without a specific example in mind, I suppose it's not worth worrying about right now.

This makes sense to me otherwise.

comment:9 Changed 4 years ago by jdemeyer

  • Reviewers set to Erik Bray

comment:10 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/doctest_runner_breaks_loading_modules_from_packages_that_use_relative_imports to 13d4a62814d5048884553385412b4285f3fa5fbd
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.