Using local_nested_autodiff for all instances of nested autodiff #1706
Conversation
|
I am not sure if I would put the nested thing in the most inner loops. This creates many additional start_nested calls which we did avoid before. What is the rationale for that? |
I definitely didn't want to change the location of any nested autodiff and aimed to put the constructor at exactly the line the |
|
Argh... I misread. Matching the old order of calls is the right thing to do. Sorry for the noise. My bad. |
|
@wds15 --- can you put in the formal review? It sounds like you did the audit for location, which is the most important part. I'm going to drop some comments here rather than in a review. I'm OK with the name as is. If it's not too much trouble, I'd suggest somethihg a bit more descriptive like Can the usage just be this instead of I'm not sure how unassigned classes are handled for scoping---they might be destroyed right away since it's not going to be used again. Doesn't the latter give you unused variable warnings? The RAII class itself is very elegantly coded and documented. It's making me so happy that devs are figuring out these kinds of patterns that make the code both easier to use and more robust. The only thing I would've required in a review is doc for the constructor and destructor, because that's where the action is. |
|
Ok, I can dive into this. |
|
I checked everything other than that the new calls are in the right place. If you don't have time, I can go through and do that, too.
|
|
So there is one thing which does concern me and I think we can solve it in a nice way. The problem is with threads. The issue is that the instance of the local nested thing can get created in thread A, but destructed in thread B... and then we are in no-mans land! However, there is a clean way to handle this. We can define that instantiating the local nested thing in thread A will always tear down the nested context in thread A - no matter which thread is running the destructor. Does that make sense to everyone? I am happy to code this up (either directly on this branch or on an extra branch based on this one). I think we should include this in this PR to make it thread safe. |
|
@wds15 I lack deep knowledge of threads, so I'd defer to you on this. But I am honestly intrigued - how do you imagine this being created and destroyed on different threads - can a scope begin on a thread and end on another thread? Note that I purposefully deleted the copy constructor, assignment operator and the
In all examples of using RAII, I've seen people declare the vars, so I would stick to this even if it is not strictly necessary (e.g. https://en.cppreference.com/w/cpp/language/raii). Though I think temporaries go out of scope sooner (https://rules.sonarsource.com/cpp/RSPEC-5184). Also I like having
It does not - the compiler detects the constructor has side effects and realizes you might want to have the variable unused to handle RAII. Regarding the name - it is easy to change it. I slightly don't like having |
|
Example: // in global scope:
local_nested* some_nested_ptr = 0;
// somewhere in the program start thread A
// thread A does this in its own running context:
some_nested_ptr = new local_nested();
// another thread B then does
delete some_nested_ptr;And there you have the issue. I am happy to code up something up which will avoid this. Essentially you have to store upon construction of the object a pointer to the actual AD tape which we want to refer to - and store it in the instance. That would be my suggestion. |
|
@wds15 The code you proposed will not compile, because I marked the |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
On Feb 14, 2020, at 6:02 AM, Martin Modrák ***@***.***> wrote:
@wds15 I lack deep knowledge of threads,
Very few people do. Nobody on our team does.
so I'd defer to you on this. But I am honestly intrigued - how do you imagine this being created and destroyed on different threads -
You're right that this can't happen because RAII is controlled by scope and scope is a per-thread thing.
can a scope begin on a thread and end on another thread? Note that I purposefully deleted the copy constructor, assignment operator and the new operator to avoid weirdness, but I certainly won't be completely shocked if that is not enough.
That should be enough to avoid unintended shenanigans. But it's really more a programming idiom thing we have to introduce. Given that it's the wild west of C underneath, you can do things like take a reference using & then pass it around as a pointer. So you could maliciously try to run the destructor in a different chain if you could somehow get ahold of a reference.
@bob-carpenter
I'm not sure how unassigned classes are handled for scoping---they might be destroyed right away since it's not going to be used again. Doesn't the latter give you unused variable warnings?
In all examples of using RAII, I've seen people declare the vars, so I would stick to this even if it is not strictly necessary
OK. Good to know.
(e.g. https://en.cppreference.com/w/cpp/language/raii). Though I think temporaries go out of scope sooner (https://rules.sonarsource.com/cpp/RSPEC-5184). Also I like having nested.set_zero_all_adjoints(); but that's a minor point.
That's a relatively expensive operation that we shouldn't be doing if we don't have to.
Doesn't the latter give you unused variable warnings?
It does not - the compiler detects the constructor has side effects and realizes this might be the desired behavior.
Cool! The C++ compilers never cease to amaze me.
Regarding the name - it is easy to change it. I slightly don't light having begin in the name - once again most RAII code I've seen doesn't do this and the classes are just called lock, not begin_lock -
Great. I'm 100% behind following standard idioms for naming.
also there is no end_nested_autodiff to match which might be confusing. I like having the explicit rev, so I would currently vote for nested_rev_autodiff or local_nested_rev_autodiff (to emphasize it is scope dependent). No strong opinions on this and will happily comply with your view on this.
I agree---the "local" bit is implied by scope anyway and I prefer shorter to longer identifiers.
|
|
Will the following compile?
THREAD 0:
local_nested* ptr; // not thread local
THREAD 1:
local_nested a;
ptr = &a;
THREAD 0:
ptr -> ~local_nested();
Even if it does, I'm not convinced it's something we need to try to programatically guard against as it would pretty much have to be done maliciously as I can't imagine anyone doing this kind of thing on purpose even by accident.
|
|
Name changed to
I guess that it will, but I agree that we shouldn't worry about this - we want to prevent inadvertent errors. Anyone wanting to do more complex nesting logic would use the global functions directly anyway. |
|
That's also my take on the situation. There's only so far we need to go in defense vs. other developers.
… On Feb 14, 2020, at 11:34 AM, Martin Modrák ***@***.***> wrote:
Name changed to nested_rev_autodiff.
Will the following compile?
I guess that it will, but I agree that we shouldn't worry about this - we want to prevent inadvertent errors. Anyone wanting to do more complex nesting logic would use the global functions directly anyway.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
…i_nested_autodiff
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
…i_nested_autodiff
…gs/RELEASE_500/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@wds15 just pinging this, I'd be glad to get this merged, it is starting to see conflicts with develop. But if there is anything you have still doubts about, I'll be happy to work through it. |
|
@bob-carpenter can u please finish the review? Thanks. If you want me to look at something specific, I can do that. |
|
I reviewed all the scopes and the class and submitted the review. The "change request" was just for an answer to my question of why one of the function's arguments changed when this was just about scoping memory. Was there an answer somewhere? |
|
The answer is here #1706 (comment) |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
Thanks @bob-carpenter |
Summary
Implemented a RAII-style class
local_nested_autodiff(are we happy with the name?) that makes it easier to ensure that nested stack is correctly recovered on leaving of the scope. The class also has aset_zero_all_adjointsmember, so that one can handle everything around nested autodiff via this class and without using global function calls.Removed all uses of
start_nested,recover_memory_nestedandset_zero_all_adjoints_nestedand uselocal_nested_autodiffinstead. The only exception are some of the tests of the autodiff where those functions are tested separately.The class itself is just a thin wrapper around the original calls.
Tests
A new test was written that replicates the tests for nested autodiff using global functions with using
local_nested_autodiff.I also moved the
gradablestruct used inrev/core/var_test.cppto its own header so that I can reuse it.Side Effects
Hopefully no. However I am worried that some of the functions I touched appear to have no tests.
Checklist
Math issue Use RAII for nested autodiff #1703
Copyright holder: Institute of Microbiology of the Czech Academy of Sciences
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested