Skip to content

MAINT: Add Tempita to randint helpers #8071

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

Merged
merged 1 commit into from
Sep 21, 2016
Merged

MAINT: Add Tempita to randint helpers #8071

merged 1 commit into from
Sep 21, 2016

Conversation

gfyoung
Copy link
Contributor

@gfyoung gfyoung commented Sep 20, 2016

Refactors the randint helpers to use a Tempita template. This will reduce technical debt in the long run.

In addition, this should help pave the way for the changes in #6938 to be smoothly integrated.

This PR depends on #8066, as that has the patch for the DEBUG=1 machine.

@charris
Copy link
Member

charris commented Sep 20, 2016

Adding

            - cython3-dbg

at line 56 of .travis.yml solves the debug test failure. See #8073.

@gfyoung
Copy link
Contributor Author

gfyoung commented Sep 20, 2016

Fair enough, though I would prefer my fix in #8066 just because it is a more long-term patch.

@charris
Copy link
Member

charris commented Sep 20, 2016

I think the root cause is that generate_cython uses sys.executable, which is python3-dbg. Hence cythonize will import from that python version, but it worked before because the cython subprocess call simply called the cython executable, whatever it was. Not sure if that executable is overridden when the dbg version is installed or if that needs to be called specifically.

@gfyoung
Copy link
Contributor Author

gfyoung commented Sep 20, 2016

Not sure either, though not sticking to one Python version while installing seems buggy to me anyhow.

@charris
Copy link
Member

charris commented Sep 20, 2016

So how about the easy fix for this PR, then the larger fix can go in afterward after consideration.

@charris
Copy link
Member

charris commented Sep 20, 2016

Many systems run/build with multiple python versions. However, I don't think the python version used matters for processing the templates, so tempita could be run from the command line as python -m tempita infile outfile using subprocess. That does require that tempita be (pip) installed, but that might be a good fallback if import from cython fails. Note that cython can also be run that way which may avoid some problems if it isn't in the path, see the cythonize workaround for that.

@gfyoung
Copy link
Contributor Author

gfyoung commented Sep 20, 2016

@charris : Sure, but I still fail to see why we would want to use two different Python's in our testing environment. It would seem logical to just use one like we do with all the other tests. Not to mention, the virtual environment is being initialized incorrectly because python=python does not select python3-dbg but rather python3.

@charris
Copy link
Member

charris commented Sep 20, 2016

I believe python3-dbg is used for the build.

# explicit python version needed here
if [ -n "$USE_DEBUG" ]; then
  PYTHON="python3-dbg"
fi

Admittedly, the whole script is a bit obscure. That aside, I run with three python versions installed and like to build with any of them. Currently, I need to install cython for each to have tempita. It does work for python2 to use the command line python -m tempita in out which will always use the default python as running cython currently does, but that doesn't work when python3 is the default due to a bug in tempita: it reads files as binary and that doesn't work with the string parsing. Simple to fix, but I guess the downside of relying on an unmaintained package is it doesn't get fixed.

The point is that this is not only a testing problem and the current universal solution is to simply install cython in the python used for the build. Whether or not to have a single python for each venv for testing is another question that I think comes down to whatever is simplest.

Note that a simple fix here splits the problem into two disjoint subproblems, so this can go in without a dependency on a future change to the testing environment.

@charris
Copy link
Member

charris commented Sep 20, 2016

Note that there used to be a copy of tempita included with numpy. We might want to revisit that idea, although it might take some work to unify the Python2 and Python3 versions.

@gfyoung
Copy link
Contributor Author

gfyoung commented Sep 20, 2016

@charris : Sure, I don't have any issues making the patch that you proposed earlier. However, it would be worthwhile to work out this issue separately with this one build.

And to reply to your point about that if-then statement. While it does set the PYTHON environmental variable, that's only after the virtual environment has been setup with the incorrect python version.

@gfyoung
Copy link
Contributor Author

gfyoung commented Sep 20, 2016

@charris : Perhaps an email discussion might be useful for my other PR (regarding the Travis build configuration)?

Refactors the randint helpers to use a Tempita
template. This will reduce technical debt in the
long run.
@charris charris merged commit 866bc02 into numpy:master Sep 21, 2016
@charris
Copy link
Member

charris commented Sep 21, 2016

OK, let's get this in. Thanks @gfyoung .

I think there is more cleanup work to do with cythonize, but we have something that works and folks who test master can start to offer feedback.

The Travis test configuration is best done on the PR as that is what I monitor and what others can see, but perhaps a post to the list would be helpful in getting more people interested in taking a look. I think it mostly comes down to making a simple decision one way or the other.

@gfyoung
Copy link
Contributor Author

gfyoung commented Sep 21, 2016

@charris : Yep, makes sense. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants