Skip to content
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
Merged

Fix for issue #1240. #1247

merged 6 commits into from
Sep 15, 2015

Conversation

kwmsmith
Copy link
Member

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.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@llllllllll
Copy link
Member

awesome find

@cpcloud
Copy link
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
Copy link
Member

cpcloud commented Sep 14, 2015

i'd put that in tests/test_interactive.py

@kwmsmith
Copy link
Member Author

@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')))"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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
Copy link
Member

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

@kwmsmith
Copy link
Member Author

@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
@cpcloud cpcloud deleted the bugfix/1240 branch September 15, 2015 17:17
@cpcloud
Copy link
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
Copy link
Member Author

@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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants