New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for issue #1240. #1247

Merged
merged 6 commits into from Sep 15, 2015

Conversation

Projects
None yet
3 participants
@kwmsmith
Member

kwmsmith commented Sep 14, 2015

Calling _str() on a tuple of expressions triggers a __repr__ call on
each element, resulting in unnecessary computation.

This fix detects when stringifying a tuple of exprs, and calls _str()
recursively in that case.

Fix for issue #1240.
Calling _str() on a tuple of expressions triggers a __repr__ call on
each element, resulting in unnecessary computation.

This fix detects when stringifying a tuple of exprs, and calls _str()
recursively in that case.
@@ -290,6 +290,8 @@ def _str(s):
return get_callable_name(s)
elif isinstance(s, Node):
return str(s)
elif isinstance(s, (list, tuple)):
return tuple(_str(x) for x in s)
else:
stream = StringIO()
pprint(s, stream=stream)

This comment has been minimized.

@llllllllll

llllllllll Sep 14, 2015

Member

Sort of unrelated, but the else clause is just return pformat(s).rstrip()

@llllllllll

This comment has been minimized.

Member

llllllllll commented Sep 14, 2015

awesome find

@cpcloud

This comment has been minimized.

Member

cpcloud commented Sep 14, 2015

@kwmsmith Nice job! Can you add a test? Something like this:

def test_str_does_not_repr():
    d = Data([('aa', 1), ('b', 2)], dshape='2 * {a: string, b: int64}')
    expr = transform(d, c=d.a.strlen() + d.b)
    assert str(expr) == 'make sure this does not contain the repr of the underlying data'
@cpcloud

This comment has been minimized.

Member

cpcloud commented Sep 14, 2015

i'd put that in tests/test_interactive.py

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Sep 14, 2015

@cpcloud done. Pls take a look at the test -- I have a feeling there's a better way to do the assert.

# see GH issue #1240.
d = Data([('aa', 1), ('b', 2)], dshape='2 * {a: string, b: int64}')
expr = transform(d, c=d.a.strlen() + d.b)
assert sub(r'_\d+', 'XXX', str(expr)) == "Merge(_child=XXX, children=(XXX, label(strlen(_child=XXX.a) + XXX.b, 'c')))"

This comment has been minimized.

@cpcloud

cpcloud Sep 14, 2015

Member

small nitpick can you keep the re.sub call style?

i also think that even though the name of the expression is an implementation detail it should be consistently numbered and therefore it'd be okay to not do the replace and just keep _1 or whatever it happens to be

This comment has been minimized.

@kwmsmith

kwmsmith Sep 15, 2015

Member

I thought of doing that, but my concern is that the _149 (I think that's the symbol) is not necessarily reproducible run-to-run, especially with py.test running things in parallel. Is it reproducible? If so, then I'll make the replacement and remove the re.sub call.

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Sep 15, 2015

The generated symbol name appears to be test order dependent. When I run the standard testsuite command (py.test --doctest-modules --pyargs blaze) it's assigned _179, and when I run py.test -x --pyargs blaze, it's assigned _177:

$ py.test --doctest-modules --pyargs blaze

[...]
=============================================================== FAILURES ===============================================================
________________________________________________________ test_str_does_not_repr ________________________________________________________

    def test_str_does_not_repr():
        # see GH issue #1240.
        d = Data([('aa', 1), ('b', 2)], dshape='2 * {a: string, b: int64}')
        expr = transform(d, c=d.a.strlen() + d.b)
>       assert str(expr) == "Merge(_child=_149, children=(_149, label(strlen(_child=_149.a) + _149.b, 'c')))"
E       assert "Merge(_child...179.b, 'c')))" == "Merge(_child=...149.b, 'c')))"
E         - Merge(_child=_179, children=(_179, label(strlen(_child=_179.a) + _179.b, 'c')))
E         ?                ^               ^                         ^         ^
E         + Merge(_child=_149, children=(_149, label(strlen(_child=_149.a) + _149.b, 'c')))
E         ?                ^               ^                         ^         ^

blaze/tests/test_interactive.py:102: AssertionError
========================== 1 failed, 1014 passed, 44 skipped, 27 xfailed, 3 xpassed, 2 error in 12.33 seconds ==========================

And:

$ py.test -x --pyargs blaze
[...]
=============================================================== FAILURES ===============================================================
________________________________________________________ test_str_does_not_repr ________________________________________________________

    def test_str_does_not_repr():
        # see GH issue #1240.
        d = Data([('aa', 1), ('b', 2)], dshape='2 * {a: string, b: int64}')
        expr = transform(d, c=d.a.strlen() + d.b)
>       assert str(expr) == "Merge(_child=_149, children=(_149, label(strlen(_child=_149.a) + _149.b, 'c')))"
E       assert "Merge(_child...177.b, 'c')))" == "Merge(_child=...149.b, 'c')))"
E         - Merge(_child=_177, children=(_177, label(strlen(_child=_177.a) + _177.b, 'c')))
E         ?                ^^              ^^                        ^^        ^^
E         + Merge(_child=_149, children=(_149, label(strlen(_child=_149.a) + _149.b, 'c')))
E         ?                ^^              ^^                        ^^        ^^

blaze/tests/test_interactive.py:102: AssertionError
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
=============================== 1 failed, 819 passed, 41 skipped, 26 xfailed, 2 xpassed in 11.44 seconds ===============================
@llllllllll

This comment has been minimized.

Member

llllllllll commented Sep 15, 2015

Can you explicitly name the interactive symbol? Does the name argument appear as the name in the str?

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Sep 15, 2015

@llllllllll thanks for pointing that out, it works great.

cpcloud added a commit that referenced this pull request Sep 15, 2015

@cpcloud cpcloud merged commit 8586636 into master Sep 15, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@cpcloud cpcloud deleted the bugfix/1240 branch Sep 15, 2015

@cpcloud

This comment has been minimized.

Member

cpcloud commented Sep 15, 2015

@kwmsmith Thanks for finding and fixing this!

@cpcloud cpcloud added this to the 0.8.3 milestone Sep 15, 2015

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Sep 15, 2015

@cpcloud @llllllllll I'm glad we got it fixed and that the fix itself was straightforward. Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment