Remove has no python #483

bryanwweber · 2017-11-08T21:23:14Z

Changes proposed in this pull request:

Remove the HAS_NO_PYTHON configuration variable to allow building just the library, then adding the Python interfaces without rebuilding everything. See https://groups.google.com/forum/#!topic/cantera-users/a3IK3A4XEfI
Rework SConstruct to recognize that SCons can be run by Python 3 now, and remove the assumption that Python 2 is running SCons. This still keeps the ability to build both interfaces simultaneously, but attempts to make the options more consistent
Also make the minimal Python interface compatible with and able to be installed for Python 3

I'd appreciate some feedback on this, since the changes are pretty big to SConstruct. AFAICT, everything should keep working with old versions of SCons. This is still WIP because the tests aren't working yet, and I need to update the documentation (if this gets approved). Would still appreciate some feedback while I try to fix the tests.

speth

There are a couple of places where HAS_NO_PYTHON is still referenced in the test suite, which should be cleaned up.

The mess involved in iterating over Python 2 and 3 certainly makes me look forward to dropping support for Python 2...

speth · 2017-11-09T11:20:44Z

SConstruct

@@ -109,6 +109,10 @@ if 'clean' in COMMAND_LINE_TARGETS:
    else:
        Alias('clean', [])

+if 'test-clean' in COMMAND_LINE_TARGETS:


This command needs to be documented in the docstring for SConstruct (and possibly elsewhere)

Yes, I need to update the docs for the new python2_* options I added too. And probably also describe the small changes in behavior.

OK I updated the docs for most things, but this command was already documented in the docstring for SConstruct. The bit added here just cleans up some more files that weren't being cleaned up before.

Oops, sorry, my mistake. I forgot that this was already a command. 😬

speth · 2017-11-09T12:59:48Z

SConstruct

+    if env['python2_package'] == 'default':
+        env['python2_package'] = 'n'
+    if env['python3_package'] == 'default':
+        env['python3_package'] = 'n'


The end effect is the same, but if none is the canonical name, we should use that here.

speth · 2017-11-09T13:13:41Z

SConstruct

+            sys.exit(1)
+        elif want_python:
+            print('WARNING: ' + message)
+            env['python_package'] = 'minimal'


Does setting python_package at this point have the desired effect, or does the code above which sets python2_package and python3_package based on python_package need to come after this point?

I think this has the desired effect (at least, I'm pretty sure its doing what I expect it to do; whether that's desired or not is a different question 😄).

If any of python_package, python2_package, or python3_package are full (after the code above, y and new become full), require_python is True, and if Cython can't be imported with the python_cmd interpreter, we want to error out, because the build can't be completed.

If all of python_package, python2_package, and python3_package are 'default' (or any combination are 'none' (but not all of them 'none') with any one of them as 'default'), then require_python will be False and want_python will be True. In that case, if we can't find Cython, python_package will be set to 'minimal' and the flow will continue to check what version of Python is specified in python_cmd. Then, the Python in python_cmd will be used to build the minimal interface for that version of Python (provided that the python*_package that matches the version of Python in python_cmd is not specified).

That's what I wanted this line to do, and I'm pretty sure that's what its doing.

speth · 2017-11-09T13:26:21Z

SConstruct

+            print("ERROR: The python_package points to Python {v} and python{v}_package has been "
+                  "specified. Please change python_package to point to a different version of "
+                  "Python or disable it.".format(v=major))
+            sys.exit(1)


I find this error message confusing. Is the idea that specifying both python_package and whichever of python3_package or python2_package corresponds to python_cmd is not allowed?

Should we allow this if the settings are not incompatible?

Yes, that's the intention. How would we determine if the settings are incompatible? What if python_cmd points to Python 3.5 and python3_cmd points to Python 3.6? Which should we build? I think it simpler if we just error out when both are specified, although the error message could certainly be improved.

speth · 2017-11-09T13:29:35Z

SConstruct

+            sys.exit(1)
+        elif env[py_pkg] == 'default':
+            py_vars = {'python{}_array_home': '', 'python{}_cmd': 'python{}'.format(major),
+                       'python{}_prefix': '$prefix'}


Shouldn't these values correspond to the values of env['python_cmd'] etc.?

These are the default values for these variables. I'm checking whether the value present in the env is equal to the default. This could be clearer, I'll clean it up.

speth · 2017-11-09T13:31:09Z

SConstruct

+                            "    import numpy",
+                            "    print(numpy.__version__)",
+                            "except ImportError as err:",
+                            # "    print(err, file=sys.stderr)",


Why not print the error?

The error is reported later, so the printing here is duplicate (and not very clear, because it doesn't say which interface is reporting the error).

speth · 2017-11-09T13:39:42Z

SConstruct

@@ -1787,6 +1821,9 @@ if 'msi' in COMMAND_LINE_TARGETS:

 ### Tests ###
 if any(target.startswith('test') for target in COMMAND_LINE_TARGETS):
+    if env['python2_package'] == 'none' and env['python3_package'] == 'none':
+        env['python{}_package'.format(env['python_version'][0])] = 'minimal'
+        SConscript('interfaces/python_minimal/SConscript')


I wonder whether this is insufficient to do what you presumably want, and doesn't trigger the 'build' actions in the SConscript file when you only run scons test.

This is indeed insufficient, and I need to fix this still.

speth · 2017-11-09T13:43:49Z

interfaces/cython/SConscript

@@ -172,7 +170,7 @@ if localenv['python_package'] == 'full':
    py2env['py_extension'] = ext[0].name
    py2env.SubstFile('setup2.py', 'setup.py.in')
    build_cmd = ('cd interfaces/cython &&'
-                 ' $python_cmd_esc setup2.py build --build-lib=../../build/python2')
+                 ' $python2_cmd setup2.py build --build-lib=../../build/python2')


I think we will continue to need to use a quoted/escaped Python command for the same reason it was originally introduced (see 4f3dd06)

But the Python 3 interface does not use the quoted command. Should that be changed to use a quoted command?

Yes, presumably. I guess the quoting can be done more locally than in the current version.

Shouldn't we then use the escaped command everywhere? Like, all the places we check for NumPy, in the tests, etc?

speth · 2017-11-09T13:44:35Z

interfaces/python_minimal/SConscript

+            localenv.Depends(make_setup, s)
+
+        build_cmd = ('cd interfaces/python_minimal &&'
+                     ' $python{v}_cmd setup{v}.py build --build-lib=../../build/python{v}'.format(v=v))


likewise here

bryanwweber · 2017-11-09T14:12:26Z

I was thinking this morning that it would probably make sense to abstract all the Python checking code into a function so that the loop is neater, so I'll do that. It will indeed be nice when we can drop Python 2, but then they'll release Python 4 which won't be compatible with Python 3 😃

speth · 2017-11-09T14:55:11Z

Hopefully the decade of growing pains associated with the Python 2 to 3 transition will convince them never to make a Python 4.

bryanwweber · 2017-11-10T23:02:42Z

@speth What is the python_module_loc_sc key for in the platform/posix/SConscript and interfaces/matlab/SConscript? The reason I'm asking is because I'm trying to figure out how to set that based on which Python interface is installed. Actually, I think it would be better if we didn't mess with people's PYTHONPATH, but given that we are doing that, if both interfaces are built, which should be put in the PYTHONPATH? We shouldn't put both because that will cause both interfaces to be unable to be imported (I think, maybe its only the later of the two that can't be imported).

Also, what is the purpose of this logic in setup_cantera:

if [ "@python_module_loc_sc@" != "" ]; then
    if [ -z $PYTHONPATH ]; then
        PYTHONPATH=@python_module_loc@
    else
        PYTHONPATH=@python_module_loc@:$PYTHONPATH
    fi
fi

Why do we check the value of python_module_loc_sc in setup_cantera, and then set the PYTHONPATH based on just python_module_loc?

Also, in those same two SConscript files, why do we depend on install_python2_action? It seems like at least the substitution and installation of the setup_cantera script is done even if the Python 2 interface isn't built at all.

bryanwweber · 2017-11-13T16:42:53Z

interfaces/cython/SConscript

+        """)
+        b = build(py2env.Command([], '#build/python2/cantera/examples', convert_script))
+        py2env.Depends(b, a)
+        py2env.Depends(mod, b)


@speth I'm really not sure what's going on here. I'm trying to run the converter before the module is built, and after the example files are copied to the right place. Instead, the setup script is copying the example files and isn't converting them. I don't see how what I'm doing here is different from how it used to be in terms of the dependency, but clearly something is different. Any advice would be appreciated.

Is it because the Depends is on a folder, rather than a set of files?

I think the issue is that in your new way, b is an empty list (because you specify an empty list as the target for the Command), so there's no state for SCons to base the dependency on. I think what you actually want here is env.AddPostAction, which will run after a, but only if the a action has to run in the first place.

bryanwweber · 2017-11-13T16:44:40Z

OK so the tests all pass now (but installation is still broken, pending clarification of my other comment above), but 3to2 can't be found because I changed how the executable was found. So rather than relying on the executable, who's installation location is platform- and installation-method-dependent, I thought it would be better to run the converter function directly from the library. However, as I mentioned in my comment, its not actually running that code, so I don't know what's going wrong.

speth · 2017-11-17T18:10:58Z

What is the python_module_loc_sc key for in the platform/posix/SConscript and interfaces/matlab/SConscript?
...
Why do we check the value of python_module_loc_sc in setup_cantera, and then set the PYTHONPATH based on just python_module_loc?

The python_module_loc_sc variable is different from python_module_loc to avoid appending the user site-packages directory to the PYTHONPATH, since it should already be in sys.path. I think it shouldn't matter whether we add python_module_loc_sc or python_module_loc in the end, since if the former is non-empty, they should be the same.

Actually, I think it would be better if we didn't mess with people's PYTHONPATH

What's the alternative, though, if the user is installing to a directory that's not on sys.path? I feel like we would end up with a lot of support request, similar to how adding the Matlab toolbox to the Matlab path generates so much difficulty for many users. We're only modifying the path if the user sources the setup_cantera script, in any case -- it's not like we're editing their .bashrc.

if both interfaces are built, which should be put in the PYTHONPATH

Beats me. I have no idea why both Python 2 and Python 3 look at the same variable for this, it makes working with both simultaneously a nightmare. I guess at this point I would default to the Python 3 module location if both are built?

bryanwweber · 2017-11-17T19:04:43Z

The python_module_loc_sc variable is different from python_module_loc to avoid appending the user site-packages directory to the PYTHONPATH, since it should already be in sys.path. I think it shouldn't matter whether we add python_module_loc_sc or python_module_loc in the end, since if the former is non-empty, they should be the same.

OK, I suspected that was the case, but glad to have confirmation.

What's the alternative, though, if the user is installing to a directory that's not on sys.path?

How often do users install to non-standard paths? I guess this is something we have no way of knowing, and you're right that PYTHONPATH is only set if the user sources setup_cantera, so would go away when that shell is closed (unless they source setup_cantera in their .bashrc (which we do potentially recommend; from the post-install message: before using Cantera, or else include its contents in your shell login script.)).

A potentially better way of doing this is to install a cantera.pth file to ~/.local/lib/pythonX.Y/site-packages that points to the installed location. This way, both interfaces would be found in whatever location they were installed, and no editing of PYTHONPATH would be necessary. The disadvantage is that it might cause problems importing the proper version if two versions of the same interface are installed to different directories (e.g., for testing). I guess since we haven't had any reports of problems with PYTHONPATH, there's no reason to change, but we can keep the .pth file method in mind if necessary in the future.

FWIW, this would work like

bryan@solus ~$ echo "/home/bryan/canteratest" > ~/.local/lib/python3.6/site-packages/cantera.pth
bryan@solus ~$ python
Python 3.6.3 |Anaconda, Inc.| (default, Oct  6 2017, 17:14:46) 
[GCC 7.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path
['', '/home/bryan/miniconda3/lib/python36.zip', '/home/bryan/miniconda3/lib/python3.6', '/home/bryan/miniconda3/lib/python3.6/lib-dynload', 
'/home/bryan/.local/lib/python3.6/site-packages', '/home/bryan/canteratest', '/home/bryan/miniconda3/lib/python3.6/site-packages']

Note that the /home/bryan/canteratest/ directory has to exist for it to show up in the path (although it can be empty).

I guess at this point I would default to the Python 3 module location if both are built?

Will do. What should I do about this line:

cantera/platform/posix/SConscript

Line 26 in 684262e

localenv.Depends(target, env['install_python2_action'])

Change it to Python 3 if both are installed or Python 3 is by itself and Python 2 if only Python 2 is built? I don't really understand why we're adding this dependence at all though, actually...

speth · 2017-11-18T02:42:28Z

A potentially better way of doing this is to install a cantera.pth file to ~/.local/lib/pythonX.Y/site-packages that points to the installed location.

I don't really understand the point of .pth files, and have found that at least for me, they cause more problems than they solve. If you can write to ~/.local/lib/pythonX.Y/site-packages, why not just install the module to that location? One important use case for having multiple versions of Cantera installed in different locations is to be able to have both development and stable versions available, and I think the only way of switching between those is by using PYTHONPATH.

Change it to Python 3 if both are installed or Python 3 is by itself and Python 2 if only Python 2 is built?

Yes, if we have are building the Python 3 module, then I think this should be for the Python 3 install action, and we should be using the python3_module_loc variable and setting python_module_loc_sc from that as well. The reason that the action of determining python_module_loc depends on the module installation is that we figure out the actual location of the installation directory by looking at the record of installed file locations -- it's not known a priori.

bryanwweber · 2017-11-18T16:19:16Z

If you can write to ~/.local/lib/pythonX.Y/site-packages, why not just install the module to that location?

Presumably, the user doesn't want to install to that directory for some reason. However, it should be writable in general, and if we were to remove the PYTHONPATH setting, it would probably be the most likely place to install a .pth file as a replacement for PYTHONPATH.

I don't really understand the point of .pth files, and have found that at least for me, they cause more problems than they solve... One important use case for having multiple versions of Cantera installed in different locations is to be able to have both development and stable versions available, and I think the only way of switching between those is by using PYTHONPATH.

Yes, I agree with that use case, that would be the disadvantage of the .pth files. Leaving PYTHONPATH is fine with me.

Yes, if we have are building the Python 3 module, then I think this should be for the Python 3 install action, and we should be using the python3_module_loc variable and setting python_module_loc_sc from that as well.

👍

The reason that the action of determining python_module_loc depends on the module installation is that we figure out the actual location of the installation directory by looking at the record of installed file locations -- it's not known a priori.

Ah, that makes much more sense, thanks!

bryanwweber · 2017-11-19T17:59:20Z

In f982156 I reformatted the postInstallMessage. If you think this should be dropped, that's fine with me, but I made the change because the indentation all over the place was making my eyes bleed 👀 🤕 😃 Also, I updated it to use the format method... I presume it was using the old % method because of Python 2.5 compatibility, but I don't think that's necessary anymore, so in cleaning it up I made the switch.

Provided that the tests pass and there are no further requested changes, I think this is good to go.

speth · 2017-11-24T22:47:14Z

SConstruct

+    try:
+        print(site.getusersitepackages())
+    except AttributeError:
+        print(site.USER_SITE)""")


What is the prefered indentation following textwrap.dedent? I see this style used in the documentation for the textwrap module, but honestly, I think it looks horrible -- it's very hard to see where the triple-quoted string ends. Wouldn't it be better to indent this one level?

Do you mean like

script = textwrap.dedent("""\ from __future__ import print_function import sys print('{v.major}.{v.minor}'.format(v=sys.version_info)) try: import numpy print(numpy.__version__) except ImportError: print('0.0.0') import site try: print(site.getusersitepackages()) except AttributeError: print(site.USER_SITE) """)

speth · 2017-11-24T22:51:10Z

SConstruct

+    if env['python2_package'] == 'full':
+        env['python2_example_loc'] = pjoin(env['python2_module_loc'], 'cantera', 'examples')
+        install_message += textwrap.indent(textwrap.dedent("""
+            Python 2 package (cantera)  {python2_module_loc!s}


textwrap.indent is new as of Python 3.3, so we can't use it.

I guess this also means our automated tests don't run the install action, so this isn't getting tested, which probably ought to be fixed as well.

OK, I guess I'll write my own indent function for this specific case. Can't wait until Python 2 is done...

Yes, I realized that as well, but didn't want to make this PR any bigger... it already blew up a little bit 😄

speth · 2017-11-24T22:59:20Z

interfaces/cython/SConscript

    else:
        # Install Python module in the default location
        extra = ''

-    env['python%s_module_loc' % ver] = '<unspecified>'
+    env['python%s_module_loc' % major] = '<unspecified>'
+    localenv['python{}_cmd_esc'.format(major)] = quoted(env['python{}_cmd'.format(major)])


Isn't this redundant with the setting of pythonX_module_loc in the block for each Python version below before the install_module function is called?

As best I can tell, pythonX_module_loc is only set in the install_module function. This is set to the empty string in SConstruct, but only if that Python interface isn't being built.

Sorry, I got confused and read the wrong line while writing that -- I meant pythonX_cmd_esc.

bryanwweber · 2017-11-25T16:11:14Z

In 1c46173 I fixed a problem with parallel building of the Matlab interface on macOS. Prior to that change, building in parallel with the Matlab interface was giving

g++ -o interfaces/matlab/toolbox/ctmethods.mexmaci64 -Wl,-exported_symbol,_mexFunction -dynamiclib build/src/matlab/ctfunctions.os build/src/matlab/ctmethods.os build/src/matlab/flowdevicemethods.os build/src/matlab/funcmethods.os build/src/matlab/kineticsmethods.os build/src/matlab/mixturemethods.os build/src/matlab/onedimmethods.os build/src/matlab/phasemethods.os build/src/matlab/reactormethods.os build/src/matlab/reactornetmethods.os build/src/matlab/reactorsurfacemethods.os build/src/matlab/surfmethods.os build/src/matlab/thermomethods.os build/src/matlab/transportmethods.os build/src/matlab/wallmethods.os build/src/matlab/xmlmethods.os -Lbuild/lib -L/Applications/MATLAB_R2016b.app/bin/maci64 -lcantera -lmx -lmex -lmat -framework Accelerate
ld: archive has no table of contents file 'build/lib/libcantera.a' for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
scons: *** [interfaces/matlab/toolbox/ctmethods.mexmaci64] Error 1
ranlib build/lib/libcantera.a
scons: building terminated because of errors.

which I think was because the Matlab library was building against the static Cantera library which wasn't finished yet. However, a few lines below my change, we set the Matlab library to depend on the Cantera shared library, which is why I changed to put the shared libs in the link line. Is that the right fix?

speth · 2017-11-25T16:18:59Z

I don't recall offhand the reason why we link to the static library on macOS, but I'd rather leave that alone and change the Depends to the static library instead. This should at least avoid introducing any new bugs.

bryanwweber · 2017-11-25T16:41:19Z

OK I made the change to the Depends line

speth · 2017-11-25T17:04:34Z

OK, I think this is fine as far as the end state goes. Could you squash a few of the commits that are fixups internal to this PR, to reduce the amount of churn, and then we can merge this?

This variable is not needed since in the two places its used we do a runtime check of the presence of the Python interface anyways

Use relative imports in the package, compatible with Python 2 and 3

If no full or minimal Python interface is being built, copy the minimal interface into the build directory and use the sys.executable to run it, so the tests that require CTML or CTI conversion can run.

bryanwweber · 2017-11-25T20:31:44Z

Done and done! Thanks for all the comments, very helpful to understand how SCons works a bit better 😄

speth · 2017-11-25T23:37:56Z

Sorry, just found a couple more problems when trying this PR on the Buildbot builders (http://gir.mit.edu:8010/waterfall) -- there appear to be a couple of uses of python_prefix after it's been deleted, in SConstruct, on lines 1407 and 1433, both of which I think should be python2_prefix, correct?

Update and make more consistent the specification of Python package building. Since SCons can be run by Python 3 now, we cannot assume that the Python running SCons is Python 2. This changes a bunch of assumptions in SConstruct about where things should be built or installed. This commit addresses those assumptions by making the options for Python 2 and Python 3 symmetric.

Use Python 3 variables by default, and Python 2 only if Python 3 isn't being built

Switch to importing the lib3to2 as a check, which is platform agnostic and doesn't depend on how 3to2 was installed. Also, take advantage of the fact that the 3to2 converter recurses by default to avoid spawning a bunch of subprocesses. Finally, don't depend on the location of the 3to2 script and just use the library directly to do the conversion.

The triple quoted strings made 3to2 think that was the module docstring, so it was putting __future__ imports in the wrong places

Building in parallel would error out because the macOS Matlab library was relying on the static Cantera library before that was finished.

bryanwweber · 2017-11-26T01:51:35Z

@speth Good catch, thanks! Fixed now. This is why I deleted those variables 😄 although clearly I didn't test the packaging machinery

bryanwweber requested a review from speth November 8, 2017 21:23

speth reviewed Nov 9, 2017

View reviewed changes

bryanwweber commented Nov 13, 2017

View reviewed changes

bryanwweber force-pushed the remove_has_no_python branch from 6db0057 to 5e0685b Compare November 17, 2017 23:08

bryanwweber changed the title ~~[WIP] Remove has no python~~ Remove has no python Nov 19, 2017

speth requested changes Nov 24, 2017

View reviewed changes

bryanwweber force-pushed the remove_has_no_python branch from 1c46173 to 052a04f Compare November 25, 2017 16:39

bryanwweber added 4 commits November 25, 2017 15:11

Remove HAS_NO_PYTHON configuration variable

661545f

This variable is not needed since in the two places its used we do a runtime check of the presence of the Python interface anyways

Remove testing related files if test-clean is specified

6c7e39c

Make the minimal Python interface compatible with Python 3

bd0df2f

Use relative imports in the package, compatible with Python 2 and 3

Use the minimal Python interface for tests when others are not built

f261b58

If no full or minimal Python interface is being built, copy the minimal interface into the build directory and use the sys.executable to run it, so the tests that require CTML or CTI conversion can run.

bryanwweber force-pushed the remove_has_no_python branch from 052a04f to 17f8dd6 Compare November 25, 2017 20:30

bryanwweber added 2 commits November 25, 2017 20:47

Fix substitution of setup_cantera and ctpath.m

80c2760

Use Python 3 variables by default, and Python 2 only if Python 3 isn't being built

bryanwweber force-pushed the remove_has_no_python branch from 17f8dd6 to 37392a0 Compare November 26, 2017 01:47

bryanwweber added 6 commits November 25, 2017 20:51

Update documentation for new Python configuration options

290f2fb

Update options for Travis Python tests

914912b

Fix 3to2 conversion of NonIdealShockTube example

b96421f

The triple quoted strings made 3to2 think that was the module docstring, so it was putting __future__ imports in the wrong places

Reformat postInstallMessage code to be easier to read in SConstruct

d524664

Fix dependency problem with matlab library

4447830

Building in parallel would error out because the macOS Matlab library was relying on the static Cantera library before that was finished.

bryanwweber force-pushed the remove_has_no_python branch from 37392a0 to 4447830 Compare November 26, 2017 01:51

speth approved these changes Nov 26, 2017

View reviewed changes

speth merged commit 8312396 into Cantera:master Nov 26, 2017

bryanwweber deleted the remove_has_no_python branch November 27, 2017 12:45

Remove has no python #483

Remove has no python #483

Conversation

bryanwweber commented Nov 8, 2017 • edited Loading

speth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanwweber Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanwweber commented Nov 9, 2017

speth commented Nov 9, 2017

bryanwweber commented Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanwweber commented Nov 13, 2017 • edited Loading

speth commented Nov 17, 2017

bryanwweber commented Nov 17, 2017 • edited Loading

speth commented Nov 18, 2017

bryanwweber commented Nov 18, 2017

bryanwweber commented Nov 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanwweber Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanwweber commented Nov 25, 2017

speth commented Nov 25, 2017

bryanwweber commented Nov 25, 2017

speth commented Nov 25, 2017

bryanwweber commented Nov 25, 2017

speth commented Nov 25, 2017

bryanwweber commented Nov 26, 2017 • edited Loading

bryanwweber commented Nov 8, 2017 •

edited

Loading

bryanwweber Nov 10, 2017 •

edited

Loading

bryanwweber commented Nov 10, 2017 •

edited

Loading

bryanwweber commented Nov 13, 2017 •

edited

Loading

bryanwweber commented Nov 17, 2017 •

edited

Loading

bryanwweber Nov 25, 2017 •

edited

Loading

bryanwweber commented Nov 26, 2017 •

edited

Loading