Simplify stan::lang::compile by throwing exceptions on errors
47 views
Skip to first unread message
Allen B. Riddell
Dec 18, 2015, 7:21:57 AM12/18/15
to stan...@googlegroups.com
Hi,
It would be great for interface writers if errors were standardized. If
we're going to throw exceptions when some "invalid" data is passed to a
function, we should always try to throw exceptions.
One case where something in-between is happening is stan::lang::compile
and I wonder if it couldn't be simplified by just throwing exceptions
when there is a parse error or an error in generating the cpp code.
For example, it seems like we're going to throw errors in the var_context
refactor; here's a bit of Michael's code which throws an exception
when someone passes an invalid `name` to a function:
> std::vector<size_t> dims = dims_r(name);
> if (dims.size() != 1) {
> std::stringstream msg;
> msg << "Requested the vector " << name
> << ", but " << name
> << " is not defined as a vector in variable context"
> << std::endl;
> throw std::runtime_error(msg.str());
> }
Could we simplify ``stan::lang::compile`` by throwing
std::runtime_error`s? Here's the current signature
bool compile(std::ostream* msgs,
std::istream& stan_lang_in,
std::ostream& cpp_out,
const std::string& model_name,
const std::string& in_file_name = "input") {
Could we do something like this instead, returning ``cpp_out``.
std::string compile(std::string stan_lang_in, const std::string& model_name)
and rely on exceptions to communicate parsing and generator errors?
Packing several parsing errors into an exception doesn't seem so bad to
me -- I think we have lengthy exception error messages elsewhere. If it
is a bad idea then could we standardize what we should do in this kind
of case? Like create a simple struct which encapsulates the possible
results and which could be used in other cases? Here's what compile
could return instead, which would be closer to the existing setup:
struct StanStringResult {
int error; // 0 or 1
std::string error_msg;
std::string result;
} StanResult;
(The current signature for stan::lang::compile also looks a little like
a cmdstand-ism lurking in Stan).
If there's interest in this change, I'm willing to work on it as part of
the overall refactor effort.
Best,
Allen
It would be great for interface writers if errors were standardized. If
we're going to throw exceptions when some "invalid" data is passed to a
function, we should always try to throw exceptions.
One case where something in-between is happening is stan::lang::compile
and I wonder if it couldn't be simplified by just throwing exceptions
when there is a parse error or an error in generating the cpp code.
For example, it seems like we're going to throw errors in the var_context
refactor; here's a bit of Michael's code which throws an exception
when someone passes an invalid `name` to a function:
> std::vector<size_t> dims = dims_r(name);
> if (dims.size() != 1) {
> std::stringstream msg;
> msg << "Requested the vector " << name
> << ", but " << name
> << " is not defined as a vector in variable context"
> << std::endl;
> throw std::runtime_error(msg.str());
> }
Could we simplify ``stan::lang::compile`` by throwing
std::runtime_error`s? Here's the current signature
bool compile(std::ostream* msgs,
std::istream& stan_lang_in,
std::ostream& cpp_out,
const std::string& model_name,
const std::string& in_file_name = "input") {
Could we do something like this instead, returning ``cpp_out``.
std::string compile(std::string stan_lang_in, const std::string& model_name)
and rely on exceptions to communicate parsing and generator errors?
Packing several parsing errors into an exception doesn't seem so bad to
me -- I think we have lengthy exception error messages elsewhere. If it
is a bad idea then could we standardize what we should do in this kind
of case? Like create a simple struct which encapsulates the possible
results and which could be used in other cases? Here's what compile
could return instead, which would be closer to the existing setup:
struct StanStringResult {
int error; // 0 or 1
std::string error_msg;
std::string result;
} StanResult;
(The current signature for stan::lang::compile also looks a little like
a cmdstand-ism lurking in Stan).
If there's interest in this change, I'm willing to work on it as part of
the overall refactor effort.
Best,
Allen
Krzysztof Sakrejda
Dec 18, 2015, 9:18:05 AM12/18/15
to stan development mailing list, a...@ariddell.org
On Friday, December 18, 2015 at 7:21:57 AM UTC-5, Allen B. Riddell wrote:
> Hi,
>
> It would be great for interface writers if errors were standardized. If
> we're going to throw exceptions when some "invalid" data is passed to a
> function, we should always try to throw exceptions.
> Hi,
>
> It would be great for interface writers if errors were standardized. If
> we're going to throw exceptions when some "invalid" data is passed to a
> function, we should always try to throw exceptions.
The other use for the 'msgs' parameter is to also return warnings (pretty sure about that) so throwing exceptions won't map exactly onto current behavior. I like the idea of throwing well-defined exceptions for errors and passing back a struct as a result since it would make interface code clearer. Or if not throwing exceptions for compile errors then at least making some easily machine-readable part to the messages (i.e.-CODE:MESSAGE or something like that).
Allen B. Riddell
Dec 18, 2015, 9:53:10 AM12/18/15
to stan...@googlegroups.com
On 12/18, Krzysztof Sakrejda wrote:
> On Friday, December 18, 2015 at 7:21:57 AM UTC-5, Allen B. Riddell wrote:
> > Hi,
> >
> > It would be great for interface writers if errors were standardized. If
> > we're going to throw exceptions when some "invalid" data is passed to a
> > function, we should always try to throw exceptions.
>
> The other use for the 'msgs' parameter is to also return warnings (pretty sure about that) so throwing exceptions won't map exactly onto current behavior. I like the idea of throwing well-defined exceptions for errors and passing back a struct as a result since it would make interface code clearer. Or if not throwing exceptions for compile errors then at least making some easily machine-readable part to the messages (i.e.-CODE:MESSAGE or something like that).
Is this true? (We're certainly an missing a test in pystan in that case)
> On Friday, December 18, 2015 at 7:21:57 AM UTC-5, Allen B. Riddell wrote:
> > Hi,
> >
> > It would be great for interface writers if errors were standardized. If
> > we're going to throw exceptions when some "invalid" data is passed to a
> > function, we should always try to throw exceptions.
>
> The other use for the 'msgs' parameter is to also return warnings (pretty sure about that) so throwing exceptions won't map exactly onto current behavior. I like the idea of throwing well-defined exceptions for errors and passing back a struct as a result since it would make interface code clearer. Or if not throwing exceptions for compile errors then at least making some easily machine-readable part to the messages (i.e.-CODE:MESSAGE or something like that).
That certainly would be hard to handle with exceptions. Perhaps what
stan::lang::compile could return in that case would be something like:
struct StanStringResult {
int errors; // 0 or 1
vector<std::string> errors_msgs;
int warnings; // 0 or 1
vector<std::string> warning_msgs;
std::string result;
}
Best,
Allen
Krzysztof Sakrejda
Dec 18, 2015, 10:18:03 AM12/18/15
to stan development mailing list
Now I'm not sure, I thought the warnings about lhs transformations and Jacobians came up during stan::lang::compile but maybe not. I'm still trying to sort out this stuff so sorry if I confused things. Krzysztof
Krzysztof Sakrejda
Dec 18, 2015, 10:45:41 AM12/18/15
to stan development mailing list, a...@ariddell.org
On Friday, December 18, 2015 at 9:53:10 AM UTC-5, Allen B. Riddell wrote:
> On 12/18, Krzysztof Sakrejda wrote:
> > On Friday, December 18, 2015 at 7:21:57 AM UTC-5, Allen B. Riddell wrote:
> > > Hi,
> > >
> > > It would be great for interface writers if errors were standardized. If
> > > we're going to throw exceptions when some "invalid" data is passed to a
> > > function, we should always try to throw exceptions.
> >
> > The other use for the 'msgs' parameter is to also return warnings (pretty sure about that) so throwing exceptions won't map exactly onto current behavior. I like the idea of throwing well-defined exceptions for errors and passing back a struct as a result since it would make interface code clearer. Or if not throwing exceptions for compile errors then at least making some easily machine-readable part to the messages (i.e.-CODE:MESSAGE or something like that).
>
> Is this true? (We're certainly an missing a test in pystan in that case)
> On 12/18, Krzysztof Sakrejda wrote:
> > On Friday, December 18, 2015 at 7:21:57 AM UTC-5, Allen B. Riddell wrote:
> > > Hi,
> > >
> > > It would be great for interface writers if errors were standardized. If
> > > we're going to throw exceptions when some "invalid" data is passed to a
> > > function, we should always try to throw exceptions.
> >
> > The other use for the 'msgs' parameter is to also return warnings (pretty sure about that) so throwing exceptions won't map exactly onto current behavior. I like the idea of throwing well-defined exceptions for errors and passing back a struct as a result since it would make interface code clearer. Or if not throwing exceptions for compile errors then at least making some easily machine-readable part to the messages (i.e.-CODE:MESSAGE or something like that).
>
> Is this true? (We're certainly an missing a test in pystan in that case)
I looked at the stanc code again and I don't know how the warnings are dealt with but I was assuming something that wasn't in the code when I wrote my initial message. Sorry! Krzysztof
Allen B. Riddell
Dec 18, 2015, 2:16:21 PM12/18/15
to stan...@googlegroups.com
The warnings are put into a stream; they aren't returned explicitly.
The 'DIAGNOSTIC(S) FROM PARSER' are written in stan/lang/parser.hpp.
On 12/18, Krzysztof Sakrejda wrote:
> You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
The 'DIAGNOSTIC(S) FROM PARSER' are written in stan/lang/parser.hpp.
On 12/18, Krzysztof Sakrejda wrote:
> Now I'm not sure, I thought the warnings about lhs transformations and Jacobians came up during stan::lang::compile but maybe not. I'm still trying to sort out this stuff so sorry if I confused things. Krzysztof
>
> --
>
> You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
Bob Carpenter
Dec 18, 2015, 3:58:59 PM12/18/15
to stan...@googlegroups.com
> On Dec 18, 2015, at 9:53 AM, Allen B. Riddell <a...@ariddell.org> wrote:
>
> On 12/18, Krzysztof Sakrejda wrote:
>> On Friday, December 18, 2015 at 7:21:57 AM UTC-5, Allen B. Riddell wrote:
>>> Hi,
>>>
>>> It would be great for interface writers if errors were standardized. If
>>> we're going to throw exceptions when some "invalid" data is passed to a
>>> function, we should always try to throw exceptions.
>>
>> The other use for the 'msgs' parameter is to also return warnings (pretty sure about that) so throwing exceptions won't map exactly onto current behavior. I like the idea of throwing well-defined exceptions for errors and passing back a struct as a result since it would make interface code clearer. Or if not throwing exceptions for compile errors then at least making some easily machine-readable part to the messages (i.e.-CODE:MESSAGE or something like that).
>
> Is this true? (We're certainly an missing a test in pystan in that case)
> That certainly would be hard to handle with exceptions. Perhaps what
> stan::lang::compile could return in that case would be something like:
>
> struct StanStringResult {
> int errors; // 0 or 1
> vector<std::string> errors_msgs;
> int warnings; // 0 or 1
> vector<std::string> warning_msgs;
> std::string result;
> }
>
Plus, if you have boolean values it should be "bool",
not "int".
- Bob
Bob Carpenter
Dec 18, 2015, 4:04:29 PM12/18/15
to stan...@googlegroups.com
What's happening under the hood is that the stream's
getting plumbed through the parser so that each semantic
action on a rule (for instance, the sampling statement
rule) can write to the stream.
The way to make this more structured isn't to continue
to write to the stream and add a flag, it's to return
a structured set of warnings.
I'm still not clear on why you want to change the behavior
with stanc. Who's going to catch these exceptions and what
are they going to do with them? That's always the place to
start with exception design.
Right now, there's two ways to fail. You can either get
an exception in the code when there's a failed expectation
(such as an opening brace without the expected close brace)
and a parse failure when the program's ill formed for other
reasons. This is just the behavior inherited from the Spirit
Qi parser, but we could always take a negative return and
throw an exception.
The compile method in stan::lang::compile() is the top level call.
You can see that it gets a boolean return from the underlying
Spirit Qi parser, and it just returns it if it's false. Otherwise,
it generates the C++, which can't fail and returns true.
If you then look at stan::lang::parse(), which is called by the
compiler, you can see the try/catch blocks where the errors
are trapped and written rather than rethrown. The reason
for that is that there's nothing higher-level to catch them.
Would you rather that functionality moved up to the calling
program, which could catch an exception?
- Bob
getting plumbed through the parser so that each semantic
action on a rule (for instance, the sampling statement
rule) can write to the stream.
The way to make this more structured isn't to continue
to write to the stream and add a flag, it's to return
a structured set of warnings.
I'm still not clear on why you want to change the behavior
with stanc. Who's going to catch these exceptions and what
are they going to do with them? That's always the place to
start with exception design.
Right now, there's two ways to fail. You can either get
an exception in the code when there's a failed expectation
(such as an opening brace without the expected close brace)
and a parse failure when the program's ill formed for other
reasons. This is just the behavior inherited from the Spirit
Qi parser, but we could always take a negative return and
throw an exception.
The compile method in stan::lang::compile() is the top level call.
You can see that it gets a boolean return from the underlying
Spirit Qi parser, and it just returns it if it's false. Otherwise,
it generates the C++, which can't fail and returns true.
If you then look at stan::lang::parse(), which is called by the
compiler, you can see the try/catch blocks where the errors
are trapped and written rather than rethrown. The reason
for that is that there's nothing higher-level to catch them.
Would you rather that functionality moved up to the calling
program, which could catch an exception?
- Bob
Allen B. Riddell
Dec 18, 2015, 5:37:25 PM12/18/15
to stan...@googlegroups.com
On 12/18, Bob Carpenter wrote:
> What's happening under the hood is that the stream's
> getting plumbed through the parser so that each semantic
> action on a rule (for instance, the sampling statement
> rule) can write to the stream.
>
> The way to make this more structured isn't to continue
> to write to the stream and add a flag, it's to return
> a structured set of warnings.
>
That sounds good to me. Just streaming the error messages (in an
> What's happening under the hood is that the stream's
> getting plumbed through the parser so that each semantic
> action on a rule (for instance, the sampling statement
> rule) can write to the stream.
>
> The way to make this more structured isn't to continue
> to write to the stream and add a flag, it's to return
> a structured set of warnings.
>
unstructured form) doesn't seem optimal.
> I'm still not clear on why you want to change the behavior
> with stanc. Who's going to catch these exceptions and what
> are they going to do with them? That's always the place to
> start with exception design.
>
user.
> Right now, there's two ways to fail. You can either get
> an exception in the code when there's a failed expectation
> (such as an opening brace without the expected close brace)
> and a parse failure when the program's ill formed for other
> reasons. This is just the behavior inherited from the Spirit
> Qi parser, but we could always take a negative return and
> throw an exception.
>
> The compile method in stan::lang::compile() is the top level call.
> You can see that it gets a boolean return from the underlying
> Spirit Qi parser, and it just returns it if it's false. Otherwise,
> it generates the C++, which can't fail and returns true.
>
> If you then look at stan::lang::parse(), which is called by the
> compiler, you can see the try/catch blocks where the errors
> are trapped and written rather than rethrown. The reason
> for that is that there's nothing higher-level to catch them.
>
> Would you rather that functionality moved up to the calling
> program, which could catch an exception?
catch the exception since that's what we seem to be doing elsewhere.
In general, I think it would be good if stan::lang::compile could accept
a simpler set of params and return a tidy object. This seems like it
would be in the spirit of the refactor and it would make wrapping
stan::lang::compile quite a bit easier.
Are you happy with the stan::lang::compile interface as it is?
Bob Carpenter
Dec 19, 2015, 11:07:00 PM12/19/15
to stan...@googlegroups.com
What we have now is fine for CmdStan, but I can't speak for
the other interfaces. I'm sure CmdStan would be easy to
modify for whatever we decide.
The Stan 3 command refactor has the interfaces pass in
callbacks for I/O. This allows consistent behavior
across interfaces, which is my main goal along with removing
code duplication.
I don't like the idea of having the interfaces catch the
exceptions and deal with them as it will neither reduce
code duplication nor encourage consistent behavior across interfaces.
Right now, we have a callback structure for the interfaces,
but the callback is a simple output stream. It would be a
lot of work, but straightforward to change the parser API to pass
in a structured callback handler for warnings, errors, etc.
So the question is what should it do?
For example, how much logger-like behavior do you want?
Loggers often configure messages to have log levels, like
fatal error, recoverable error, warning, debug (very verbose mode), etc.
Here's the spec for log4j:
https://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/Level.html
Then there's the issue of formatting the output --- do you include
thread IDs, time stamps, etc. And then there's the issue of how a user
or programmatic client configures the logger and where it pipes output.
- Bob
P.S. I don't like big heterogeneous data structures for returns.
For example, I don't like the return for R's built-in lm() function,
because it's mutable in such a way that allows it to get into
inconsistent states (models mismatching data mismatching estimates).
There are ways to mitigate these problems, but I think the callback
route is much cleaner. And it allows streaming.
the other interfaces. I'm sure CmdStan would be easy to
modify for whatever we decide.
The Stan 3 command refactor has the interfaces pass in
callbacks for I/O. This allows consistent behavior
across interfaces, which is my main goal along with removing
code duplication.
I don't like the idea of having the interfaces catch the
exceptions and deal with them as it will neither reduce
code duplication nor encourage consistent behavior across interfaces.
Right now, we have a callback structure for the interfaces,
but the callback is a simple output stream. It would be a
lot of work, but straightforward to change the parser API to pass
in a structured callback handler for warnings, errors, etc.
So the question is what should it do?
For example, how much logger-like behavior do you want?
Loggers often configure messages to have log levels, like
fatal error, recoverable error, warning, debug (very verbose mode), etc.
Here's the spec for log4j:
https://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/Level.html
Then there's the issue of formatting the output --- do you include
thread IDs, time stamps, etc. And then there's the issue of how a user
or programmatic client configures the logger and where it pipes output.
- Bob
P.S. I don't like big heterogeneous data structures for returns.
For example, I don't like the return for R's built-in lm() function,
because it's mutable in such a way that allows it to get into
inconsistent states (models mismatching data mismatching estimates).
There are ways to mitigate these problems, but I think the callback
route is much cleaner. And it allows streaming.
Bob Carpenter
Dec 19, 2015, 11:32:42 PM12/19/15
to stan...@googlegroups.com
The Stan 3 command refactor has the interfaces pass in callbacks
for I/O. This allows consistent behavior
across interfaces, which is my goal.
for I/O. This allows consistent behavior
Your suggestion to throw exceptions and have the interfaces
deal with it will allow each interface to go its own way in
config and reporting, which is what we're trying to avoid (though
perhaps in vain).
Right now, we have that callback structure for the interfaces,
but the callback is a simple output stream. It would be possible
to change that API to have the interfaces pass in a structured
callback handler for warnings, errors, etc.
As far as being happy with what we currently have, it's fine
for CmdStan, but I can't speak for the other interfaces.
I'm sure it'll be easy for CmdStan to adapt to whatever changes
make sense for Python or R.
Refactoring the whole parsing interface to pass in a callback
handler for errors is a lot of work, but I think it would all be
straightforward. So the thing to do still is figure out what
the callback would be.
I don't think returning a big structure is the right
thing to do. It's a classic example of a "god object", like
the fit object in RStan, which I also don't like at all. It's
a despised anti-pattern in the OO community.
See, e.g., https://en.wikipedia.org/wiki/God_object
So I'd rather stick to callbacks for a presentation layer.
If we do go with something like callbacks, there's the issue of how
much logger-like behavior we want. Loggers often configure
messages to have log levels, like fatal error, recoverable error,
warning, debug (very verbose mode), etc.
Here's the spec for log4j:
https://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/Level.html
Then there's the issue of formatting the output --- do you include
thread IDs, time stamps, etc. And then there's the issue of how a user
or programmatic client configures the logger and where it pipes output.
- Bob
warning, debug (very verbose mode), etc.
Here's the spec for log4j:
https://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/Level.html
Then there's the issue of formatting the output --- do you include
thread IDs, time stamps, etc. And then there's the issue of how a user
or programmatic client configures the logger and where it pipes output.
Bob Carpenter
Dec 19, 2015, 11:33:53 PM12/19/15
to stan...@googlegroups.com
Sorry, that second message was an unedited dupe! The edited
version was the first message.
- Bob
version was the first message.
- Bob
Allen B. Riddell
Dec 20, 2015, 9:15:12 AM12/20/15
to stan...@googlegroups.com, Krzysztof Sakrejda
On 12/19, Bob Carpenter wrote:
> What we have now is fine for CmdStan, but I can't speak for
> the other interfaces. I'm sure CmdStan would be easy to
> modify for whatever we decide.
>
> The Stan 3 command refactor has the interfaces pass in
> callbacks for I/O. This allows consistent behavior
> across interfaces, which is my main goal along with removing
> code duplication.
>
> I don't like the idea of having the interfaces catch the
> exceptions and deal with them as it will neither reduce
> code duplication nor encourage consistent behavior across interfaces.
This sounds good to me. I'm willing to work having an interface to
> What we have now is fine for CmdStan, but I can't speak for
> the other interfaces. I'm sure CmdStan would be easy to
> modify for whatever we decide.
>
> The Stan 3 command refactor has the interfaces pass in
> callbacks for I/O. This allows consistent behavior
> across interfaces, which is my main goal along with removing
> code duplication.
>
> I don't like the idea of having the interfaces catch the
> exceptions and deal with them as it will neither reduce
> code duplication nor encourage consistent behavior across interfaces.
stan::lang::compile where the interfaces pass in callbacks. As you say,
this would make for consistent behavior which is excellent for everyone,
especially future interface developers/maintainers.
> Right now, we have a callback structure for the interfaces,
> but the callback is a simple output stream. It would be a
> lot of work, but straightforward to change the parser API to pass
> in a structured callback handler for warnings, errors, etc.
>
> So the question is what should it do?
>
> For example, how much logger-like behavior do you want?
> Loggers often configure messages to have log levels, like
> fatal error, recoverable error, warning, debug (very verbose mode), etc.
> Here's the spec for log4j:
>
> https://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/Level.html
>
> Then there's the issue of formatting the output --- do you include
> thread IDs, time stamps, etc. And then there's the issue of how a user
> or programmatic client configures the logger and where it pipes output.
behavior of stan::lang::compile, right? If so, so the callback would
just accept string messages. If there was demand for something more
sophisticated, we could elaborate the message format.
Reply all
Reply to author
Forward
0 new messages