lf-lang / lingua-franca

Intuitive concurrent programming in any language
https://www.lf-lang.org
Other
231 stars 62 forks source link

TypeScript specific Target Properties #73

Closed MattEWeber closed 4 years ago

MattEWeber commented 4 years ago

I'd like to establish some target properties specific to TypeScript. I was thinking of starting with "timeout". The timeout property is handled in the C target by being given as a command line argument as part of the "run" target property. Currently the TypeScript target doesn't accept command line arguments (maybe it should?), so a specific "timeout" property seems like the way to go. Right now the LF grammar requires target properties to be literals. I'd like to extend this to include times as well. Is there a pressing reason why target properties must only be literals?

edwardalee commented 4 years ago

We were thinking that the C target should also have a timeout property, replacing the run command option. It would specify a default timeout, which can be overridden on the command line. Allowing time values seems like a good idea...

Edward


Edward A Lee Professor UC Berkeley

On Dec 7, 2019, at 7:18 AM, mew2ub notifications@github.com wrote:

I'd like to establish some target properties specific to TypeScript. I was thinking of starting with "timeout". The timeout property is handled in the C target by being given as a command line argument as part of the "run" target property. Currently the TypeScript target doesn't accept command line arguments (maybe it should?), so a specific "timeout" property seems like the way to go. Right now the LF grammar requires target properties to be literals. I'd like to extend this to include times as well. Is there a pressing reason why target properties must only be literals?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/icyphy/lingua-franca/issues/73?email_source=notifications&email_token=ACA6ONXMWOR2PDGW2D7EQTDQXLMULA5CNFSM4JXDKJP2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4H6Y2LZA, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACA6ONSIFWCOEEC445GUCNDQXLMULANCNFSM4JXDKJPQ .

lhstrh commented 4 years ago

Whatever it is you take as a command line argument you'll have to parse at runtime anyway, so I see no reason for storing anything more specific than strings as target properties.

The timeout property would serve as a default that can be overridden by a command line argument; the target property can thus be handled the same way as the corresponding command line argument.

MattEWeber commented 4 years ago

I just checked in a change to the grammar.

Here are two reasons I can think of to store non-strings (which was already allowed in the grammar for integers before I added Times) as target properties:

  1. It may be desirable to use a target property in a code generator to directly modify the generated code without directly using the property as a command line argument.
  2. When programming a reactor in LF it's ugly to translate properties into strings when they aren't actually string types. For example, I vastly prefer a target declaration that looks like

target TypeScript( timeout = 10 sec );

to

target TypeScript( timeout = "10 sec" );

because a timeout is expressing a time. It also means we can validate the reactor's use of a time type in the validator and not in each code generator.

lhstrh commented 4 years ago

My point was that if you have run time code (i.e., a function) that is able to parse command line arguments, you can use that same function to parse properties and set defaults. We do the same thing in the C run time. As such, I would prefer to have defaults and command line arguments be written in the exact same format; as string literals that can be used interchangeably.

There is something to be said for using the validator, but then again, validation of command line arguments has to be done at run time as well, so the same argument as above applies.

Anyway, if we keep this, then please make the following change to the grammar:

Property:
    name=ID '=' (literal=Literal | (time=INT unit=TimeUnit));

This way we don't have to refer to stuff using .value.value, which looks ugly...

Also, your change broke the build. Probably because the C and Cpp compilers weren't updated. If you are unsure as to whether a change will break the build, best practice is to work in your own branch and submit a pull request when the feature is ready to be merged into main. Pull requests are also run by Travis, so if the request will cause failures, this will be apparent.

MattEWeber commented 4 years ago

I think what you're saying makes sense regarding a standardized format for properties, but it doesn't seem incompatible with writing times as time types in LF. A code generator, which knows about all the properties for a target, can easily convert a given (value, timeunit) pair into a string when it knows to expect a time property.

Are you of the opinion that every property should also be a command line argument? Because some of your comments sound like you're implying that.

I like your suggested grammar change a lot so I checked that in. Also that's a nice trick regarding pull requests and Travis. I'll try to use that in the future.

lhstrh commented 4 years ago

On Mon, Dec 9, 2019 at 6:03 PM mew2ub notifications@github.com wrote:

I think what you're saying makes sense regarding a standardized format for properties, but it doesn't seem incompatible with writing times as time types in LF.

So how would you deal with a command line argument that specifies a timeout then? Not possible?

Note: The Cpp code generator only accepts integers without a time unit (interpreted as seconds) so the timeout property that you’ve introduced is incompatible with that. We’d have to talk to Christian to resolve that, but if I recall correctly, his consideration was to be compliant with GNU Coding Standards regarding CLIs.

A code generator, which knows about all the properties for a target, can

easily convert a given (value, timeunit) pair into a string when it knows to expect a time property.

Obviously, yes.

Are you of the opinion that every property should also be a command line

argument? Because some of your comments sound like you're implying that.

No, I’m not implying that.

I like your suggested grammar change a lot so I checked that in. Also

that's a nice trick regarding pull requests and Travis. I'll try to use that in the future.

Cool! --

-- Marten Lohstroh, MSc. | Ph.D. Candidate University of California | 545Q Cory Hall Berkeley, CA 94720 | +1 510 282 9135

edwardalee commented 4 years ago

Chiming in with an opinion: I agree with Matt that if a property has a time value, it should be allowed to specify it without quotation marks. I don’t think this conflicts with Cpp, except that for Cpp, the timeout should be able to be specified as a number without quotation marks.

Perhaps the grammar should allow any string at all, with or without quotation marks, that does not include a comma or closing brace?

Edward


Edward A. Lee EECS, UC Berkeley eal@eecs.berkeley.edu http://eecs.berkeley.edu/~eal

On Dec 9, 2019, at 6:03 PM, mew2ub notifications@github.com wrote:

 I think what you're saying makes sense regarding a standardized format for properties, but it doesn't seem incompatible with writing times as time types in LF. A code generator, which knows about all the properties for a target, can easily convert a given (value, timeunit) pair into a string when it knows to expect a time property.

Are you of the opinion that every property should also be a command line argument? Because some of your comments sound like you're implying that.

I like your suggested grammar change a lot so I checked that in. Also that's a nice trick regarding pull requests and Travis. I'll try to use that in the future.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

lhstrh commented 4 years ago

I originally argued for simplicity, because we had something that worked and was general (i.e., a list of properties). We have now added the ability to check the values of which their key is known and associated with a particular type. To be clear: I'm fine with that, I just didn't think it was necessary.

I don't think it is a good idea to have such known keys with types that are different from one target to another. Target properties with the same key should have the same type. Now, if I have to specify a timeout with a time unit in the code, it would make sense to do the same on the command line. But the Cpp runtime does not allow this, and I think Christian will argue it if it does, generated programs should not accept an unquoted time value. Currently, it accepts an int that denotes a number of seconds.

That said, do we really need anything else than seconds to specify a timeout? If seconds suffice, I would argue the value of the timeout property should really just be an int.

edwardalee commented 4 years ago

All of this is making me think that maybe we should stick with just specifying the default command-line options as a target property, as currently done for the C target, and not have a timeout property at all…

Edward

—————— Edward A. Lee Professor of the Graduate School EECS, UC Berkeley http://eecs.berkeley.edu/~eal

On Dec 10, 2019, at 8:59 AM, Marten Lohstroh notifications@github.com wrote:

I originally argued for simplicity, because we had something that worked and was general (i.e., a list of properties). We have now added the ability to check the values of which their key is known and associated with a particular type. To be clear: I'm fine with that, I just didn't think it was necessary.

I don't think it is a good idea to have such known keys with types that are different from one target to another. Target properties with the same key should have the same type. Now, if I have to specify a timeout with a time unit in the code, it would make sense to do the same on the command line. But the Cpp runtime does not allow this, and I think Christian will argue it if it does, generated programs should not accept an unquoted time value. Currently, it accepts an int that denotes a number of seconds.

That said, do we really need anything else than seconds to specify a timeout? If seconds suffice, I would argue the value of the timeout property should really just be an int.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

MattEWeber commented 4 years ago

Marten, I think you're making a compelling case that if a property is representing a command line argument it should be a string. If we come to the consensus that a timeout should always be a command line argument, which I'm fine with, then I agree with Edward that we should do away with a separate timeout property altogether. Currently the TypeScript target doesn't have any command line options, which I why I looked to a timeout property in the first place, but command line options seem like a good feature to have.

In summary, with respect to the grammar I think default command line options should be specified as one property as a string. Non-command line option properties should be allowed to have time types if they represent a time value.

What's the current state of command line options for other targets and how feasible is it to standardize across all of them?

cmnrd commented 4 years ago

I think that the command line arguments and the properties should be seen as separate things. I think it would be great to have a set of standardized properties such as timeout across targets and in my view it only makes sense to use the time type there. This would also be perfectly fine for the Cpp generator, since it can translate the timeout to an integer the same way as it does with all other time values.

It is completely independent of this timeout property, if and how a target allows to overwrite this tiemout on the command line. For instance, the Cpp target could still expect seconds, while the C target is capable of parsing units and another target might have no CLI arguments at all.

To summarize: In my opinion the CLI arguments should be a method to customize the application on execution and the target properties should be a method for customizing an application on compile time. Therefore, CLI arguments should be strings (since CLI only knows strings) and LF properties should be of any type that makes sense for the particular property.

edwardalee commented 4 years ago

I agree with this 100%. I suggest we go with this as a matter of policy.

lhstrh commented 4 years ago

Sounds good to me.

One a different but related note: I think it would also be a good idea to standardize the CLI arguments across targets. If we're OK with that, I would prefer to follow the convention of the Cpp target.

edwardalee commented 4 years ago

OK, though I'm a little worried that limiting the CLI timeout to seconds will come back and bite us. I don't really understand the reason for this.

lhstrh commented 4 years ago

This is just following the POSIX standard guidelines for CLI args. How about if we want to add a unit, we simply add another switch e.g.: ./MyApp -timeout 1 -unit msec

edwardalee commented 4 years ago

Where can I find a specification of this standard?

lhstrh commented 4 years ago

I found: https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html

But is does not look like it forbids whitespace in arguments.

However, man getopt reads:

Traditional implementations of getopt(1) are unable to cope with white‐
space and other (shell-specific) special characters  in  arguments  and
non-option parameters.

It was Christian who broad this up earlier, so he might be able to elaborate more.

cmnrd commented 4 years ago

I am not sure whether this 'standard' is written down somewhere. It seems to be more a convention that most unix programs follow. In the C++ target I use a small external library (CLI11) for parsing command line arguments. This library is limited to arguments following the 'standard'. Arguments that are not prefixed by an option (-n or --name) are considered positional arguments. They are always seen distinct from any options.

I think that having a standardized sets of CLI arguments makes sense and I am strongly in favor of following the gnu convention as it allows to use existing libraries for CLI parsing in the target languages. This, however, does not mean that we are limited to timeouts being given always in seconds. We can easily do something like this: --timeout 5ms or --timeout "5 ms". The only reason why this is currently not supported by the C++ target is that I did not put in the effort for writing a small parser that converts these strings to an integer.

BTW: Is there a reason for not using SI time units in LF?

edwardalee commented 4 years ago

I like the idea of just removing the space between the number and the time unit. However, then shouldn’t we also also remove it in LF? According to Wikipedia, the space is required in SI units.

On SI units of time: We could easily get carried away here. Note that minutes, hours, days, and weeks are not SI units (they are not multiples of 10 of the base unit). On the other hand, ks is (a kiloseceond). So is ds (decisecond). According to the following page, seconds can be abbreviated “s” or “sec”; I chose the latter, but we could certainly switch to the former or support both. I personally find “usec” more readable than “us”.

http://www.exactlywhatistime.com/measurement-of-time/units-of-measurement/

The use of plurals is not usual with SI units. Should we eliminate those? If we use “s” for seconds, then plurals would be very odd: “ss”, “uss”.

cmnrd commented 4 years ago

I like the idea of just removing the space between the number and the time unit. However, then shouldn’t we also also remove it in LF? According to Wikipedia, the space is required in SI units.

I would make the space optional. For the string parser it shouldn't make a big difference whether there is a space or not. Then the user can decide whether she wants to write 5msec or "5 msec".

I asked about the units mostly out of curioisity. I don't mind the way it works now, but I think having additional support for other common units such as SI would be beneficial.

schoeberl commented 4 years ago

I find when not using a space the reading is odd. +1 for removing the plural form. I also think it is better to have less variations to say the same thing.

Martin

On 12 Dec, 2019, at 16:30, Christian Menard notifications@github.com wrote:

I like the idea of just removing the space between the number and the time unit. However, then shouldn’t we also also remove it in LF? According to Wikipedia, the space is required in SI units.

I would make the space optional. For the string parser it shouldn't make a big difference whether there is a space or not. Then the user can decide whether she wants to write 5msec or "5 msec".

I asked about the units mostly out of curioisity. I don't mind the way it works now, but I think having additional support for other common units such as SI would be beneficial.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/icyphy/lingua-franca/issues/73?email_source=notifications&email_token=AAE63GDGARX42G7IFNWL27LQYJKILA5CNFSM4JXDKJP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGXBAZY#issuecomment-565055591, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE63GCDVCDK76EXBNBB4G3QYJKILANCNFSM4JXDKJPQ.

lhstrh commented 4 years ago

Moving to SI seems to harm readability, which I'm not in favor of. Also, leaving out the plural form yields weird-looking statements like 3 week or 2 sec.

I see no reason to leave out spaces in the language. As Christian suggests, for CLI args we could accept "2 secs" as well as 2secs. I like this solution.

edwardalee commented 4 years ago

Or we could parse command-line arguments that have multiple words... It's really not that hard to parse these.

cmnrd commented 4 years ago

It is not hard if you write the parser yourself, but in all the CLI parser libraries that I know it is not possible to have multiple word arguments. If we want to allow --timeout 4 sec, this would mean that for most (if not all targets) we have to write a complete CLI parser instead of using the very convinient parser libraries that are available in many languages. I believe that this is a very high cost.

lhstrh commented 4 years ago

Especially for achieving something that many would assume is illegal syntax anyway...

schoeberl commented 4 years ago

Right, I’ve not yet seen a language where an integer literal is “attached” to a keyword. Spaces are used between tokens in (most?) languages.

Martin

On 13 Dec, 2019, at 19:26, Marten Lohstroh notifications@github.com wrote:

Especially for achieving something that many would assume is illegal syntax anyway...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/icyphy/lingua-franca/issues/73?email_source=notifications&email_token=AAE63GE7XJTZE7TCQ5IBILLQYPHWTA5CNFSM4JXDKJP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG22U7A#issuecomment-565553788, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE63GGACHPTAJS4MRXA5QDQYPHWTANCNFSM4JXDKJPQ.

edwardalee commented 4 years ago

I have drafted a proposal here: https://github.com/icyphy/lingua-franca/wiki/Targets,-Hosts,-and-Distributed-Execution

For timeout on the command line, can we use quotation marks, like this:

bin/Foo —timeout “100 msec”
lhstrh commented 4 years ago

This discussion seems resolved. Target properties are expressed in YAML format.

In order to allow single line comments that start with a #, which are used in YAML, the SL_COMMENT terminal has been overridden. Python-style comments are now allowed throughout LF programs.

Note that adding support for new (target-specific) properties now requires declaring these in Targets enum so that the validator doesn't flag them with a warning.