Annotations as Options Replacement, Immutable Option Parsing

[seldridge]

Overview

Annotations and command line options should be fungible. A user calling the FIRRTL compiler should be able to interact with the compiler in the same way that an earlier frontend that produces annotations, but does not interact with a Driver (and vice versa). This does the following high level refactors:

• The old mutable scopt parsing is replaced with immutable parsing
• All command line options are parsed into an AnnotationSeq

The latter change enables immutable option parsing to be split across disjoint traits while using the same parser (e.g., HasFirrtlOptions, HasChiselExecutionOptions both parse using a shared OptionParser[AnnotationSeq]). The drivers or transforms that consume command line options are then expected to extract what they need from the command-line created AnnotationSeq, deleting what is specific to them. This then stages for a logical refactor of Driver subclassing Transform.

API changes

• Removes methods for mutating options (e.g., setTopName)
• A warning (as opposed to an error) results if the implicit annotation file is not found
• All custom transforms must be globally visible (no anonymous transforms)
• All custom transforms will be constructed (specific transform instances cannot be run)
• An ExecutionOptionsManager can only be constructed using (String, Array[String])
• Case classes for driver options (e.g., FirrtlOptions) use Option[T] to store optional data, as opposed to empty strings
• CommonOptions no longer exists and has been rolled into FirrtlExecutionOptions

API Deprecations

• FirrtlExecutionOptions.{getInputFileName, getOutputConfig, getTargetFile} (moved to HasFirrtlOptions)

API additions

• Annotation files may include annotation files via InputAnnotationFileAnnotation with automatic include guard checking
• Adds Driver.execute(args: String)
• HasFirrtlOptions.{getInputFileName, getOutputConfig, getTargetFile}

Miscellaneous changes

• Checks that no long or short options conflict with each other (with tests)
• Bump scopt from 3.6.0 to 3.7.0 as we need some new methods to extract information to run the checks in the preceding bullet
• Factors out HasFirrtlOptions from ExecutionOptionsManager.scala into FirrtlExecutionOptions.scala
• Options vals (commonOptions, firrtlOptions) are necessarily lazily evaluated
• Lots of documentation

Issues

• Fixes [RFC] Annotations subsume ExecutionOptions? #764
• Fixes Funny Behavior when topName isn't specified #485
• Fixes Command-line option class-log-level should be allowed more than once #683
• Fixes Approach for Providing Parameters to Custom FIRRTL Passes #552
• Fixes Add Support for Custom Command-Line Options #558

Todo

• Align with Chisel3
  • Remove [skip chisel tests] (I'm not sure this can be merged without this...)
• Tests
  • Multiple Annotations/Transforms try to claim the same option
  • Check that we catch a recursive annotation file import
• Read annotation files directly during option parsing as opposed to delaying this until the Driver
• Documentation
• Cleanup
• [ ] All FIRRTL options should be deleted after use (I'm opting to leave this in for now---it's unclear to me if other transforms need these)
• Integrate @chick toy-options registration
• Remove usage of ExecutionOptionsManager with X with Y with Z as a parameter
• [ ] Seal Annotations that should not be emitted by Transforms, e.g., RunFirrtlTransform
  • I don't think that we can reasonably assert what a user will want to do here. Also, there are complications with RunFirrtlTransformAnnotation as Chisel's Driver uses this. Similar logic applies for "Driver-only" annotations, like TopNameAnnotation. A transform could reasonably change this. There's a lot of power here, but I think the better approach would be to roll this into adding checks for weirdness at the form levels.

[jackkoenig]

We should also think about possible command-line option collisions that could occur if two custom transforms (likely in different projects), try to claim the same option. I'm sure scopt errors when this occurs, but I wonder if there's some way we can prevent this from happening. Would it be possible to enforce that custom transforms have a prefix on their option or something? This might be really hard or onerous to do, but maybe it's possible!

[seldridge]

Good thought, @jackkoenig. While I think that this will be unlikely for some time, it's a huge issue...

Surprisingly, scopt does not error for this... All that appears to be happening is the last callback is the one that wins. This, unfortunately, appears to happen entirely silently...

I think that the only way to utterly disambiguate things is to use the full class path. That seems reasonable to me, even if it's verbose. It's just a question of whether this should be enforced and how it should be enforced.

[seldridge]

Scala 2.12.4 seems to be bumping up against Travis memory limits. The closest that I've gotten to replicating is that I can locally run myself out of metaspace if I repeatedly run tests in the same sbt session. Not sure if this is what is going on or something else. The problems with 2.12.4 clearly seem to start with the ClassFind commits in the patch set.

[jackkoenig]

Did you remove the class-finder stuff due to 2.12.4 issues with Travis memory limits?

[seldridge]

@jackkoenig: No, not motivated by that (should have clarified in the commit). After discussing this last week, the consensus was that this was not the right approach for the following two reasons:

• It's way too slow generally if you have to look through all classes. Theoretically, this should work and be fast to find options in arbitrary libraries pulled in from a Maven repo. However, ClassFinder churning through every possible class searching for any transform that may provide options is abhorrently slow. I was skirting around this by only looking inside FIRRTL.
• There's got to be a better way than to inspect everything. Reflection can very quickly find anything that mixes in a sealed trait via knownDirectSubclasses, but that's not suitable here either as the traits can be anywhere.

Summarily, everyone thought that there should exist a cleaner solution and that this PR should be separated from that. I'm working to get this more towards only dealing with refactoring the options manager to provide:

1. Immutability of option parsing
2. Representation of all options as annotations (i.e., the parser is an OptionParser[AnnotationSeq])

[jackkoenig]

@seldridge Ah, sorry I missed the meeting--had a big paper push going on.

I'm totally down with automatically finding such transforms to a separate PR since this it is a fairly separate piece of work. It also relates to potentially improving annotation deserialization and some way of automatically instantiating transforms based on the presence of certain annotations (ie. moving that functionality from Chisel to FIRRTL).

Just for my own understanding, how slow is it? I wanted to use ClassFinder for finding classes that extend Annotation, but if it's really that slow then I guess it won't work. As you alluded to above, maybe it will be fast enough if we can be judicious in which packages we search (wg. explicitly exclude sbt, scala, and specific plugins)...

[seldridge]

It's actually not that bad, but it was an annoying shock from not using it.

It takes about 2 seconds to do the class path search with ClassFinder. That is searching everything locally (including ~/.ivy2). It speeds up a little once things are cached (e.g., when running a test suite).

Now that I think about it, this may not be all that bad from a usability perspective. It's basically the same as import tensorflow. This is assumedly linear with respect to the number of classes, however... Larger projects would get a larger hit.

[jackkoenig]

I see. Well we can take a look at a later time! This PR looks to be shaping up well!

[jackkoenig]

println or should this be logger.info? logger.warn?

[seldridge]

I think this is safe to leave in as it is. I could wrap this in a logger, but this is really information that I want to get to the command line for debugging before a logger may exist and I want this to happen even if a logger doesn't exist or I have the incorrect log level.

I.e., I think I can wrap the scopt parse call with a logger, but I'd want to pass in an explicit log level which would seem to violate the motivation for having a logger at all.

[ucbjrl]

This seems to be responsible for:

An exception or error caused a run to abort: scopt.OptionDef.shortOpt()Lscala/Option; 
java.lang.NoSuchMethodError: scopt.OptionDef.shortOpt()Lscala/Option;
	at firrtl.options.DuplicateHandling$$anonfun$5.apply(OptionParser.scala:22)
	at firrtl.options.DuplicateHandling$$anonfun$5.apply(OptionParser.scala:22)
	at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
	at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
	at scala.collection.immutable.List.foreach(List.scala:392)
	at scala.collection.generic.TraversableForwarder$class.foreach(TraversableForwarder.scala:35)
	at scala.collection.mutable.ListBuffer.foreach(ListBuffer.scala:45)
	at scala.collection.TraversableLike$class.map(TraversableLike.scala:234)
	at scala.collection.AbstractTraversable.map(Traversable.scala:104)
	at firrtl.options.DuplicateHandling$class.parse(OptionParser.scala:22)
	at firrtl.options.ExecutionOptionsManager$$anon$1.parse(ExecutionOptionsManager.scala:20)
	at firrtl.options.ExecutionOptionsManager.parse(ExecutionOptionsManager.scala:68)
	at firrtl_interpreter.Driver$.execute(Driver.scala:33)
	at firrtl_interpreter.DriverSpec$$anonfun$1$$anonfun$apply$mcV$sp$1.apply$mcV$sp(DriverSpec.scala:21)
	at firrtl_interpreter.DriverSpec$$anonfun$1$$anonfun$apply$mcV$sp$1.apply(DriverSpec.scala:10)
	at firrtl_interpreter.DriverSpec$$anonfun$1$$anonfun$apply$mcV$sp$1.apply(DriverSpec.scala:10)

[ucbjrl]

False alarm. This was due to IntelliJ using an old version of scopt for the firrtl-interpreter build.

[seldridge]

Possibly a stale scopt version? That method, shortOpt, showed up in 3.7.0.

[seldridge]

@chick: Here's what I came up with for using Annotations for internal tool data movement of things which we can't serialize with json4s (currently). The idea here is that you mix this into an Annotation and before this is serialized to JSON, it will be converted to an annotation that is serializable. This is damn ugly as I wasn't able to think of a way to do this with type checking. Maybe there's a way with proper variances, but nothing was jumping out at me. This should enable something (which I'll add to Chisel) of a ChiselCircuitGeneratorAnnotation(dut: () => RawModule) that will serialize to a ChiselCircuitAnnotation(circuit: Circuit) but can be used for Chisel testers.

@jackkoenig: This is to address a narrow problem of passing annotations into the Driver of Chisel testers. It needs a () => MultiIOModule to pass to a PeekPokeTester and this is information that can't be contained in a serializable annotation without completely rewriting how Chisel testers works. Can you think of any way to add type safety here?

[seldridge]

Ping @jackkoenig, @chick: I have a separate branch with this PR rebased and refactored massively. I could PR this as either sequential commits or about 9 PRs (possibly more...). Note that only one of those 9 PRs would have an actual functional impact on FIRRTL (the one that migrates to the "options are annotations" pattern). Everything else is either small patches or adding the framework to enable "options are annotations".

I haven't tested that every commit builds, but they are supposed to (or that's the intention). I'll spoon feed them to Travis to get those ✔️s.

Would it be a good idea to close this and reopen? Any thoughts on separate PRs or one PR with a chain of sequential, reasoned patches?

See: master...seldridge:issue-764-refactor

FWIW, if anyone else is trying to do this type of rebasing, instead of fighting with rebasing a dumpster 🔥 of 77 commits with endless merge conflicts, I had luck with applying the equivalent patch onto a new branch and then repeatedly rebasing to break out logical items.

[seldridge]

This makes sense to me. I'm opting for the series of PRs approach. I'll re-open if people feel differently.

[jackkoenig]

I think the series of PRs sounds great, thanks!