pnfft: new package #2646

mkuron · 2016-12-20T08:45:45Z

PNFFT is a library for parallel non-equispaced FFTs and kind of combines #2255 (parallel FFT) and #2612 (non-equispaced FFT).

davydden · 2016-12-20T08:49:11Z

var/spack/repos/builtin/packages/pnfft/package.py

+from spack import *
+
+
+class Pnfft(AutotoolsPackage):


given those extra steps you do in install, i think it should be plain Package

The Nfft and Pfft packages also derive from AutotoolsPackage and basically have package files identical to this one. Fftw derives from Package, and it's got only slightly more extra code. So it's a bit inconsistent already.

the simple answer is that, IMHO, they both should not use AutotoolsPackage because they have custom implementation of install anyway. Look at hdf5 for a good example where def configure_args(self): is implemented in derived class.

Ok, changed it on all three packages.

davydden

LGTM apart from minor issue

…o Package

citibeth · 2016-12-20T17:14:45Z

var/spack/repos/builtin/packages/nfft/package.py

@@ -25,7 +25,7 @@
 from spack import *


-class Nfft(AutotoolsPackage):
+class Nfft(Package):


Why is this being moved from AutotoolsPackage BACK to Package? Without a good reason, that would seem to be the wrong direction.

Because we are overriding install, see @davydden's comment, and for consistency with the fftw package.

@citibeth in other words, AutotoolsPackage package is NOT used here at all, the installation is still done via custom install() as opposed to overriding def configure_args(self): which is used in AutotoolsPackage.

citibeth · 2016-12-20T17:15:23Z

var/spack/repos/builtin/packages/pnfft/package.py

+
+        configure(*options)
+        make()
+        if self.run_tests:


Shouldn't this be using Spack functionality for (1) AutotoolsPackage and (2) running tests?

No, because we need to run the entire build process three times if float and long_double are enabled.

citibeth · 2016-12-20T17:17:29Z

var/spack/repos/builtin/packages/pnfft/package.py

+            make("check")
+        make("install")
+
+        if '+float' in spec['fftw']:


Repetitive code her should be eliminated. Only the configure() statement needs to go inside the if block. (Although all this might go away with AutotoolsPackage).

More importantly... I see that this is taking options from the dependencies and propagating them UP the DAG. It looks like a neat idea to me. @adamjstewart is this kosher?

This all applies to the nfft, pfft and fftw packages too as they use the same code.

Repetitive code her should be eliminated

one could do that by defining a custom function and calling it with options (first parameter) and long double|float|... as a second one.

but my opinion is that this is not that important, so I vote to leave it as is.

citibeth · 2016-12-20T17:18:04Z

var/spack/repos/builtin/packages/pnfft/package.py

+       three-dimensional nonequispaced FFTs."""
+
+    homepage = "https://www-user.tu-chemnitz.de/~potts/workgroup/pippig/software.php.en#pnfft"
+    url      = "https://www-user.tu-chemnitz.de/~potts/workgroup/pippig/software/pnfft-1.0.7-alpha.tar.gz"


Now extra spaces after url (PEP-8 compliance)

I don't think that's a problem. Every package has spaces after url. That's how spack create initializes them.

citibeth · 2016-12-20T18:21:40Z

No, because we need to run the entire build process three times if float and long_double are enabled.

I see... I looked back at fftw package, and it is making more sense now. Still... this rubs me the wrong way (but the root of the problem is in fftw).

@tgamblin How about the alternative of having fftw/package.py just build FFTW once, depending on the variant chosen (multi-valued variants from @alalazo would be useful here)? If you want FFTW once with double and once with single, then you have Spack install FFTW twice. We can install as many different variants as we want side-by-side, that is a strength of Spack.

http://www.fftw.org/fftw3_doc/Precision.html#Precision

I really feel that doing three separate builds in one package is against the spirit of Spack, and could cause problems down the line. Moving to one build will simplify FFTW --- and also simplify things that depend on FFTW (such as pnfft).

As for forwarding variants up the DAG in this PR... the more I think about it, the more I think we should not do it, at least not here. AFAIK, this would be the fisrt Spack package to work that way, all others forward variants top-down. Whatever the merits of this type of forwrading, it's better to just follow the way things have been done before.

Putting these two suggestions together, nfft would now look like:

class Nfft(AutotoolsPackage):
    ...
    version('3.3.2', '550737c06f4d6ea6c156800169d8f0d9')

    # Only one of these three allowed at once
    variant('double', default=True)
    variant('float', default=False)
    variant('long_double', default=False)

    depends_on('fftw+double', when='+double')
    depends_on('fftw+float', when='+float')
    depends_on('fftw+long_double', when='+long_double')

    def configure_args(self):
        if '+float' in self.spec:
            return ['--enable-float']
        elif '+long_double' in self.spec:
            return ['--enable-long-double']
        elif '+double' in self.spec:
            return []

This would be simplified even further with #2386

davydden · 2016-12-20T18:27:33Z

I suppose it all boils down to this

Only one of these three allowed at once

I don't know much about packages which use fftw, but it is possible that there are packages which need all variants at once:

some part of the code is using float to do FFTW faster, and another part is using double for more precision. As I vaguely remember FFTW allows using different number types within a single application.

p.s. the situation is opposite to petsc+complex, where only real-valued or complex-valued build of PETSc can be used in a single application because they have different header files.

adamjstewart

I'm fine with the changes from AutotoolsPackage to Package. If you're not taking advantage of separate build phases, AutotoolsPackage is basically identical.

I don't mind variant forwarding up the dag. I think I've seen this once before. It's very clever, and a much better solution than trying to forward variants down the dag with our current broken logic.

I'm on the fence about whether to run configure/make/make install multiple times in the same package. It would make life easier if it only did things once and we could use AutotoolsPackage. It depends on how useful the resulting build is. I can see why someone would want to support all 3 in one library.

davydden · 2016-12-20T18:29:29Z

FFTW docs confirm that it is possible:

You can install single and long-double precision versions of FFTW, which replace double with float and long double, respectively (see Installation and Customization). To use these interfaces, you:

Link to the single/long-double libraries; on Unix, -lfftw3f or -lfftw3l instead of (or in addition to) -lfftw3. (You can link to the different-precision libraries simultaneously.)
Include the same <fftw3.h> header file.
Replace all lowercase instances of ‘fftw_’ with ‘fftwf_’ or ‘fftwl_’ for single or long-double precision, respectively. (fftw_complex becomes fftwf_complex, fftw_execute becomes fftwf_execute, etcetera.)

http://www.fftw.org/doc/Precision.html

Consequently, I am not in favour of splitting FFTW into separate packages with different number types.

citibeth · 2016-12-20T18:40:38Z

I don't know much about packages which use fftw, but it is possible that there are packages which need all variants at once:

OK, that's a convincing argument. So we're stuck with the crazy triple-build...

I'm fine with the changes from AutotoolsPackage to Package. If you're not taking advantage of separate build phases, AutotoolsPackage is basically identical.

But it might not be in the future. My feeling is... if it uses Autotools, we should extend AutotoolsPackage, even if that's a NOP for now. This will help even with simple questions, like "How many Autotools projects are in Spack?" (answerable via grep).

I don't mind variant forwarding up the dag. I think I've seen this once before. It's very clever, and a much better solution than trying to forward variants down the dag with our current broken logic.

In that case, should we be moving our other stuff to forward up the DAG? If we're going to make a change in policy, we should do it in a public way, not buried inside an obscure PR.

However... I'm not so sure it's a good idea. There's an analog here with autotools/cmake. Suppose I have feature x. One way to enable it is to root around in the system, see if the libraries that would allow me to enable x are present, and then enable it only if I find those libraries. The other way is to require the user to ask for x at the top level --- and then give an error if the libraries needed for x are not present. Over time, people have decided that the latter approach is preferable. It is more predictable and transparent to the user. Same thing with our current Spack approach of forwarding variants down.

Another reason to not start casually forwarding variants up is that the variant is then undocumented. When you say spack info, it tells you what variants a package has. If variants are forwarded up (without any additional support from Spack), then spack info nfft will not tell users that a +float variant is available (or that it needs to be enabled on fftw).

Also... forwarding variants up only works if the forwarding comes from only one dependency. The +xwindows variant (actually +X) can not work in that way; what if some of your dependencies are built +X and some not, then what variant do you infer for yourself?

Another problem is that (as done here), this trick only works for one level. If something depends on nfft, it cannot check for 'float' in spec['nfft'].

For all those reasons... let's please not go down this path until / unless we've thought it through and developed appropriate support for it. Whatever the pros/cons of the existing approach, there is no reason (that I can see) that would prevent this package from just following the established pattern.

mkuron · 2016-12-20T19:58:04Z

But it might not be in the future. My feeling is... if it uses Autotools, we should extend AutotoolsPackage, even if that's a NOP for now.

Ok, I have switched it back now and also made that change to the fftw package.

In that case, should we be moving our other stuff to forward up the DAG?

Let me add some background information about PFFT, NFFT and PNFFT: these are all (more or less) thin wrappers around FFTW. In an alternate universe, they might have been part of FFTW. Therefore, it makes no sense to build the [P|N|PN]FFT with flags other than those FFTW was built with. So even if forwarding the variant is not a good idea in general, it seems like a reasonable thing to do here.

Another problem is that (as done here), this trick only works for one level. If something depends on nfft, it cannot check for 'float' in spec['nfft'].

In that case, it would need to check for 'float' in spec['fftw'].

For all those reasons... let's please not go down this path until / unless we've thought it through and developed appropriate support for it.

Strictly speaking, we have already gone down that path. The nfft and pfft packages already do exactly the same.

citibeth · 2016-12-20T20:02:13Z

Let's see what @tgamblin thinks.

…

On Tue, Dec 20, 2016 at 2:58 PM, Michael Kuron ***@***.***> wrote: But it might not be in the future. My feeling is... if it uses Autotools, we should extend AutotoolsPackage, even if that's a NOP for now. Ok, I have switched it back now and also made that change to the fftw package. In that case, should we be moving our other stuff to forward up the DAG? Let me add some background information about PFFT, NFFT and PNFFT: these are all (more or less) thin wrappers around FFTW. In an alternate universe, they might have been *part* of FFTW. Therefore, it makes no sense to build the [P|N|PN]FFT with flags other than those FFTW was built with. So even if this is not a good idea in general, it seems like a reasonable thing to do here. Another problem is that (as done here), this trick only works for one level. If something depends on nfft, it cannot check for 'float' in spec['nfft']. In that case, it would need to check for 'float' in spec['fftw']. For all those reasons... let's please not go down this path until / unless we've thought it through and developed appropriate support for it. Strictly speaking, we have already gone down that path. The nfft and pfft packages already do exactly the same. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd1InaA8m2XQ6CnnyINmIC3lr7mMCks5rKDNNgaJpZM4LRl5H> .

adamjstewart · 2016-12-21T17:54:40Z

As far as variant forwarding up the DAG goes, @tgamblin himself recommended this approach here: #491 (comment)

citibeth · 2016-12-21T18:50:01Z

Thanks for digging that up! @davydden @becker33 I'd also like to see what he thinks about it now; especially after recent discussions/designs on universal variants (bottom-up; #2492) and variant forwarding (top-down; #2594). I feel we're in an in-between state where we know there's a problem but don't know which solution(s) we wish to take.

…

On Wed, Dec 21, 2016 at 12:54 PM, Adam J. Stewart ***@***.***> wrote: As far as variant forwarding up the DAG goes, @tgamblin <https://github.com/tgamblin> himself recommended this approach here: #491 (comment) <#491 (comment)> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd3taf9Q5q1uEQh5Kk6QgsD1Kf_z2ks5rKWfigaJpZM4LRl5H> .

becker33 · 2016-12-21T22:41:43Z

I am not opposed for forwarding variants up the DAG for bindings and other libraries that serve as wrapper layers. The key point of distinction for me is that no one uses pnfft or another wrapper without understanding that it in turn depends on fftw. I would not want to see the same forwarding applied to dealii and one of its many dependencies, because I would be concerned about that being hidden from the user.

In general, I prefer variants to be forwarded down the DAG because that allows variants to be specified at the root level from the command line, which I think is the most intuitive interface. However, I don't think that this preference should be taken as rigid even if we agree unanimously, because for any heuristic there will invariably be cases in which the opposite is the most intuitive way to do things.

citibeth · 2017-03-24T19:06:53Z

Or override other phase methods to set them to `pass`? I really think if it uses Autotools, it should subclass `AutotoolsPackage`. There are other goodies we want to add, and removing the superclass will disable this.

…

On Fri, Mar 24, 2017 at 3:04 PM, Adam J. Stewart ***@***.***> wrote: ***@***.**** requested changes on this pull request. One last request, everything else looks good to me. ------------------------------ In var/spack/repos/builtin/packages/fftw/package.py: > @@ -25,7 +25,7 @@ from spack import * -class Fftw(Package): +class Fftw(AutotoolsPackage): This change should be undone. Otherwise, the package will run ./configure and make before getting to the install() method. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2646 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd295n2EG_ziiFOkNjRZdtmkNWe7nks5rpBOkgaJpZM4LRl5H> .

adamjstewart · 2017-03-24T19:09:49Z

I don't think there's an easy way to convert either of these packages to AutotoolsPackage. The problem is that AutotoolsPackage runs ./configure, make, and make install. These packages run these steps 3 separate times. spack build pnfft wouldn't even make sense in that context.

citibeth · 2017-03-24T19:11:52Z

How about: ``` def configure(self, spec, prefix): pass def build(self, spec, prefix): pass ```

…

On Fri, Mar 24, 2017 at 3:09 PM, Adam J. Stewart ***@***.***> wrote: I don't think there's an easy way to convert either of these packages to AutotoolsPackage. The problem is that AutotoolsPackage runs ./configure, make, and make install. These packages run these steps 3 separate times. spack build pnfft wouldn't even make sense in that context. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cdzDZbemv8mBNJwId0QVXyHG0aeqSks5rpBT-gaJpZM4LRl5H> .

adamjstewart · 2017-03-24T19:14:39Z

@citibeth What would be the benefit of that...?

citibeth · 2017-03-24T19:25:01Z

For example... we are contemplating putting a standardized `+shared/static` and `+debug/run` variant onto `CMakePackage`. Then we get that functionality across the board, rather than having to implement it again and again on each package. If there is anything that Autotools-based packages do across-the-board, then we will want to implement that functionality in `AutotoolsPackage` as well. The superclass is, first and foremost, a declaration that "this package is built with Autotools." Then Spack can do all sorts of things, including running `make`, `make install`, etc, that are standard for Autootools-based packages. The superclass has semantic meaning, it is *not* just a "shortcut" on writing `install()` methods. If a package is non-standard in one way, then the semantically correct thing to do is to say "this package uses Autotools, except that these things are different." That's what overriding `configure()` and `build()` say in this case.

…

On Fri, Mar 24, 2017 at 3:14 PM, Adam J. Stewart ***@***.***> wrote: @citibeth <https://github.com/citibeth> What would be the benefit of that...? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd_x57edE6kLxZzEdG7q-Mi7powVxks5rpBYggaJpZM4LRl5H> .

adamjstewart · 2017-03-24T19:28:11Z

shrugs

I guess I'm fine with it using AutotoolsPackage if @mkuron wants to do that, and I'm fine with the way it is as well. As long as it builds 😄

mkuron · 2017-03-24T22:00:35Z

I see that class AutotoolsPackage contains phases = ['autoreconf', 'configure', 'build', 'install']. Would it make sense to replace that with phases = ['autoreconf', 'install'] in the FFTW-derived packages?, instead of passing on the non-implemented phases as suggested by @citibeth?

citibeth · 2017-03-24T22:41:04Z

Yes, that sounds like a better idea to me.

…

On Fri, Mar 24, 2017 at 6:00 PM, Michael Kuron ***@***.***> wrote: I see that class AutotoolsPackage contains phases = ['autoreconf', 'configure', 'build', 'install']. Would it make sense to replace that with phases = ['autoreconf', 'install'] in the FFTW-derived packages?, instead of passing on the non-implemented phases? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd1ae8Vw-wQbL1qCpKOGyR_0WEEzPks5rpD0EgaJpZM4LRl5H> .

mkuron · 2017-03-25T10:46:13Z

Ok, done.

alalazo · 2017-03-25T10:51:12Z

@mkuron I don't think the commands spack configure ... and spack build ... will behave as expected. Can you confirm that?

mkuron · 2017-03-25T10:55:14Z

These commands cannot behave as expected since these packages do not allow separation of the build phases. The commands will just configure/build the double-precision version.

alalazo · 2017-03-25T10:56:06Z

@mkuron If the packages are such that you can build them out of source, I think the cleanest solution will be reordering the operations, i.e. do all the configure in the configure(self, spec, prefix) callback etc.

alalazo · 2017-03-25T10:58:30Z

These commands cannot behave as expected since these packages do not allow separation of the build phases

I am not sure of that. We have multiple calls to make & Co. so it may be possible that it is a problem of our implementation rather than the packages not allowing separate configurations / builds.

mkuron · 2017-03-25T11:01:33Z

Separate out-of-source builds sound like a good idea. Will try that and update the pull request if it works.

mkuron · 2017-03-25T13:34:24Z

Ok, seems to be working. Please have a look.

adamjstewart

If this works, I like this design much better! Can you confirm that you can build all of these packages with all variants enabled?

adamjstewart · 2017-03-25T13:28:36Z

var/spack/repos/builtin/packages/fftw/package.py

        # Base options
        options = [
            '--prefix={0}'.format(prefix),
            '--enable-shared',
            '--enable-threads'
-        ]
+        ] + self.configure_args()


self.configure_args() will only contain --prefix, right? That means it will be duplicated.

No, it's empty.

Oh right, the default configure() adds --prefix to configure_args(). I still think we don't need this call if we know it's empty.

adamjstewart · 2017-03-25T13:29:49Z

var/spack/repos/builtin/packages/fftw/package.py

@@ -102,29 +103,56 @@ def install(self, spec, prefix):
            float_options.append('--enable-sse2')


I can't comment on lines that you didn't change, but there's an autoreconf call up above. Might want to move that to the autoreconf stage.

adamjstewart · 2017-03-25T13:31:59Z

var/spack/repos/builtin/packages/fftw/package.py

+            with working_dir('quad'):
+                make("install")
+
+    def check(self, spec, prefix):


I would move this between build() and install() since that's when it runs.

adamjstewart · 2017-03-25T13:33:32Z

var/spack/repos/builtin/packages/fftw/package.py


-        # Build float/long double/quad variants
+        # Build double/float/long double/quad variants
+        with working_dir('double', create=True):


It seems like double is the only one that isn't optional. Maybe we should make this one a variant too just for uniformity?

mkuron · 2017-03-25T14:06:49Z

I addressed your comments and looked over all my changes again.

I've successfully tested a representative sample of variants of FFTW along with NFFT. As PFFT and PNFFT have otherwise identical package files, those will also work.

adamjstewart · 2017-03-25T18:06:50Z

I'm sure this could be simplified further with a list of variants and for loops that declare the variants, check to see if they're activate, create a working directory, and run make. But I'm satisfied with the way things are now. Does anyone else have any requests or is this good to merge?

alalazo · 2017-03-25T18:48:51Z

@mkuron @adamjstewart If it is fine with you, I can take care of further simplifications in another PR.

* 'develop' of https://github.com/LLNL/spack: pnfft: new package (spack#2646)

@adamjstewart

* pnfft: new package * Convert some packages with overridden install from AutotoolsPackage to Package * pnfft: fix URL * Switch FFTW-derived packages back to AutotoolsPackage * Disable unneeded build phases in FFTW and derived packages * Separate build phases for FFTW and derived packages * Fix broken merge * fftw: pfft_patches for 3.3.6 * fftw: address @adamjstewart’s review comments

@adamjstewart

* pnfft: new package * Convert some packages with overridden install from AutotoolsPackage to Package * pnfft: fix URL * Switch FFTW-derived packages back to AutotoolsPackage * Disable unneeded build phases in FFTW and derived packages * Separate build phases for FFTW and derived packages * Fix broken merge * fftw: pfft_patches for 3.3.6 * fftw: address @adamjstewart’s review comments

@adamjstewart

* pnfft: new package * Convert some packages with overridden install from AutotoolsPackage to Package * pnfft: fix URL * Switch FFTW-derived packages back to AutotoolsPackage * Disable unneeded build phases in FFTW and derived packages * Separate build phases for FFTW and derived packages * Fix broken merge * fftw: pfft_patches for 3.3.6 * fftw: address @adamjstewart’s review comments

@adamjstewart

* pnfft: new package * Convert some packages with overridden install from AutotoolsPackage to Package * pnfft: fix URL * Switch FFTW-derived packages back to AutotoolsPackage * Disable unneeded build phases in FFTW and derived packages * Separate build phases for FFTW and derived packages * Fix broken merge * fftw: pfft_patches for 3.3.6 * fftw: address @adamjstewart’s review comments

pnfft: new package

d2c1316

davydden reviewed Dec 20, 2016

View reviewed changes

davydden approved these changes Dec 20, 2016

View reviewed changes

Convert some packages with overridden install from AutotoolsPackage t…

ff83585

…o Package

citibeth reviewed Dec 20, 2016

View reviewed changes

pnfft: fix URL

81829cb

adamjstewart reviewed Dec 20, 2016

View reviewed changes

Switch FFTW-derived packages back to AutotoolsPackage

5cc36bf

mkuron and others added 2 commits March 25, 2017 11:42

Disable unneeded build phases in FFTW and derived packages

5be8662

Merge branch 'develop' into pnfft

53e0568

mkuron added 4 commits March 25, 2017 14:13

Separate build phases for FFTW and derived packages

ca40c3d

Merge branch 'pnfft' of github.com:mkuron/spack into pnfft

88372e6

Fix broken merge

07c9e4d

fftw: pfft_patches for 3.3.6

92dd815

adamjstewart requested changes Mar 25, 2017

View reviewed changes

fftw: address @adamjstewart’s review comments

bf8233c

adamjstewart approved these changes Mar 25, 2017

View reviewed changes

davydden approved these changes Mar 25, 2017

View reviewed changes

adamjstewart merged commit 55300d1 into spack:develop Mar 25, 2017

gmatteo added a commit to gmatteo/spack that referenced this pull request Mar 25, 2017

Merge branch 'develop' of https://github.com/LLNL/spack into develop

6aaf36f

* 'develop' of https://github.com/LLNL/spack: pnfft: new package (spack#2646)

citibeth mentioned this pull request Apr 19, 2017

Add variant +wide for ncurses #3870

Closed

		@@ -102,29 +103,56 @@ def install(self, spec, prefix):
		float_options.append('--enable-sse2')

pnfft: new package #2646

pnfft: new package #2646

Conversation

mkuron commented Dec 20, 2016

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

davydden 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

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

citibeth commented Dec 20, 2016

davydden commented Dec 20, 2016 • edited Loading

adamjstewart left a comment

Choose a reason for hiding this comment

davydden commented Dec 20, 2016 • edited Loading

citibeth commented Dec 20, 2016

mkuron commented Dec 20, 2016 • edited Loading

citibeth commented Dec 20, 2016 via email

adamjstewart commented Dec 21, 2016

citibeth commented Dec 21, 2016 via email

becker33 commented Dec 21, 2016

citibeth commented Mar 24, 2017 via email

adamjstewart commented Mar 24, 2017

citibeth commented Mar 24, 2017 via email

adamjstewart commented Mar 24, 2017

citibeth commented Mar 24, 2017 via email

adamjstewart commented Mar 24, 2017

mkuron commented Mar 24, 2017 • edited Loading

citibeth commented Mar 24, 2017 via email

mkuron commented Mar 25, 2017

alalazo commented Mar 25, 2017

mkuron commented Mar 25, 2017

alalazo commented Mar 25, 2017

alalazo commented Mar 25, 2017 • edited Loading

mkuron commented Mar 25, 2017

mkuron commented Mar 25, 2017

adamjstewart 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

mkuron commented Mar 25, 2017

adamjstewart commented Mar 25, 2017

alalazo commented Mar 25, 2017 • edited Loading

davydden commented Dec 20, 2016 •

edited

Loading

davydden commented Dec 20, 2016 •

edited

Loading

mkuron commented Dec 20, 2016 •

edited

Loading

mkuron commented Mar 24, 2017 •

edited

Loading

alalazo commented Mar 25, 2017 •

edited

Loading

alalazo commented Mar 25, 2017 •

edited

Loading