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

[RFC][Relay] Text Format Part 2 #3016

Closed
joshpoll opened this issue Apr 12, 2019 · 15 comments
Closed

[RFC][Relay] Text Format Part 2 #3016

joshpoll opened this issue Apr 12, 2019 · 15 comments

Comments

@joshpoll
Copy link
Contributor

joshpoll commented Apr 12, 2019

The following RFC is for more bikeshedding about syntax of the Relay text format not covered by #1782.

The guiding principles for syntax choices are

  • readability: Syntax should be easy to read and parse visually.
  • internal consistency: Syntax choices should be similar to each other (e.g. function definitions and function types should have similar syntax).
  • external consistency: We should strive to reuse existing syntax choices from other languages so users that are familiar with those languages have less to learn. Also existing syntax choices are usually well-thought-out.

Please provide feedback on the syntax choices below or other syntactic features you think should be discussed that weren't already agreed on in #1782.

References

RefCreate

Existing languages:
OCaml

ref 5

ReasonML

ref(5)

Relay Proposal

ref(5)

RefWrite

OCaml

r := v

ReasonML

r := v

Relay Proposal

r := v

RefRead

OCaml

!r

ReasonML (adopted because ! is already used for not)

r^

It may be temping to use &x, but this implies that any expression has a reference, whereas in ML-like languages references must be created explicitly.
Relay Proposal

!r

Prefixes seem better than postfixes, but we need to avoid ! do to ambiguity. I'm open to other suggestions.

ADTs

ReasonML

type myVariant =
  | HasNothing
  | HasSingleInt(int)
  | HasSingleTuple((int, int))
  | HasMultipleInts(int, int)
  | HasMultipleTuples((int, int), (int, int))

Rust

enum myVariant {
  HasNothing,
  HasSingleInt(int),
  HasSingleTuple((int, int)),
  HasMultipleInts(int, int),
  HasMultipleTuples((int, int), (int, int)),
}

Relay Proposal

type myVariant =
  | HasNothing
  | HasSingleInt(int)
  | HasSingleTuple((int, int))
  | HasMultipleInts(int, int)
  | HasMultipleTuples((int, int), (int, int))

Pattern Matching

ReasonML

switch (x) {
  | HasNothing => 0
  | HasSingleInt(x) => 0
  | HasSingleTuple((x, y)) => 0
  | HasMultipleInts(x, y) => 0
  | HasMultipleTuples((x, y), (q, r)) => 0
}

Rust

match x {
  HasNothing => 0,
  HasSingleInt(x) => 0,
  HasSingleTuple((x, y)) => 0,
  HasMultipleInts(x, y) => 0,
  HasMultipleTuples((x, y), (q, r)) => 0,
}

Relay Proposal

match (x) {
  | HasNothing => 0
  | HasSingleInt(x) => 0
  | HasSingleTuple((x, y)) => 0
  | HasMultipleInts(x, y) => 0
  | HasMultipleTuples((x, y), (q, r)) => 0
}

case is also a viable alternative keyword.

Generics (i.e. type arguments)

Rust

enum Option<T> {
    Some(T),
    None,
}

fn takes_anything<T>(x: T) {
    // do something with x
}
let x: Option<i32> = Some(5);
let x: Option<i64> = Some(5);

ReasonML

type Option('a) =
    | Some('a)
    | None

Scala

Some[A](a : A)

Python

Option[A] = Union[A, None]

Relay Proposal

type Option[A] =
  | Some(A)
  | None

fn takes_anything[T](%x: T) {
    // do something with %x
}

let x = Some[i32](5);
let x = Some[i64](5);

TupleGetItem

ReasonML

("foo",2,true).0 // "foo"

Rust

("foo",2,true).0 // "foo"

Scala

("foo",2,true)._1 // "foo"

Python

("foo",2,true)[0] # "foo"

Relay Proposal

("foo",2,true).0 // "foo"

Semicolons

We need to consider whether or not to use semicolons as separators in sequence operations. The pros of using semicolons is they're easier to implement and allow us to ignore newline characters when writing and parsing programs. The cons are they are more cumbersome and make the text format less python-like.

Attributes

Python

def foo(a, b, c=None, d=None):

Relay Proposal

@foo(%a, %b, %c=None, %d=None)

when the attrs type index matches the operator's attrs type index

@foo(%a, %b, BarAttrs={%c: None, %d: None})

when it doesn't.
I'm not as certain about this syntax piece, so I'm very open to suggestions here.

cc @tqchen @MarisaKirisame @jroesch @wweic @vinx13 @junrushao1994 @nhynes

@MarisaKirisame
Copy link
Contributor

No No No No No on r^.
I'll still say !r is the best - not doesnt appear much in deep learning, and should not take !.

@wweic
Copy link
Contributor

wweic commented Apr 13, 2019

RefRead and RefRead are swapped in the spec.

For @foo(%a, %b, BarAttrs={%c: None, %d: None}), does it mean Relay supports record? Or Relay core recognizes the record like syntax and maintains the record internally, source code can not access the record?

@MarisaKirisame
Copy link
Contributor

MarisaKirisame commented Apr 13, 2019

@wweic
The Attribute System achieved a record syntax.
We see the point of record though, we just do not have enough man power and it is not high priority. If you need them, the easiest way is to probably use ADT instead (fields can be function implemented by pattern matching). Or if you really need them, can you tell me the use case?

@tqchen
Copy link
Member

tqchen commented Apr 13, 2019

  • Semi-colons: we should not have them, to make most syntax consistent with python
  • attrs when it does not match attr's type index
    • This needs more deliberation, it needs to be consistent with the case when we don't print out BarAttrs(in the meta-data). Or print BarAttrs directly(although not allowed atm), My recommendation simply use attrs=ValueFormat, where ValueFormat can be any printed format of the attrs

@wweic
Copy link
Contributor

wweic commented Apr 13, 2019

@MarisaKirisame Thanks for clarification. Just curious about the implementation details. I don't have a use case right now.

@joshpoll
Copy link
Contributor Author

@wweic thanks I fixed the typo

@joshpoll
Copy link
Contributor Author

@tqchen could you provide examples of the attrs=ValueFormat suggestion?

@joshpoll
Copy link
Contributor Author

joshpoll commented May 7, 2019

This PR has been open for a while now. Please post your final thoughts so we can close this and begin implementing the language features. In particular, we should try to come to some resolution on attributes.

@tqchen @MarisaKirisame @jroesch @wweic @vinx13 @junrushao1994 @nhynes

@MarisaKirisame
Copy link
Contributor

My single objection is no ^.

@wweic
Copy link
Contributor

wweic commented May 8, 2019

I'm good with the RFC overall. Slightly prefer ! same as @MarisaKirisame.

@tqchen
Copy link
Member

tqchen commented Jun 13, 2019

Please conclude and summarize the RFC so we could start a vote about the text format

@joshpoll
Copy link
Contributor Author

Here is a summary of the proposed text format changes.

RefCreate: ref(n)
RefWrite: r := v
RefRead: !r
ADTs:

type myVariant =
  | HasNothing
  | HasSingleInt(int)
  | HasSingleTuple((int, int))
  | HasMultipleInts(int, int)
  | HasMultipleTuples((int, int), (int, int))

Pattern Matching:

match (x) {
  | HasNothing => 0
  | HasSingleInt(x) => 0
  | HasSingleTuple((x, y)) => 0
  | HasMultipleInts(x, y) => 0
  | HasMultipleTuples((x, y), (q, r)) => 0
}

Generics

type Option[A] =
  | Some(A)
  | None

fn takes_anything[T](%x: T) {
    // do something with %x
}

let x = Some[i32](5);
let x = Some[i64](5);

TupleGetItem: ("foo", 2, true).0 // "foo"
Semicolons: optional unless collapsing multiple lines into one as in Python
Attrs: We still don't have a satisfying answer for this so we may punt it. We will default to Tianqi's suggestion of attrs=ValueFormat where we allow different options for ValueFormat.

Please vote on these changes. This will allow us to nearly solidify the text format.
cc @tqchen @MarisaKirisame @jroesch @wweic @vinx13 @junrushao1994 @nhynes

@MarisaKirisame
Copy link
Contributor

@joshpoll what about type inference? How will it work if the type parameter is inferred? e.g. Some(5)?

@wweic
Copy link
Contributor

wweic commented Sep 17, 2019

We might also need to support Any #3042.

@tqchen
Copy link
Member

tqchen commented Oct 8, 2019

This RFC isa bit out-dated, I would recommend close it, and open new checklist for the next todos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants