Use setuptools.find_packages() in setup.py (was: Prepend bin/ to PATH in test build runner script)

[elebow]

Description of proposed changes

This causes the command augur (invoked by snakemake) to always be the dev helper script bin/augur when running the test builds. No functional changes to the app.

Related issue(s)

none

Testing

The test build script should run on systems where augur is not even installed—only the repo needs to be checked out. You can simulate this by making sure augur is not in your PATH and trying the test build runner.

Edit: cherry-picking this change into #526 allowed that build to pass. Now that I think about it, I'm not sure how the test builds ever worked without this. What augur executable is snakemake invoking in the Travis container? Is it a Conda thing?

[codecov]

Codecov Report

Merging #527 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
+ Coverage   18.65%   18.68%   +0.02%     
==========================================
  Files          31       31              
  Lines        5061     5053       -8     
  Branches     1282     1281       -1     
==========================================
  Hits          944      944              
+ Misses       4090     4082       -8     
  Partials       27       27              

Impacted Files	Coverage Δ
augur/sequence_traits.py	8.95% <0.00%> (+0.06%)	⬆️
augur/translate.py	16.99% <0.00%> (+0.06%)	⬆️
augur/traits.py	7.96% <0.00%> (+0.06%)	⬆️
augur/ancestral.py	11.11% <0.00%> (+0.11%)	⬆️
augur/clades.py	11.34% <0.00%> (+0.11%)	⬆️
augur/utils.py	23.07% <0.00%> (+0.16%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e03e1da...c1041b8. Read the comment docs.

[jameshadfield]

Could you write a bit more about the general idea here?

I don't use the ./bin/augur helper script, instead using a editable pip install (pip install -e) which makes augur available on the path & reflects any changes in source code. This is also how we install augur for the travis builds. In my mind the editable install is closer to the actual way users install augur (via pip) compared with the helper script. Not sure what's the most standard way of doing these things.

[elebow]

Here's an example of the problematic behavior: https://travis-ci.com/github/nextstrain/augur/jobs/315709402. That branch introduces a new package augur.util_support, but Python is unable to find it.

Invoking augur with python -m augur works, because the current directory is always added to the path in that mode.

bin/augur adjusts the path (https://github.com/nextstrain/augur/blob/master/bin/augur#L21), so that also works.

I did not realize pip -e installed an executable; that does seem like it should be the right solution. But Travis failed in the link above, even though it executed pip3 install -e .[dev].

I'll be the first to admit I lack experience with the "environment" side of Python; I don't really know what the standard approach is here, either.

[jameshadfield]

There's a difference in how #526 imports augur code compared with how it's imported by current augur commands, which I believe is causing the error. Again, I'm unsure about best practices, but I'm worried if Travis fails with pip install -e then it will fail on other pip installs too.

PR 526:

from augur.util_support.shell_command_runner import ShellCommandRunner

Our "normal" way of doing it (example)

from .utils import run_shell_command, nthreads_value, shquote

[elebow]

If relative imports are the project's preferred way to do it, then that's an easy fix and this PR can be closed.

But PEP 8 does encourage absolute imports (emphasis added because that may be the case here):

Absolute imports are recommended, as they are usually more readable and tend to be better behaved (or at least give better error messages) if the import system is incorrectly configured (such as when a directory inside a package ends up on sys.path):

---

Regarding readability:

If the new module had required something from utils, then it would need an upwards relative import. For example,

from ..utils import myopen

which can quickly become difficult to read, especially if augur is refactored to have many more modules (and a deeper hierarchy of modules) than it does now. See, for example, https://github.com/nextstrain/augur/pull/494/files, where date.py may contain:

from ...utils import ambiguous_date_to_date_range

---

Maybe the change proposed in PR is not the right solution. But IMO the inability to use absolute imports is an opportunity for improvement.

[tsibley]

My 2¢: Both absolute and relative imports should work identically in augur. Sometimes relative imports are nicer to use, sometimes absolute are. If they don't both work, that's a dev/test environment or installation bug that needs fixing.

In development and testing, there is often confusion over which copy of the package is loaded/used. This depends on the method of invocation, the cwd, PYTHONPATH, and lots of other variables. Fixing import issues in tests usually means first figuring out exactly what's getting loaded and why. For example, Python automatically adds the current directory to its library path, so import augur.foo will look first for ./augur/foo.py before looking for an installed augur/foo.py.

(The so-called "src layout" for repos addresses this issue and several related ones. I prefer it, but it's not what Augur uses, so there are lots of nuances.)

[elebow]

The "src layout" makes a lot of sense. I was unaware of many of its benefits.

Here's what I propose to move forward:

1. I'll update the other two PRs to use only relative imports. Then we can get the benefit of the refactoring and test coverage sooner without being blocked by the testing imports question.

2. We can leave this PR open (or close it and open a new issue) to discuss the testing imports question. Maybe it makes sense to rip off the band-aid and convert the project to the "src layout"—the sooner we do it, the more time we spend under the better system.

How does that sound?

[tsibley]

1. 👍
2. I'd say leave it open and we can continue the discussion (maybe now, maybe later). Definitely need input from the rest of the core team before we do a layout switch. Also, I'm not sure how disruptive it'd be right now to adjust the layout, but minimizing disruption seems good right now.

[elebow]

It seems the relative vs absolute question was a red herring. The Travis build failed even when using a relative import:

Traceback (most recent call last):
  File "/home/travis/miniconda/envs/augur/bin/augur", line 5, in <module>
    from augur.__main__ import main
  File "/home/travis/miniconda/envs/augur/lib/python3.6/site-packages/augur/__init__.py", line 11, in <module>
    from .utils import first_line
  File "/home/travis/miniconda/envs/augur/lib/python3.6/site-packages/augur/utils.py", line 18, in <module>
    from .util_support.shell_command_runner import ShellCommandRunner
ModuleNotFoundError: No module named 'augur.util_support'

https://github.com/nextstrain/augur/pull/526/checks

It still passes locally, same as the absolute import.

[elebow]

Okay, found the problem. I was way off.

It turns out that listing a package in setup.py does not impliedly include subpackages—every package in the hierarchy must be enumerated. Setuptools provides setuptools.find_packages() for exactly this purpose (see https://setuptools.readthedocs.io/en/latest/setuptools.html#using-find-packages).

Pushed the change and renamed this PR.

#532 includes this commit, cherry-picked, and the build passes correctly.

[huddlej]

Closing this PR, since its code was merged in #532. Thanks again, @elebow, for this fix and the ambiguous date refactor and tests!