New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix __future__ imports across cells #308

Merged
merged 6 commits into from Oct 27, 2017

Conversation

Projects
None yet
2 participants
@lesteve
Contributor

lesteve commented Oct 24, 2017

I found this problem in scikit-learn when building the documentation with Python 2. An example showing the problem is:

"""
This example tests that __future__ imports works across cells

"""

from __future__ import print_function

####################
# Section
print('test', end='')

You get a SyntaxError because the __future__ import in the first code cell is not taken into account in the second cell.

I am not an expert in the ast/compile flags, so if someone sees a better way to do the same thing, let me know.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 24, 2017

Do we know if these __future__ imports are properly isolated across examples? We could check, at least on Python 2, by doing assert 1 / 2 == 0 in one file and from __future__ import division; assert 1 / 2 == 0.5 in another. Is it related to this / worth adding? (I think I've had something like this come up before with print_function not working even though a file had from __future__ import print_function before.)

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 24, 2017

Also any suggestion of testing this without adding a dummy example would be more than welcome!

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 24, 2017

Add it to the tinybuild instead then it won't go in the docs but will be tested

https://github.com/sphinx-gallery/sphinx-gallery/tree/master/sphinx_gallery/tests/tinybuild

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 24, 2017

Do we know if these future imports are properly isolated across examples?

I think they are because I used a separate compiler per file but I have not checked.

Add it to the tinybuild instead then it won't go in the docs but will be tested

I saw that but my understanding was that basically, the test_embed_links function in in sphinx_gallery/tests/test_full.py would test my fix, which is a bit misleading. Ideally it would be better to have a more fine-grained way of testing different things in tiny_build, wouldn't it?

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 24, 2017

Ideally it would be better to have a more fine-grained way of testing different things in tiny_build, wouldn't it?

Sure. Feel free to rename or change as necessary. Either way, it seems nicer to have the regression testing explicitly done by pytest rather than needing to run make doc (the current PR requirement)

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 24, 2017

Sure. Feel free to rename or change as necessary. Either way, it seems nicer to have the regression testing explicitly done by pytest rather than needing to run make doc (the current PR requirement)

I will try this route and see how far I can go.

Test __future__ imports in tinybuild
* use module-level fixture for running sphinx app
* split test_embed_links into test_run_sphinx that just runs sphinx and
  test_embed_links that only test the embedding of links

@lesteve lesteve force-pushed the lesteve:future-imports-across-cells branch from 9d05f03 to a569d66 Oct 25, 2017

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 25, 2017

I am using tinybuild now, I did some work in sphinx_gallery/tests/test_full.py to have a module-level fixture to run the sphinx app only once and split the existing test in two (one that just checks that the sphinx app runs with basic checks, and test_embed_links that just tests the embedding of the links')

tinybuild is a nice enhancement I think we could test more things there ... making gen_rst.generate_file_rst more easily testable would certainly be another improvement going in the same direction.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 25, 2017

Since we're checking __future__ usage, WDYT about:

  1. adding a from __future__ import division; print(1 / 2); in some example that will be run before a separate example that does print(1 / 2), and
  2. on Python 2 check to make sure the former prints 0.5 and the latter 0, and on Python 3 check that they are both 0.5

It seems to get at mostly the same point of this PR, namely that __future__ imports are handled properly. But it's also reasonable to do it separately later if you want to be done for now.

# The matplotlib doc download is a bit unsafe, so skip for now:
# assert '#module-matplotlib.pyplot' in lines
# assert 'pyplot_api.html' in lines
assert '#module-matplotlib.pyplot' in lines

This comment has been minimized.

@larsoner

This comment has been minimized.

@lesteve

lesteve Oct 25, 2017

Contributor

Yeah I did not understand why and it was passing locally so I figured I would give it a go ... oh well, maybe I'll comment them out again ... maybe it depends on the sphinx version ?

This comment has been minimized.

@larsoner

larsoner Oct 25, 2017

Contributor

Yeah probably sphinx. I guess you could check whether version 1.5+ is used and only check if that's the case, as I think that Ubuntu build uses 1.4.9

This comment has been minimized.

@lesteve

lesteve Oct 25, 2017

Contributor

For the record I could not reproduce this locally with sphinx 1.4.9. I thought maybe the matplotlib version has something to do with it (and the possible mismatch with the matplotlib doc fetched from the internet but I did not manage to reproduce either).

This comment has been minimized.

@larsoner

larsoner Oct 25, 2017

Contributor

Yeah at one point I tried to reproduce, as well. To really find the problem I figured I'd need to get a Trusty system going with system Python packages to mirror those of Travis, which was too much for me to follow through on.

@@ -1,5 +1,5 @@
setuptools
matplotlib
matplotlib==2.0

This comment has been minimized.

@larsoner

larsoner Oct 25, 2017

Contributor

revert this, too?

This comment has been minimized.

@lesteve

lesteve Oct 25, 2017

Contributor

Yep, done.

@lesteve lesteve force-pushed the lesteve:future-imports-across-cells branch from e7e48be to 5298609 Oct 25, 2017

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 25, 2017

It seems to get at mostly the same point of this PR, namely that future imports are handled properly. But it's also reasonable to do it separately later if you want to be done for now.

I could do that, the example order makes it not that obvious which example is run first though. I could make two example folders so that at least the order is easier to guess (examples and second_folder_to_run would work I think). To be honest I would rather make gen_rst.generate_file_rst easier to test but the first solution may take less time.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 25, 2017

I could do that, the example order makes it not that obvious which example is run first though.

The default IIRC is "amount of code" so just ensure that the second one to be run has more lines of code. Or explicitly set the order in conf.py, as it's a configurable thing now (can be alphabetical if you want to do it that way, or explicit if you just want to list them)

@larsoner

This comment has been minimized.

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 25, 2017

Yeah I remember this. It's just that the test is not as obvious as it could be.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 25, 2017

I think it could be made sufficiently explicit with comments in the vicinity of the test. But up to you if you want to try it or not

@lesteve lesteve force-pushed the lesteve:future-imports-across-cells branch 2 times, most recently from 91d97e8 to fee28fd Oct 26, 2017

@lesteve lesteve force-pushed the lesteve:future-imports-across-cells branch from fee28fd to f488607 Oct 26, 2017

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 26, 2017

I think this is ready to go. It was a bit painful but I discovered a small issue with the fix (hence plot_future_imports_broken.py) so it was worth it.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 26, 2017

Changes LGTM, glad to see this __future__ bug fixed!

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 27, 2017

OK merging this one then, thanks for the review @larsoner!

@lesteve lesteve merged commit dbd4b7b into sphinx-gallery:master Oct 27, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lesteve lesteve deleted the lesteve:future-imports-across-cells branch Oct 27, 2017

@lesteve lesteve referenced this pull request Nov 10, 2017

Merged

[MRG] Encoding problems #311

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment