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

Proposal: drop/ignore return value rather than yielding it #15

Closed
garrison opened this issue Feb 20, 2018 · 24 comments
Closed

Proposal: drop/ignore return value rather than yielding it #15

garrison opened this issue Feb 20, 2018 · 24 comments

Comments

@garrison
Copy link
Contributor

Hi there

I'm very excited about ResumableFunctions.jl. It is a wonderful demonstration of the power of julia's macro system.

On the other hand, I find one design choice to be puzzling: the choice to yield the final/return value of a resumable function as an additional value. I propose that instead, all such values should be @yielded explicitly, and any such return value should be dropped.

This would have a number of advantages:

  • The comment and additional complexity regarding "default returns" in this example could be eliminated
  • It will be easier to design resumable functions that are type stable.
  • It will be possible for a resumable function to yield no values rather than always having to yield at least one
  • This would be consistent with the notion that "Explicit is better than implicit" (or at least says The Zen of Python). It is also consistent with the way python generators work.

See also issue #13, which may be related.

@garrison
Copy link
Contributor Author

Also related to #2.

@BenLauwens
Copy link
Collaborator

Hi

I am aware of this "feature";)

I use @resumable function as a way to abstract the finite-state machine when doing process driven event-driven simulation. The yielding of the final/return value is a safe-guard for the end-users of SimJulia who are not necessarily aware what is happening behind the @resumable macro. A process stops when the process function reaches a normal return. The yielding of an event, means that their is still something to do afterwards.

A more important problem is how your proposal can be implemented. When a value is yielded (implemented as a normal return) the @resumable function can not know that this is the final value. Only when a real return is encountered the @resumable function stops iterating. In C# the behaviour is identical and this comes from the way how it is implemented.

In Julia a function returns always something (nothing) so I can not create a @resumable function that yields no value. In Python a return without argument is allowed to stop the generator, but this is implemented at a language level allowing this behaviour...

So I am a little bit stuck, I understand your motivations for this modification but I see no way how to implement it.

If you have any ideas how to do this. Feel free to share it and I am willing to integrate it in the package.

Kind regards

Ben

@garrison
Copy link
Contributor Author

Hi Ben,

Thank you for the reply.

I have only scratched the surface so far, but let me mention a few things that come to mind. I think Python is able to deal with this issue in the way I expect because its iteration protocol relies on the StopIteration exception, which can be thrown upon iteration when there are no more values. It seems that the Revised iteration protocol Julep (which addresses JuliaLang/julia#8149) may actually solve the underlying issue here, which would then make this proposal possible.

@cortner
Copy link

cortner commented Feb 21, 2018

just to say I also just tried this package and probably have to give up on it because of this. Not being able to ignore the final return makes it much much more tedious to use. (otherwise I love it!) unfortunately I don't have an idea either how to do this.

@BenLauwens
Copy link
Collaborator

Please check out branch Python-generators. Can you confirm that this is the behaviour you really like?
If so I will update documentation and release v0.2;)
Kind regards

Ben

@BenLauwens
Copy link
Collaborator

Only on Julia v0.7 this is supported... I will back port if asked for

@BenLauwens
Copy link
Collaborator

I did a quick back-port to Julia v0.6, so check out branch Python-generators-v0.6

@cortner
Copy link

cortner commented Feb 23, 2018

Thank you, I’ll try to test this tonight.

@garrison
Copy link
Contributor Author

Thank you!

One difference between this and python's generators is that the values are always calculated one ahead of what is currently necessary. For instance,

@resumable function f(n)
    for i in 1:n
        println("yielding $i")
        @yield i
    end
end

for a in f(4)
    println(a)
end

yields

yielding 1
yielding 2
1
yielding 3
2
yielding 4
3
4

This may be a reasonable stopgap measure, though, until JuliaLang/julia#18823 is addressed.

I also noticed one issue when specifying the type of the return value, which I believe is a surmountable implemention issue:

@resumable function f(n)::Int
    for i in 1:n
        @yield i
    end
end

collect(f(4))
ERROR: MethodError: Cannot `convert` an object of type Nothing to an object of type Int64

@BenLauwens
Copy link
Collaborator

Hi

I found inspiration in the new Julia iteration protocol that behaves identically: values are always calculated one ahead.

When specifying the return type. The function has to return the correct type... because values are calculated one ahead. A simple solution can be:

@resumable function f(n)::Int
    for i in 1:n
        @yield i
    end
    0
end

collect(f(4))

I do some further testing and if no other problems are raised I will release somewhere next week an updated version.

@cortner
Copy link

