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

Debugging messages about type mismatches #1031

Closed
wants to merge 9 commits into from

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Feb 8, 2020

This adds a few warning messages about conversions to Float64 or worse, which seem to be a common problem, today #1026, last week #815.

These are behind @debug, which I see is also used by NNlib for some such warnings. Another option would be to make these errors, controlled by some Flux.allowconvert(false) along the lines of what CuArrays does for scalar access, see earlier commit.

I'm not sure they will catch all the activation-function bugs mentioned in performance.md. Perhaps it would be better to attach the warning to every layer, not to Chain.

@DhairyaLGandhi
Copy link
Member

I'm a big fan of providing users with the option to get a better hold of performance and debugging that comes through so easily, thanks @mcabbott !!

Could you please do a quick rebase?

@mcabbott
Copy link
Member Author

mcabbott commented Mar 2, 2020

Sure. I guess a remaining question is whether these want to be a bigger warning, say @warn. And if so, whether there needs to be a mechanism for turning them off. An earlier commit had one.

@DhairyaLGandhi
Copy link
Member

From your comment on the other issue, you'd shown a significant speedup that could have been avoided if we could be type stable, which makes it critical.

Having tooling around being able to find these gotchas I think is a more stable and longer term useful situation, so as long as people can access it easily, without performance penalties to the actual runtime, I think is a fair deal, so a warn, though very tempting, might be unnecessary.

@DhairyaLGandhi
Copy link
Member

Could do with tests that demonstrate the cases this catches and ones it misses

@mcabbott
Copy link
Member Author

mcabbott commented Mar 17, 2020

Perhaps this is ready?

What it doesn't do is tell you which layer of a Chain messes up the forward pass. Nor will it catch at all if a particular layer messes up the backward pass, although that doesn't seem to be common.

Edit: what it also doesn't do is flag similar conversions of input type in Conv layers:

(a::Conv{<:Any,<:Any,W})(x::AbstractArray{<:Real}) where {T <: Union{Float32,Float64}, W <: AbstractArray{T}} =
a(T.(x))

Although you will still get the "Chain(...) receives input of eltype Float64 but creates output of eltype Float32" warning.

@DhairyaLGandhi
Copy link
Member

Pinging @MikeInnes for his thoughts on this

@racinmat
Copy link
Contributor

@MikeInnes pinging again as this would really help, e.g. with hunting down obscure bugs like #965

@mcabbott
Copy link
Member Author

mcabbott commented Oct 21, 2020

It'll have to be a fresh PR as apparently rebasing has disabled the "reopen" button. IMO this would still be worth doing, and should be a warning not an @debug message (i.e. printed by default). But it's unclear to me who is around and has merge privileges.

@racinmat
Copy link
Contributor

@CarloLucibello seemed active last days.
I would love to see it merged, it would have saved us so much time we spent hunting bugs 😅

@mcabbott
Copy link
Member Author

mcabbott commented Nov 2, 2020

https://discourse.julialang.org/t/flux-con-warning/49456 is today’s example of a nice friendly this-will-be-slow warning.

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.

3 participants