Fix tox with C-extension #310

mcara · 2021-04-23T17:03:57Z

For background info, see #295 and related issues/PRs.

This PR avoids building the package in its own directory when running tox. This is done by changing import mode in pytest (see setup.cfg) and switching to absolute imports in test modules (which is a good thing anyway).

The only test that is failing is doc test when trying to import synphot due to relative import of _astropy_init - you will see this in failing tests.

CC: @pllim

pllim · 2021-04-23T18:18:24Z

With or without import-mode gave different errors. 😿

pllim · 2021-04-23T18:44:04Z

tox.ini

@@ -53,9 +52,6 @@ extras =
    alldeps: all

 commands =
-    # FIXME: Not sure why need this for tox to see the C-ext
-    python setup.py build_ext --inplace
-    # End of FIXME
    pip freeze
    !cov: pytest --pyargs synphot {toxinidir}/docs {posargs}


@jdavies-st , I do have --pyargs here. Am I using it wrong?

With changedir commented out above, because you are running pytest --pyargs from within a directory that has synphot as a subdir, every import synphot will import it from the local checkout, not the installed location, following normal python import rules that always imports the local dir thing before checking python path.

pllim · 2021-04-26T13:20:00Z

I converted this to draft PR for now. FYI.

codecov · 2021-04-26T20:54:51Z

Codecov Report

Merging #310 (2cce31f) into master (c6623f4) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #310   +/-   ##
=======================================
  Coverage   94.85%   94.85%           
=======================================
  Files          14       14           
  Lines        1983     1983           
=======================================
  Hits         1881     1881           
  Misses        102      102

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 c6623f4...2cce31f. Read the comment docs.

pllim · 2021-04-26T20:56:36Z

setup.cfg

@@ -4,8 +4,9 @@ testpaths = "synphot" "docs"
 norecursedirs = build docs/_build synphot/src
 astropy_header = true
 doctest_plus = enabled
+doctest_rst = true


Huh, why this addition, is addopts = --doctest-rst not enough?

pllim · 2021-04-26T20:57:40Z

tox.ini

 # Run the tests in a temporary directory to make sure that we don't import
 # package from the source tree
-#changedir = .tmp/{envname}
+changedir = {homedir}


Does it have to be homedir? Did changedir = .tmp/{envname} not work?

Does it matter?

Yes, homedir might be pre-populated with stuff. .tmp/{envname} is clean.

The point is that tests do run in the install directory unlike what it was before - the source directory. Whether they run in

home/.tox/py39.../lib/python3.9/site-packages/synphot

or

chdir .tmp/py39env ../../home/.tox/py39env/lib/python3.9/site-packages/synphot

does it make a difference?

I'd rather you change it to .tmp/{envname} anyway, if either one works. That is what astropy is using and easier for me to copy paste from upstream in the future.

tox.ini

pllim · 2021-04-26T21:00:06Z

tox.ini

+    # FIXME: Seems like using -c option with multiline Pytest is broken: see
+    # https://github.com/tox-dev/tox-travis/issues/146
+    # at the very least on CI - it runs fine locally on MacOSX.
+    cov: pytest --rootdir={toxinidir} -c {toxinidir}/setup.cfg --pyargs synphot --cov={envsitepackagesdir}/synphot --cov-config={toxinidir}/setup.cfg {posargs} --cov-report xml:{toxinidir}/coverage.xml


Can you explain why the changes here (besides tox-dev/tox-travis#146)?

Not really. It seems like the directory in which pytest is going to run depends on a combination of factors such as rootdir, location of the config for pytest, etc. I was just lucky enough to guess the right incantation.

mcara · 2021-04-28T15:57:39Z

@pllim, in

synphot_refactor/setup.cfg

Line 8 in c6623f4

addopts = --doctest-rst --doctest-ignore-import-errors

why do you use this option --doctest-ignore-import-errors?

pllim · 2021-04-28T16:20:09Z

It was from scientific-python/pytest-doctestplus#108 but I don't remember now if it was added as a precaution or I needed it. You can try remove it and see.

pllim

I feel like this might have added some stuff that is unnecessary. I'll play with this a bit and report back. Thanks for investigating!

pllim

So... I look at the logs here and noticed that it is not actually running the doctest in the docs folder.

And... Purely by accident, I found out that the presence of conftest.py in the root directory messes up tox. So, simply by removing it (#313), tox picks up the installed version again and is also running the doctest. Though I hate to lose the really informative pytest header, I think removing conftest.py at the root dir is the more correct way to go. What do you think, @mcara and @jdavies-st ?

Also see spacetelescope/jdaviz#583 .

pllim · 2021-05-07T21:51:37Z

Actually... Maybe you hit the gold mine with --import-mode=append here. Works nicely over at #314. Not sure about the other stuff you added.

mcara · 2021-05-07T22:48:07Z

Not sure about the other stuff you added.

Like what? -c ? I do not think I added anything else.

pllim · 2021-05-08T00:28:29Z

Well, look at the diff.

mcara · 2021-05-08T16:49:22Z

What specifically about diff worries you? I just removed your workaround that you have marked with # FIXME: Not sure why need this for tox to see the C-ext and the code below that was building the package in place and as the result the test was run in the source directory. I thought you wanted to be able to run tests in the install directory. If that is not what you want - feel free to close this PR.

mcara · 2021-05-08T17:03:53Z

That is, this PR allows running unit tests on installed package instead of the source directory with the package build in place (as it is currently done). All tests seem to be passing and running from the installed directory.

However, I see that #314 is achieving the same with other added benefits like picking up rst tests in the source directory. I think that one has better results and for this reason I am closing this PR in favor of #314

pllim · 2021-05-10T14:01:02Z

@mcara , this PR really helped shine the light on why things weren't working. So even though this wasn't merged, thank you very much for your hard work on this! I'll be sure to give you co-author credit for #314.

mcara self-assigned this Apr 23, 2021

github-actions bot added installation testing labels Apr 23, 2021

mcara force-pushed the fix-cextensions-tox branch from 9e5ec58 to 7154544 Compare April 23, 2021 17:19

mcara requested a review from pllim April 23, 2021 17:44

pllim mentioned this pull request Apr 23, 2021

MNT: Remove _astropy_init.py, replace deprecated np.float #311

Merged

mcara force-pushed the fix-cextensions-tox branch from 7154544 to 090fe5b Compare April 23, 2021 17:57

mcara force-pushed the fix-cextensions-tox branch from c10d4e3 to 090fe5b Compare April 23, 2021 18:22

pllim mentioned this pull request Apr 23, 2021

TST: Investigate tox behavior with C-extension #295

Closed

pllim requested a review from jdavies-st April 23, 2021 18:43

pllim reviewed Apr 23, 2021

View reviewed changes

pllim marked this pull request as draft April 26, 2021 13:19

pllim reviewed Apr 26, 2021

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

pllim reviewed Apr 26, 2021

View reviewed changes

mcara force-pushed the fix-cextensions-tox branch from be8832e to 208bf4b Compare April 26, 2021 21:07

mcara marked this pull request as ready for review April 26, 2021 21:15

mcara requested a review from pllim April 26, 2021 21:18

mcara force-pushed the fix-cextensions-tox branch 2 times, most recently from db483f0 to 5bcdad0 Compare April 28, 2021 07:38

mcara added 2 commits April 28, 2021 12:38

Change to absolute imports in tests and fix tox with C-extension

02a141b

Restore settings for astropy aficionados

16107c6

mcara force-pushed the fix-cextensions-tox branch from d735c8c to 16107c6 Compare April 28, 2021 16:38

pllim reviewed May 4, 2021

View reviewed changes

mcara mentioned this pull request May 4, 2021

Use absolute imports in unit tests #312

Merged

Keep only changes to tox and setup

2cce31f

mcara force-pushed the fix-cextensions-tox branch from 395b7dc to 2cce31f Compare May 4, 2021 22:59

pllim requested changes May 7, 2021

View reviewed changes

pllim mentioned this pull request May 7, 2021

TST: Use --import-mode so tox tests installed package #314

Merged

2 tasks

pllim changed the title ~~Change to absolute imports in tests and fix tox with C-extension~~ Fix tox with C-extension May 7, 2021

mcara closed this May 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tox with C-extension #310

Fix tox with C-extension #310

mcara commented Apr 23, 2021

pllim commented Apr 23, 2021

pllim Apr 23, 2021

jdavies-st Apr 26, 2021

pllim commented Apr 26, 2021

codecov bot commented Apr 26, 2021 •

edited

Loading

pllim Apr 26, 2021

pllim Apr 26, 2021

mcara Apr 26, 2021

pllim Apr 26, 2021

mcara Apr 26, 2021

pllim Apr 26, 2021

mcara Apr 26, 2021

pllim Apr 26, 2021 •

edited

Loading

mcara Apr 26, 2021

mcara commented Apr 28, 2021

pllim commented Apr 28, 2021

pllim left a comment

pllim left a comment

pllim commented May 7, 2021

mcara commented May 7, 2021

pllim commented May 8, 2021

mcara commented May 8, 2021

mcara commented May 8, 2021

pllim commented May 10, 2021 •

edited

Loading

Fix tox with C-extension #310

Fix tox with C-extension #310

Conversation

mcara commented Apr 23, 2021

pllim commented Apr 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Apr 26, 2021

codecov bot commented Apr 26, 2021 • edited Loading

Codecov Report

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

pllim Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcara commented Apr 28, 2021

pllim commented Apr 28, 2021

pllim left a comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

pllim commented May 7, 2021

mcara commented May 7, 2021

pllim commented May 8, 2021

mcara commented May 8, 2021

mcara commented May 8, 2021

pllim commented May 10, 2021 • edited Loading

codecov bot commented Apr 26, 2021 •

edited

Loading

pllim Apr 26, 2021 •

edited

Loading

pllim commented May 10, 2021 •

edited

Loading