cortner commented Feb 23, 2018

After checking out the new branch: (on v0.6)

julia> using ResumableFunctions
INFO: Precompiling module ResumableFunctions.
ERROR: LoadError: LoadError: UndefVarError: KeySet not defined
Stacktrace:
 [1] include_from_node1(::String) at ./loading.jl:576
 [2] include(::String) at ./sysimg.jl:14
 [3] include_from_node1(::String) at ./loading.jl:576
 [4] include(::String) at ./sysimg.jl:14
 [5] anonymous at ./<missing>:2
while loading /Users/ortner/.julia/v0.6/ResumableFunctions/src/transforms.jl, in expression starting on line 521
while loading /Users/ortner/.julia/v0.6/ResumableFunctions/src/ResumableFunctions.jl, in expression starting on line 14
ERROR: Failed to precompile ResumableFunctions to /Users/ortner/.julia/lib/v0.6/ResumableFunctions.ji.
Stacktrace:
 [1] compilecache(::String) at ./loading.jl:710
 [2] _require(::Symbol) at ./loading.jl:497
 [3] require(::Symbol) at ./loading.jl:405

@BenLauwens
Copy link
Collaborator

Checkout branch Python-generators-v0.6 and it will work on Julia v0.6

@cortner
Copy link

cortner commented Feb 23, 2018

fantastic - thanks. The other problem I had last time I tried your package was performance related. But I wasn't sure whether it was my fault or a limitation of this package. If I can produce a MWE then I'll just file another issue.

@garrison
Copy link
Contributor Author

garrison commented Feb 24, 2018

I found inspiration in the new Julia iteration protocol that behaves identically: values are always calculated one ahead.

I don't think this is correct. The new iteration protocol always calls step just before testing isnull. The expected output of my code would then be

yielding 1
1
yielding 2
2
yielding 3
3
yielding 4
4

under the new protocol.

@BenLauwens
Copy link
Collaborator

You're right. Now it should do things in the correct order (Julia v0.6) and also the issue when the return type is specified, is solved. Now it should do the same as the revised iteration protocol.
So please test;) and keep me informed of problems!
Thanks

Ben

@garrison
Copy link
Contributor Author

Wow that's awesome! :-) Thank you.

Here is a related idea/suggestion: perhaps the macro should error if there is a return statement that returns something. In other words, if a user says return val, we already know that val is going to be ignored, so the friendliest thing is to error. (A bare return statement, on the other hand, is fine.) This will help the user if they accidentally return instead of @yielding something. It's also possible to be more liberal about this later if a need arises, but enforcing this at a later date would mean breaking existing code once again, so I think if you like this idea it makes sense to require it now.

@BenLauwens
Copy link
Collaborator

I use the return val in SimJulia to pass a final value as a process terminates. So letting the macro error is not an option. What I can do is generating an error when a value different from nothing is returned when a @resumable function is used as an Iterator but this will be during runtime... What do you think?

@garrison
Copy link
Contributor Author

I use the return val in SimJulia to pass a final value as a process terminates.

Is there an example of this in action somewhere? Is the intended effect any different from @yield(val); return, which is arguably much more clear and explicit?

@garrison
Copy link
Contributor Author

(Actually, I can't find a single example of a file that contains both @yield and return statements in SimJulia master.)

@BenLauwens
Copy link
Collaborator

There was (is) a use case but as you have mentioned we can always do @yield val; return. I have to check with my researchers using ResumableFunctions if I can do this without breaking their code. I keep you informed (remind me in two weeks if you have had no response;)
Kind regards
Ben

@garrison
Copy link
Contributor Author

garrison commented Apr 2, 2018

Sounds good! Another possibility would be to have a deprecation warning instead of an error.

@garrison
Copy link
Contributor Author

Bump

@BenLauwens
Copy link
Collaborator

@garrison

I have implemented your request in branch Python-generators-v0.6
Please checkout. A warning will be generated. If OK I will do ASAP a release for Julia v0.6.

If I get no negative feedback, I will make it an error. This will be the default behaviour for Julia v0.7.
Kind regards

Ben

@garrison
Copy link
Contributor Author

As far as I can tell, this now behaves precisely how I would like it to. One (minor) remaining thing:

(val == :nothing || val == nothing) && return expr

I would suggest changing this line to

val == :nothing && return expr

This way, a return statement will work without warning, but an explicit return nothing will warn. Since it is not possible to return a value, I think the plain return statement is most appropriate style. (This is, of course, my humble opinion.)

Kind regards,
Jim

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

No branches or pull requests

3 participants