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

Avoid nesting testsets in test_rule #158

Merged
merged 2 commits into from
Jun 1, 2021
Merged

Avoid nesting testsets in test_rule #158

merged 2 commits into from
Jun 1, 2021

Conversation

oxinabox
Copy link
Member

Combined with the other PRs i have open this should close #146.

We don't nest testsets any more, except in test_scalar (and I don't think thats much of a problem just cos scalars rarely fail their pullbacks are simple).
Instead we use a hack to make @test print a message on failure.
and then we plumb that around as needed.

Example of one of the place we would nest a test set before. Mocking up the outer set just for this demo

julia> @testset "rrule for foo at bar" begin
           check_equal(Tangent{Foo}(1, 2), Tangent{Foo}(1, 3))
       end
rrule for foo at bar: Test Failed at /Users/oxinabox/JuliaEnvs/ChainRulesWorld/ChainRules.jl/dev/ChainRulesTestUtils/src/check_result.jl:25
  Expression: isapprox(actual, expected; kwargs...)
  Problem:  Foo.2
   Evaluated: isapprox(2, 3)
Stacktrace:
 [1] check_equal(actual::Int64, expected::Int64, msg::String; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ ChainRulesTestUtils ~/JuliaEnvs/ChainRulesWorld/ChainRules.jl/dev/ChainRulesTestUtils/src/check_result.jl:25
 [2] check_equal
   @ ~/JuliaEnvs/ChainRulesWorld/ChainRules.jl/dev/ChainRulesTestUtils/src/check_result.jl:25 [inlined]
 [3] check_equal(actual::Tangent{Foo, Tuple{Int64, Int64}}, expected::Tangent{Foo, Tuple{Int64, Int64}}, msg::String; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ ChainRulesTestUtils ~/JuliaEnvs/ChainRulesWorld/ChainRules.jl/dev/ChainRulesTestUtils/src/check_result.jl:92
 [4] check_equal (repeats 2 times)
   @ ~/JuliaEnvs/ChainRulesWorld/ChainRules.jl/dev/ChainRulesTestUtils/src/check_result.jl:86 [inlined]
 [5] macro expansion
   @ REPL[189]:2 [inlined]
 [6] macro expansion
   @ /usr/local/src/julia/julia-1.6/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1151 [inlined]
 [7] top-level scope
   @ REPL[189]:2
Test Summary:        | Pass  Fail  Total
rrule for foo at bar |    1     1      2

vs before:

julia> @testset "rrule for foo at bar" begin
           check_equal(Tangent{Foo}(1, 2), Tangent{Foo}(1, 3))
       end
Foo.2: Test Failed at /Users/oxinabox/.julia/packages/ChainRulesTestUtils/0XyYu/src/check_result.jl:19
  Expression: isapprox(actual, expected; kwargs...)
   Evaluated: isapprox(2, 3)
Stacktrace:
 [1] check_equal(actual::Int64, expected::Int64; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ ChainRulesTestUtils ~/.julia/packages/ChainRulesTestUtils/0XyYu/src/check_result.jl:19
 [2] check_equal
   @ ~/.julia/packages/ChainRulesTestUtils/0XyYu/src/check_result.jl:19 [inlined]
 [3] macro expansion
   @ ~/.julia/packages/ChainRulesTestUtils/0XyYu/src/check_result.jl:84 [inlined]
 [4] macro expansion
   @ /usr/local/src/julia/julia-1.6/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1226 [inlined]
 [5] check_equal(actual::Tangent{Foo, Tuple{Int64, Int64}}, expected::Tangent{Foo, Tuple{Int64, Int64}}; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ ChainRulesTestUtils ~/.julia/packages/ChainRulesTestUtils/0XyYu/src/check_result.jl:83
Test Summary:        | Pass  Fail  Total
rrule for foo at bar |    1     1      2
  Foo.1              |    1            1
  Foo.2              |          1      1
ERROR: Some tests did not pass: 1 passed, 1 failed, 0 errored, 0 broken.

which doesn't look like much and i guess probably wasn't a great example (someone should find and try some others)

But it is nice that the error now at the top is about what is going on
now: rrule for foo at bar: Test Failed at /Users/oxinabox/JuliaEnvs/ChainRulesWorld/ChainRules.jl/dev/ChainRulesTestUtils/src/check_result.jl:25
before: Foo.2: Test Failed at /Users/oxinabox/.julia/packages/ChainRulesTestUtils/0XyYu/src/check_result.jl:19

In general having the testset breakdown sounds nice.
But it isn't actually useful
What you really want is the error message:

  Expression: isapprox(actual, expected; kwargs...)
  Problem:  Foo.2
   Evaluated: isapprox(2, 3)
  • the line in the file that is within test/... which triggered it (so you can see exactly how it was configured).
    Everything else is kinda noise.
    but that info is no where near the testset summary at the end.

It also tweaks the content of the testset title that was set in #154 so that it also prints just primal type even if you passed in PrimalAndTangent. (this avoids printingthat long ChainrulesCore.PrimalAndTangent{Float64, Float64} type).

Copy link
Member

@mzgubic mzgubic left a comment

Choose a reason for hiding this comment

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

Needs a version bump.

src/check_result.jl Outdated Show resolved Hide resolved
src/check_result.jl Outdated Show resolved Hide resolved
src/check_result.jl Outdated Show resolved Hide resolved
src/output_control.jl Show resolved Hide resolved
### helpers for printing in log messages etc
_string_typeof(x) = string(typeof(x))
_string_typeof(xs::Tuple) = join(_string_typeof.(xs), ",")
_string_typeof(x::PrimalAndTangent) = _string_typeof(primal(x)) # only show primal
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_string_typeof(x::PrimalAndTangent) = _string_typeof(primal(x)) # only show primal
_string_typeof(x::PrimalAndTangent) = "$(_string_typeof(primal(x)))$(_string_typeof(tangent(x)))"

Are we ever interested in the tangent type? Probably not?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah i considered that. Note that that would display things like Vector{Float64} ⊢ NoTangent() which is a bit of a play on words since the ⊢ operator doesn't actually accept types.

I think we can leave it as is (displaying just primals) for now, and consider adding that later.

Test.test_expr!("@test_msg msg", ex, kws...)

result = Test.get_test_result(ex, __source__)
return :(Test.do_test($result, $ExprAndMsg($(string(ex)), $(esc(msg)))))
Copy link
Member

Choose a reason for hiding this comment

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

Is using undocumented functions a code smell of some sort? I think it indicates that either:

  1. Test package should be easier to extent/modify
    or
  2. We are not achieving our goal in the right way with the tools available

I am leaning towards 1) from other experience with Test. On the other hand, I don't fully understand this PR (namely the test_msg macro). What do get_test_result and do_test actually do?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK it is 1) I will add a comment. I don't really understand what they do either, just that this is the thing that is needed.
It is kinda copied directly from the Test stdlib.
This whole thing with ExprAndMsg is a huge hack.

Copy link
Member Author

Choose a reason for hiding this comment

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

comment added

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, fair enough. LGTM just needs a version bump

Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>
@oxinabox oxinabox merged commit 9502d6a into master Jun 1, 2021
oxinabox added a commit that referenced this pull request Jul 21, 2021
make rand_tangent of struct with no perturbable fields return DoesNotExist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to improve test report/logs for when tests fails?
2 participants