sherjilozair / char-rnn-tensorflow

Multi-layer Recurrent Neural Networks (LSTM, RNN) for character-level language models in Python using Tensorflow
MIT License
2.64k stars 960 forks source link

Simple tests on Travis CI - part 2 #79

Closed hugovk closed 7 years ago

hugovk commented 7 years ago

Builds on #74 by running some basic commands to check they can at least run on each Python version without exceptions. Also runs them with coverage.

 - coverage run --append --include=./* train.py --data_dir data/teenytinyshakespeare
 - coverage run --append --include=./* train.py --data_dir data/teenytinyshakespeare --init_from save
 - coverage run --append --include=./* sample.py

Also sends coverage to Coveralls; enable this repo here.

hugovk commented 7 years ago

@sherjilozair Is this okay to be merged?

ubergarm commented 7 years ago

@hugovk I just checked out your CI over at https://travis-ci.org/hugovk/char-rnn-tensorflow

Looks like you got it working pretty well, might be some flake8 stuff to fixup.

I like the style guide of:

  1. flake8 --max-line-length=120
  2. One argument per line aligned with opening delimiter:
    foo = long_function_name(var_one,
                         var_two,
                         var_three)
  3. Docstrings like:

    def foo():
    """A one liner description about this ubiquitous function."""
    pass
    def complex_fizz_buzz():
    """This is the first paragraph of something that will be longer
    than one line. In fact we may have multiple paragraphs of text.
    
    Indeed, here is the second paragraph.
    """
    pass

Dunno what @sherjilozair prefers, it may be worth putting something like this in the README's contributing section and possibly enforcing it somewhat...

ubergarm commented 7 years ago

I'll try this out on my fork and patch in your extensive .gitignore soon.

ubergarm commented 7 years ago

@sherjilozair when you have a chance, can you:

  1. sign onto https://travis-ci.org/profile
  2. add the repo https://github.com/sherjilozair/char-rnn-tensorflow/

I couldn't find a way to add it myself, maybe some permissions thing. Easier if you just add it probably.

I was able to add the repo to https://coveralls.io/github/sherjilozair/char-rnn-tensorflow

sherjilozair commented 7 years ago

Done! Let me know if you need anything else.

On Sun, Mar 12, 2017 at 3:02 AM, ubergarm notifications@github.com wrote:

@sherjilozair https://github.com/sherjilozair when you have a chance, can you:

  1. sign onto https://travis-ci.org/profile
  2. add the repo https://github.com/sherjilozair/char-rnn-tensorflow/

I couldn't find a way to add it myself, maybe some permissions thing. Easier if you just add it probably.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sherjilozair/char-rnn-tensorflow/pull/79#issuecomment-285901873, or mute the thread https://github.com/notifications/unsubscribe-auth/AApVxHF3pq5v3_Yb7qTTsudU_OChcpEhks5rkxL_gaJpZM4LoiCM .

ubergarm commented 7 years ago

The tests did hit one odd snag on python 3.4, a segfault?! There is a chance that if sample.py generates an odd character perhaps the coverage test fails?

https://travis-ci.org/ubergarm/char-rnn-tensorflow/jobs/210137899#L272-L281

I'll go ahead and merge this and we can keep an eye on on how the builds work before putting up a shield. :)

ubergarm commented 7 years ago

@hugovk Thanks for this sweet travis ci and coverage capability! I just added the coverage badge as it looks good, but don't want to add the build status badge until the heisenbug gets figured out: https://travis-ci.org/sherjilozair/char-rnn-tensorflow

Any thoughts why the builds don't seem to be deterministic on pass/fail? That segfault business seems fishy.

ubergarm commented 7 years ago

Now that we have a few data points, seems most likely to fail on Python 3.5 and 3.4 anecdotally... I'm only testing w/ 2.7 locally still, I'd like to get on 3.6...

If it really is failing on 3.5, that'd be bad as it is pretty widespread. Hrmm.