Local Pytest and docker TravisCI testing #49

canyon289 · 2019-01-14T18:22:31Z

Adding the following in this PR. Will unmark WIP when asking for a merge

More test infrastructure (In addition and non conflicting with Tests: Simple sense test for linear regression #39)
pylint
requirements-dev.txt
TravisCI testing, and linting
Coveralls

canyon289 · 2019-01-14T19:42:53Z

This is the issue we were having on ArviZ with tensorflow, which is why we ended up moving to docker testing. Seems like its a problem here too

Tests run fine locally but not in TravisCI for some reason

https://travis-ci.org/pymc-devs/pymc4/builds/479554525#L609

requirements.txt

springcoil

LOOKS COOL

canyon289 · 2019-01-17T00:09:30Z

Well at this point tests and lint are working if anyone wants to take a look.

Please don't merge yet though. I need to do a few more things to button up this pull request, along the lines of add documentation and propose a couple of things. Please feel free to review in the meanwhile though

springcoil

LGTM

pymc4/tests/conftest.py

twiecki · 2019-01-17T09:17:09Z

scripts/install_miniconda.sh

@@ -0,0 +1,38 @@
+#!/usr/bin/env bash


Why is this required if the docker image pulls from the miniconda image?

It is not, but I left it there in case anyone was using it to install miniconda locally.

Since you called me out on it though I'm ok with deleting it

scripts/create_testenv.sh

twiecki · 2019-01-18T09:01:45Z

pymc4/tests/test_random_variables.py

+def tf_supported_args():
+    """Provide arugments for each supported Tensorflow distribution"""
+    _tfp_supported_args = {
+        "Bernoulli": {"probs": 0.5},


why not directly map the RV classes to their parameters? Saves the getattr lookup below.

This sounds way better. Will do

I remember why I didn't do this.

I wanted a couple things

An ordered object of distribution strings so I can pass them to the id argument function

A mapping of parameters to distributions

A guarantee that when testing a method, the id string matches the distribution being tested

Only one place where order is defined. If order is defined in two places, then coordination of order is human reliant, which means it'll definitely get messed up.

If I mapped directly from rv.classes to their parameters the tests would still work, but I wouldn't have a way to get the right distribution name string to the id argument.

The other reason I felt this might be better is that when testing the distribution name is visible, even in pdb. If I mapped RV Classes, in a debugger they would show up as memory addresses since we haven't defined a __repr__ for RandomVariable yet

There also is the possibility I'm wrong or explaining things poorly so let me know if this doesn't make sense.

I don't follow that reasoning.

An ordered object of distribution strings so I can pass them to the id argument function

This isn't ordered once you created it as it's a dict. You could sort it but I don't see why you would want to do that.

A mapping of parameters to distributions

This is a mapping of parameters to strings of distributions.

A guarantee that when testing a method, the id string matches the distribution being tested

I don't see the benefit of that. You only increase the chance of having typos in the strings.

Only one place where order is defined. If order is defined in two places, then coordination of order is human reliant, which means it'll definitely get messed up.

Where is order defined here in the first place? And why would that be important?

If I mapped directly from rv.classes to their parameters the tests would still work, but I wouldn't have a way to get the right distribution name string to the id argument.

That's true, but do we need that?

The other reason I felt this might be better is that when testing the distribution name is visible, even in pdb.
We do have the class name that will get printed.

In sum, I don't really see the benefit, other than easier lookup when trying to get parameters manually but I don't see the use-case as it's just a test. But I do think it's more brittle to have that level of indirection.

That's fair! The additional layer of indirection is more brittle. The benefit I saw was allowing tests to be named by pytest making it simpler to figure out which test is failing in a log, and select tests when using the -k command line flag with for pytest

Here's an example of current implementation output

Here's an example of what I think the proposed implementation is asking for. In this case I don't like how the output log doesn't tell me which distribution is being tested. tf_distribution[2] is a lot less meaningful than 'Categorical'

*But I may have implemented incorrectly. Let me know if this is what you had in mind!
https://github.com/canyon289/pymc4/blob/pytest_alternative/pymc4/tests/test_random_variables.py

OK, that looks way nicer. I wonder if we should just add a__repr__ instead though.

I think I reformulated this to meet the ask. Let me know what you think!

twiecki · 2019-01-18T09:06:15Z

pymc4/tests/conftest.py

+
+@pytest.fixture(scope="function")
+def tf_session():
+    sess = tf.Session()


How about:

with tf.Graph().as_default(): with tf.Session() as sess: yield sess

Do we need to open and close session each time? I figured one session was fine if we cleared the graph, and opening and closing them would be a waste of computational resources, but I could be wrong

If we do implement this I'd prefer sess.close() in the finalizer since it'll make it easier to avoid stacked context managers, which then makes debugging easier and mocking easier imo

I'd vote for the current solution with graph reset. There is no difference between them. I see no computational resources waste... (If changes are already commited that is no problem)

@ferrine this suggests they are different: https://stackoverflow.com/questions/42706761/closing-session-in-tensorflow-doesnt-reset-graph

I think it's quite easy to run into memory problems here if we keep adding to graph or create multiple sessions in these tests.

eigenfoo

Looks good!

One comment: there's a binom_test.py in the top-level directory: is that supposed to be there?

scripts/Dockerfile

pymc4/tests/test_random_variables.py

canyon289 · 2019-01-18T14:48:31Z

Looks good!

One comment: there's a binom_test.py in the top-level directory: is that supposed to be there?

Whoops no. I'll delete it, thanks for catching that

ferrine · 2019-01-18T17:04:21Z

What conda channels do we plan on using? It appears the conda docker is only pulling from defaults, but we may want to grab things (tensorflow?) from conda-forge.

Fair point, right now we had some discussions about using tf-nightly and tfp-nightly and then switching to tf2 when tfp becomes compatible.

Besides those to requirements though nothing is installed by conda specifically, other than the base env packages. Everything else is done by pip and pypi

Should we then develop on top of eager mode?

canyon289 · 2019-01-18T19:18:58Z

@eigenfoo referenced this ticket somehow but I can't figure it out so adding this reference as well

tensorflow/probability#81

twiecki · 2019-01-19T12:44:10Z

Thanks!

eigenfoo · 2019-01-20T01:17:09Z

btw @canyon289 I just noticed this but fun fact, sed doesn't need to have a / follow the s: it can be any character so long as you're consistent. So

's/\/opt\/pymc4/\/home\/travis\/build\/pymc-devs\/pymc4/g'

could also just be

's_/opt/pymc4_/home/travis/build/pymc-devs/pymc4_g'

which is more readable!

canyon289 force-pushed the pylint_pytest branch from 8060579 to af08d56 Compare January 14, 2019 18:29

eigenfoo added the WIP Work in progress label Jan 14, 2019

twiecki reviewed Jan 15, 2019

View reviewed changes

requirements.txt Outdated Show resolved Hide resolved

canyon289 mentioned this pull request Jan 15, 2019

ENH: add more random variables #46

Merged

eigenfoo mentioned this pull request Jan 15, 2019

TST: tests for random variables #52

Closed

canyon289 added 7 commits January 16, 2019 15:18

Add initial test and fixture for RV

394de33

Format with black

c5bd9f4

Add testing and coveralls to CI

bf9eac9

Invalidate caching through yml file for now

1d9ccc4

Upgrade pip in CI

cd45814

More pip fixes in CI

ee2f627

Modify requirements.txt for pytest >4.0

aa3bacc

canyon289 force-pushed the pylint_pytest branch from 7e0126b to aa3bacc Compare January 16, 2019 23:19

canyon289 added 6 commits January 16, 2019 15:34

Add container.sh file

056f6f3

Add create_testenv.sh

e483d50

Add Dockerfile

eb1075e

Add install_miniconda.sh

ea38cfc

Add lint.sh

720c101

Update .travis.yml

31ad78a

canyon289 changed the title ~~[WIP] Pylint pytest~~ [WIP] Local Pytest and docker TravisCI testing Jan 16, 2019

Fix absolute import

00f7afa

springcoil reviewed Jan 16, 2019

View reviewed changes

Add __init__.py for tests

92e845f

springcoil approved these changes Jan 17, 2019

View reviewed changes

canyon289 added 2 commits January 16, 2019 16:17

Fix typo in coveralls upload

65a9e23

Add docker readme

72352bf

twiecki reviewed Jan 17, 2019

View reviewed changes

pymc4/tests/conftest.py Show resolved Hide resolved

twiecki reviewed Jan 17, 2019

View reviewed changes

twiecki reviewed Jan 18, 2019

View reviewed changes

scripts/create_testenv.sh Outdated Show resolved Hide resolved

twiecki reviewed Jan 18, 2019

View reviewed changes

scripts/create_testenv.sh Outdated Show resolved Hide resolved

twiecki reviewed Jan 18, 2019

View reviewed changes

eigenfoo reviewed Jan 18, 2019

View reviewed changes

scripts/Dockerfile Outdated Show resolved Hide resolved

pymc4/tests/test_random_variables.py Outdated Show resolved Hide resolved

pymc4/tests/test_random_variables.py Outdated Show resolved Hide resolved

canyon289 added 5 commits January 18, 2019 08:19

All tests passing

53fdf34

Fix linting error

17c2c2d

Change finalizer pattern to yield pattern

fc42f0b

Remove install_miniconda.sh script

bcf2a80

Fix black formatting

6b0c5f4

canyon289 force-pushed the pylint_pytest branch from eef4623 to 6b0c5f4 Compare January 18, 2019 16:19

canyon289 added 8 commits January 18, 2019 08:20

Remove duplicated normal test

85e3b21

Rename tests

067a9e2

Fix string comparison and remove comment

ae0730b

Add pymc contact info to Dockerfile

b5b7bae

Fix formatting errors

7d3a7c6

Remove pytest pin

478dde9

Add TFP issue link to test

c71658c

Remove unused numpy import

f090cee

canyon289 added 4 commits January 18, 2019 11:57

Close tf.session after each test

786b445

Fix typo in docstring

f8f86e3

Refer to distributions by direct reference

1b87e76

Slight formatting changes and docstring change

e9c42ed

twiecki merged commit 58f2fbb into pymc-devs:master Jan 19, 2019

eigenfoo mentioned this pull request Jan 19, 2019

TST: test all rvs, not just tfp supported distributions #54

Merged

Uh oh!

Local Pytest and docker TravisCI testing #49

Local Pytest and docker TravisCI testing #49

Uh oh!

Conversation

canyon289 commented Jan 14, 2019

Uh oh!

canyon289 commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

springcoil left a comment

Choose a reason for hiding this comment

Uh oh!

canyon289 commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

springcoil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

twiecki Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canyon289 Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twiecki Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

canyon289 Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twiecki Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferrine Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eigenfoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

canyon289 commented Jan 18, 2019

Uh oh!

ferrine commented Jan 18, 2019

Uh oh!

canyon289 commented Jan 18, 2019

Uh oh!

twiecki commented Jan 19, 2019

Uh oh!

eigenfoo commented Jan 20, 2019

Uh oh!

Uh oh!

canyon289 commented Jan 14, 2019 •

edited

Loading

canyon289 commented Jan 17, 2019 •

edited

Loading

twiecki Jan 18, 2019 •

edited

Loading

canyon289 Jan 18, 2019 •

edited

Loading

twiecki Jan 18, 2019 •

edited

Loading

canyon289 Jan 18, 2019 •

edited

Loading

twiecki Jan 18, 2019 •

edited

Loading

ferrine Jan 18, 2019 •

edited

Loading