Multiple interpreters and HTML docs testing

[flacjacket]

Currently, this is still kind of hacked together and mostly up for review, so any feedback/ways to improve things/how you would like to see this sort of thing work is much appreciated.

This primarily does two things:

• It allows you to specify multiple interpreters by adding multiple -i flags. The results for all of the results are uploaded after all of them have run and a summary GitHub comment is made. It currently always creates this summary post, which should probably be changed to do so only when there are multiple interpreters. For an example, see here.
• Tests the HTML docs building by adding the -D flag. This is basically taking what Aaron mentioned in sympy/sympy#1400 and adds it as one of the tests. The main difference is the summary in the GitHub comment is different, it gives the docs building command and the Sphinx version. I'm not sure if there is any other relevant info, but that is what I came up with as possibly useful. There isn't really any error checking to make sure the tester has, for example, sphinx installed. Also, adding the flag adds the docs as an additional test, there isn't a way to do only the docs, which would be good for PR's that only improve docs. For an example, see here.

[certik]

Nice, the argparse is a much better approach. I am +1 to everything that you did.

Let's test this a little bit. If it works, let's push it in.

Thanks!

[asmeurer]

I've already seen the results of this and I like it. I think the next step forward after this is to have the bot edit the comment to add more tests, rather than posting a new one. That way we spam much less with it.

Should we wait for sympy/sympy/#1400 to be merged first? The -D flag won't work without it.

[asmeurer]

The only downside to this PR is that the bot now requires Python 2.7, but I don't think it's that big of a deal (and probably worth it for the new features).

Perhaps it should at least be added to the README, though.

[asmeurer]

The interactive help is now a little more confusing (though I understand why). Is it possible to make sympy-bot --help always also print the help for review? If not, maybe add some preamble or postamble to make it clear that sympy-bot review -h gives a full help.

Also, is it possible to make sympy-bot with no arguments do sympy-bot -h as it did before?

[asmeurer]

When using argparse, it's better to use %(default)s instead of explicitly typing the default in the help string.

[asmeurer]

Why not just make -D only build the docs? If you want to also run the tests, you can do it by adding -i python (maybe just a bare -i could run the default Python).

[asmeurer]

By the way, @certik, if I could get your input on sympy/sympy#1400, that would be great. The only remaining doc build failure relates to the translations of the tutorial, and I'm not sure how to proceed.

[flacjacket]

Alright, so now having -D but no -i runs just the doc build and you can just have a bare -i to also run the tests with the default interpreter.

I set argparse to show the full help when there are too few args. I don't think you can display a subparser help either when it does this and as part of the default help. I also reworded the epilog in the main help to be clear on how to see the other help. Argparse can be installed for older versions of Python, so that won't break it in older versions, which I noted in the README.

Also, I changed how the config file is read in, using ConfigParser instead of exec'ing the file. You will need to change your config file and remove any quotes around strings. In Python 3 it is called configparser, but the bot doesn't run in 3, so I didn't do any handling of the import to do this. Now, you can specify multiple interpreters as default as a comma separated list and all of the command line options can be configured by the config file, tho bools might not be handled quite right.

If you think the docs pull will be in soon, I feel this can wait until then.

[asmeurer]

But you've also used things like dictionary comprehensions and set literals (if I remember correctly), which are Python 2.7 only.

[flacjacket]

Ah, right. I got rid of the dictionary comprehension. I don't think I used set literals anywhere. The {} are empty dicts, not sets, so those are fine.

[asmeurer]

You do have a set literal (try running the bot in Python 2.6). Honestly, though, it isn't that big of a deal if it requires Python 2.7.

[asmeurer]

Oh, never mind. That's just a dictionary with a regular SyntaxError.

[flacjacket]

This latest commit adds support for creating various profiles in the config files (e.g. slow tests, 32-bit interpreters, etc.) controlled by the --profile option. This will require more changes to the config file, bringing the list up to adding a [default] section line and removing quotes around things. I've made the changes clear in the README and decided to include an example config file separate from the README.

I realize I'm pulling configuration in two different direction between this and the API token/config file generation in the other PR, which in retrospect was bad. But I think if I can get one of them merged to master, merging the changes together will make things simpler, rather than messier.

[asmeurer]

Is there a way to add a profile for building the docs?

[asmeurer]

This is really awesome, by the way.

@Krastanov you might want to review this if you have the time to see if there are any features that you might want that are missing, as I think this will be a great way to run your never ending bot.

[flacjacket]

This new commit allows a profile to build the docs. One thing is currently if the config file picks up an interpreter, it will always run the tests, so if you want to be able to just build docs, you can't have a default set of interpreters. Is there some way you'd like to specify for a profile not to run the Python tests? Something like interpreter = None or interpreter =? Should there be a way to disable Python tests from the command line?

[asmeurer]

interpreter = None seems reasonable to me.

[flacjacket]

Now interpreter = None works in the config file and also I guess -i None would do the same from the command line.

[Krastanov]

When this gets merged it may be interesting to add a rule for it to the travisCI config.

I will pull it now and start using it for the bot that I run.

[Krastanov]

The test were ran and at the end I got

> View log at: /tmp/sympy-bot-tmpEBf82u/out/log
> Uploading test results for pull number 1293
> Uploaded report for '/usr/bin/python2.7' at: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYhpgfDA
> Uploaded report for '/usr/bin/python3.2' at: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYpbcfDA
> Uploaded report for '/usr/local/bin/python2.5' at: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYjb8fDA
> Uploaded report for 'build docs' at: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY7p8fDA
Traceback (most recent call last):
  File "./sympy-bot", line 451, in <module>
    main()
  File "./sympy-bot", line 167, in main
    dispatch_reviews(options, urls, username=username, password=password)
  File "./sympy-bot", line 308, in dispatch_reviews
    master_hash, branch_hash, user, config)
  File "./sympy-bot", line 382, in formulate_review
    details = get_sphinx_version()
  File "./sympy-bot", line 445, in get_sphinx_version
    import sphinx
ImportError: No module named sphinx

Do we want to catch this exception? It happened when the results were uploaded, not when the test was starting.

[flacjacket]

Yeah, that is from the end when it is creating the comment to post on the PR, it tries to import sphinx so it can get the version number (see here). Do you have sphinx installed on your bot server? As a side note, testing the html docs won't work until sympy/sympy#1400 is in, that's what sets up the make html-errors command.

Nevertheless, we may want to catch that error before trying to build the docs, just to be sure.

[Krastanov]

I was just going to report the problem with the make html-errors command.

http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9sYfDA

I have installed sphinx and now it seems that it works

[flacjacket]

Now it catches that error and disables the doc building if it can't find sphinx.

[asmeurer]

If it will make things easier, we can merge sympy/sympy#1400 (assuming it gets a positive review), and fix the last remaining sphinx error later.

[Krastanov]

There seems to be some problem with memory leaks or cash or something like that. When I test using multiple interpreters (instead of running sympy-bot multiple times) the process uses an ungodly amount of RAM.

[flacjacket]

problem with ... cash or something

Mo money mo problems :P

Barring something more insidious, that's probably happening because it holds on to all the reviews while it is reviewing and uploads them all at the end. In that case, it might be better to upload them right away and free up whatever memory we can. I can take a look at it from a memory standpoint today and see what I can come up with.

[flacjacket]

@Krastanov, try it now. If the problem was in holding onto all the logs, that should be fixed now. Also, on what order of magnitude is an ungodly amount of memory?

[Krastanov]

On 11 July 2012 18:30, Sean Vig reply@reply.github.com wrote:

Stephan, try it now. If the problem was in holding onto all the logs, that should be fixed now. Also, on what order of magnitude is an ungodly amount of memory?

It seems to be noticeably more than 500MB (500MB is the amount of ram on the computer that I use for the tests). It starts swapping and instead of 1 hour for testing 2.5, 2.7 and 3.2 it takes ~3 hours.

I will check the new version now.

[Krastanov]

by the way, the problem may have been triggered by some failing test (I have hash randomization on)

[asmeurer]

@Krastanov I think that's definitely the tests. c.f. the mailing list.

[asmeurer]

I need to test the new config stuff, but other than that, and my comment on the most recent commit, I think this is ready to go.

[asmeurer]

@flacjacket any preference as to which PR of yours gets merged first. There is going to be a merge conflict either way.

[flacjacket]

I removed that last commit.

I think this one should be merged first. I think the configuration file writing should be re-worked with ConfigParser before it goes in.

[asmeurer]

Is there a way to make one profile inherit and merge the settings from another profile, so that I don't have to duplicate the paths to all my Python executables many times?

[asmeurer]

Also, is it possible to add comments to the config file?

[asmeurer]

OK, looking at http://docs.python.org/library/configparser.html, I think I might be able to use the built-in stuff to do what I want. Comments are delineated by # or ;.

ConfigParser automatically does new-style string formatting type expansion, like

var = value
f = %(var)s

and f will be set to value. What I want is an easy way to type all my Python executables once (I have about a dozen of them). I'll see how easy it is, and post a link to my config file if I get it working.

[asmeurer]

Hmm. It seems that nested replacements are not supported (or else I did it wrong). This is what I've got: https://github.com/asmeurer/dotfiles/blob/master/.sympy/sympy-bot.conf.

[asmeurer]

Oh, I see what happened. I incorrectly assumed that all the values from [default] would be pulled in. It seems to be impossible to have values not in a profile, so can this be fixed?

[asmeurer]

OK, I see why it's doing this. I'll push a fix to flacjacket/sympy-bot#1.

[asmeurer]

Another bug:

> View logs for PR 1427 in: /var/folders/wc/dppcpmxs1tlb36nqcw853wkm0000gq/T/sympy-bot-tmpWj2xcY/out
Traceback (most recent call last):
  File "./sympy-bot", line 473, in <module>
    main()
  File "./sympy-bot", line 175, in main
    dispatch_reviews(options, urls, username=username, password=password)
  File "./sympy-bot", line 375, in dispatch_reviews
    config.build_docs, config.build_docs_command, user)
  File "./sympy-bot", line 429, in formulate_review
    details = get_platform_version(i)
  File "/Users/aaronmeurer/Documents/Python/sympy/sympy-bot/utils/cmd.py", line 97, in get_platform_version
    executable = get_executable(interpreter)
  File "/Users/aaronmeurer/Documents/Python/sympy/sympy-bot/utils/cmd.py", line 80, in get_executable
    return ouput.strip()
NameError: global name 'ouput' is not defined

[asmeurer]

This is not doing the right thing at all for the interpreter3 flag in the config file. It ignores it for profiles that have both it and interpreter, and it uses the default interpreter for those that don't have any interpreter.

[flacjacket]

The interpreter3 configuration option is working for me. The interpreters used are first taken from the -i options, and if none are specified, falls back to interpreter3 config if -3 is passed and interpreter config otherwise.

[asmeurer]

Is there a way to add -3 to the profile them? Personally I think that's somewhat confusing.

[asmeurer]

Actually what I want to do is run both Python 2 and Python 3 tests in one go. Is there a way to do that?

[flacjacket]

-3 is the same as --python3, so you can set it with python3 = True.

I think the best way to do that would be to have -i use interpreter (which is the default, and so only necessary if -D is passed), -3 use interpreter3, and -i3 use both interpreter and interpreter3. Currently -i and -3 work like this, I'd have to add the -i3 behavior if you think that is good.

[asmeurer]

I don't really care about command line options. I just want a way to make a profile that runs both Python 2 and Python 3 tests.

[asmeurer]

I guess the problem is the dual role that the interpreter option plays. Maybe we should add a -2/--python2 option, at least for the sake of the config file.

[flacjacket]

Yes, my thought exactly. I was just in the process of adding a -2 option.

[flacjacket]

I just pushed a commit that should do the trick, setting python2 = True and python3 = True.

[asmeurer]

OK, let me test it. If it works, then I think this is ready to go.