use PEP 420: implicit namespace package #990

emlys · 2022-05-17T00:27:31Z

Description

Fixes #967

Removed src/natcap/__init__.py to make natcap an implicit namespace package
Removed the deprecated namespace_packages parameter to setup. Since we already list out all packages, I don't think we need to change anything else: https://setuptools.pypa.io/en/latest/userguide/package_discovery.html
Update doc/api-docs/conf.py to work with the implicit namespace package, and some minor refactoring:
- Update the text at the top of the model entrypoints page
- Build the list of model entrypoints from the installed location of natcap.invest, not the build/lib* location
- Name the model entrypoints doc sections after the model title, not the first line of the execute docstring

Checklist

~~Updated HISTORY.rst (if these changes are user-facing)~~
~~Updated the user's guide (if needed)~~
~~Tested the affected models' UIs (if relevant)~~

emlys · 2022-05-17T20:35:51Z

doc/api-docs/conf.py

-        module = importlib.import_module(name)
-    except Exception as ex:
-        print(ex)
-        continue


Removed this because I think we want to raise an error if a module doesn't import.

I agree, better to fail loudly.

emlys · 2022-05-24T00:21:48Z

It turns out that this has some implications for our package data (currently just the placeholder directories for translation catalogs). See discussion: pypa/setuptools#3323

PEP 420 is standard and the python interpreter looks for namespace packages by default
By design, any directory within a package is also a package, even if it's empty or contains no python files.
Therefore, any "data" directories included in a package are really packages
natcap.invest.internationalization, natcap.invest.internationalization.locales, and natcap.invest.internationalization.locales.en are packages. The interpreter already finds them:

>>> import natcap.invest.internationalization.locales.en
>>> natcap.invest.internationalization.locales.en.__name__
'natcap.invest.internationalization.locales.en'

~~Therefore, we should stop trying to include them as "data" (with the package_data kwarg) and instead let them be packages.~~

I'll deal with this separately in #644.

emlys · 2022-07-12T23:23:56Z

doc/api-docs/conf.py

@@ -77,7 +69,7 @@
 #
 # This is also used if you do content translation via gettext catalogs.
 # Usually you set "language" from the command line for these cases.
-language = None
+language = 'en'


This was causing an error in the ReadTheDocs build, probably due to a sphinx update.

emlys · 2022-07-12T23:41:05Z

@phargogh this is finally ready! The RTD build issues fixed themselves in time.

phargogh

Very exciting! I just had a few very small comments and suggestions, but overall this is looking good!

phargogh · 2022-07-13T23:34:21Z

doc/api-docs/conf.py

 INVEST_ROOT_DIR = os.path.join(DOCS_SOURCE_DIR, '..', '..')
 INVEST_BUILD_DIR = os.path.join(INVEST_ROOT_DIR, 'build')


Are these used anywhere else now that we're properly defining INVEST_LIB_DIR? Might they be able to be removed?

You're right, they can be removed

doc/api-docs/conf.py

phargogh · 2022-07-13T23:38:59Z

doc/api-docs/conf.py

-        module = importlib.import_module(name)
-    except Exception as ex:
-        print(ex)
-        continue


I agree, better to fail loudly.

phargogh · 2022-07-13T23:42:47Z

doc/api-docs/conf.py

    '_core',  # anything ending in '_core'
    'recmodel_server',
-    'recmodel_workspace_fetcher'
+    'recmodel_workspace_fetcher',
+    'natcap.invest.ui'


Based on the RTD-rendered docs (https://invest--990.org.readthedocs.build/en/990/api.html for this PR) it looks like maybe this exclusion isn't working. Maybe it never worked? In any case, it would probably be nice to exclude packages, or else just have everything in there since it is in the source code.

I think this is working as expected - it's excluding those from the model entrypoints file models.rst. But they're still in the API docs.
The list of excluded modules was unnecessary, though. I refactored so that any module with an ARGS_SPEC is included in models.rst.

phargogh · 2022-07-13T23:43:33Z

doc/api-docs/conf.py

+    # all modules with an execute function should have an ARGS_SPEC
+    model_title = module.ARGS_SPEC['model_name']
+    all_modules[model_title] = name


This is so much better than our prior reliance on module-level docstrings!

phargogh · 2022-07-13T23:47:02Z

setup.py

-    namespace_packages=['natcap'],
-    packages=[
-        'natcap',
-        'natcap.invest',
-        'natcap.invest.coastal_blue_carbon',
-        'natcap.invest.delineateit',
-        'natcap.invest.ui',
-        'natcap.invest.ndr',
-        'natcap.invest.sdr',
-        'natcap.invest.recreation',
-        'natcap.invest.scenic_quality',
-        'natcap.invest.seasonal_water_yield',
-    ],
+    packages=find_namespace_packages('src'),


Great! It was not immediately clear to me from this function name that normal packages within natcap.invest would also be captured, but that does seem to be the behavior.

Yeah, it's confusing! pypa/setuptools#3427

phargogh · 2022-07-13T23:47:55Z

doc/api-docs/conf.py

+from datetime import datetime
 import importlib
 import itertools
 import os
 import pkgutil
 import subprocess
 import sys
-
-from datetime import datetime
-from sphinx.ext import apidoc
 from unittest.mock import MagicMock

+import natcap.invest
+from sphinx.ext import apidoc


Could you just check these imports and make sure that we aren't importing anything we no longer need?

You're right, a couple of them aren't needed any more

Co-authored-by: James Douglass <jamesdouglassusa@gmail.com>

emlys · 2022-07-14T17:18:31Z

@phargogh I'd like to squash these commits before merging

phargogh

Looks great, thanks@emlys !

emlys added 2 commits May 16, 2022 17:27

use PEP 420: implicit namespace package

2310a4a

update api docs config for namespace package

6e4ab26

emlys commented May 17, 2022

View reviewed changes

allow internationalization data to be included as a namespace 'package'

e59031c

emlys added 5 commits May 23, 2022 17:22

missing import

cd6dd4e

sphinx apidoc use implicit namespaces option natcap#967

729af04

restore setup package_data

396bb6e

update docs requirements to keep up with other requirements files

428e27b

set language to en

4fa015e

emlys commented Jul 12, 2022

View reviewed changes

emlys marked this pull request as ready for review July 12, 2022 23:40

emlys requested a review from phargogh July 12, 2022 23:40

emlys self-assigned this Jul 12, 2022

phargogh requested changes Jul 13, 2022

View reviewed changes

emlys and others added 2 commits July 14, 2022 10:01

remove unused imports, refactor excluded modules

4b7d954

Update doc/api-docs/conf.py

78bb8e1

Co-authored-by: James Douglass <jamesdouglassusa@gmail.com>

emlys requested a review from phargogh July 14, 2022 17:18

phargogh approved these changes Jul 14, 2022

View reviewed changes

phargogh merged commit 05525e9 into natcap:main Jul 14, 2022

emlys deleted the task/967 branch March 8, 2023 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use PEP 420: implicit namespace package #990

use PEP 420: implicit namespace package #990

emlys commented May 17, 2022 •

edited

emlys May 17, 2022

phargogh Jul 13, 2022

emlys commented May 24, 2022 •

edited

emlys Jul 12, 2022

emlys commented Jul 12, 2022

phargogh left a comment

phargogh Jul 13, 2022

emlys Jul 14, 2022

phargogh Jul 13, 2022

phargogh Jul 13, 2022

emlys Jul 14, 2022

phargogh Jul 13, 2022

phargogh Jul 13, 2022

emlys Jul 14, 2022

phargogh Jul 13, 2022

emlys Jul 14, 2022

emlys commented Jul 14, 2022

phargogh left a comment

		INVEST_ROOT_DIR = os.path.join(DOCS_SOURCE_DIR, '..', '..')
		INVEST_BUILD_DIR = os.path.join(INVEST_ROOT_DIR, 'build')

use PEP 420: implicit namespace package #990

use PEP 420: implicit namespace package #990

Conversation

emlys commented May 17, 2022 • edited

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlys commented May 24, 2022 • edited

Choose a reason for hiding this comment

emlys commented Jul 12, 2022

phargogh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlys commented Jul 14, 2022

phargogh left a comment

Choose a reason for hiding this comment

emlys commented May 17, 2022 •

edited

emlys commented May 24, 2022 •

edited