This repository has been archived by the owner on Aug 20, 2024. It is now read-only.
Let firrtl based applications run despite loading unknown annotations #2387
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
An application like barstools may contain a main that loads an annotations file containing annotation classes that are not on it's classpath. This change allows unknown annotations to be preserved by wrapping them in a UnrecognizedAnnotation. If annotations are then output to a file, they will be unwrapped during serialization This feature can be enabled via an AllowUnrecognizedAnnotations annotation
chick
force-pushed
the
allow-unrecognized-annotations
branch
from
d32b384
to
3cd4288
Compare
jackkoenig
suggested changes
Oct 28, 2021
src/test/scala/firrtlTests/annotationTests/UnrecognizedAnnotationSpec.scala
Outdated
Show resolved
Hide resolved
- Added `UnrecogizedAnnotationsException` for internal communications of errors during annotation deserialization - Made tracking of Unrecognized annotation types more robust, by extracting class with regex - fixed AllowUnrecognizedAnnotations to stop it from eating following annotation - change to use mutable package inline with ArrayBuffer - removed intermediate result values used in debugging JsonProtocol - added handler in `stage.view` annotation handler - Added new tests to show behavior when run from command line
- text has been simplified when unrecognized errors occur
- finally got the flag passing down into the deserialization right in - JsonProtocol - GetIncludes - Rearranged allowing unrecognized tests - Adding checking for annotation having made it through to the output annotations
mwachs5
reviewed
Nov 2, 2021
| @@ -165,3 +166,5 @@ object Annotation | |||
| case class DeletedAnnotation(xFormName: String, anno: Annotation) extends NoTargetAnnotation { | |||
| override def serialize: String = s"""DELETED by $xFormName\n${anno.serialize}""" | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
can this have scaladoc added?
There was a problem hiding this comment.
I could but shouldn't an unrelated tests like this go in a separate PR?
There was a problem hiding this comment.
@chick I think you may have responded to the wrong comment
There was a problem hiding this comment.
@jackkoenig @mwachs5 I meant to say, adding scaladoc to pre-existing code that is unrelated to this PR should go on a separate PR. But I'm happy to add some. Any thoughts on what it should say, I feel like DeletedAnnotations are heading towards deprecation and using them, particularly looking inside them, is an anti-pattern
jackkoenig
suggested changes
Nov 4, 2021
| @@ -564,7 +564,7 @@ class JsonAnnotationTests extends AnnotationTests { | |||
| val manager = setupManager(Some(anno)) | |||
|
|
|||
| the[Exception] thrownBy Driver.execute(manager) should matchPattern { | |||
| case InvalidAnnotationFileException(_, _: AnnotationClassNotFoundException) => | |||
| case InvalidAnnotationFileException(_, _: UnrecogizedAnnotationsException) => | |||
| } | |||
| } | |||
There was a problem hiding this comment.
I suspect, but am not certain, that this change will change the error you receive if you have an unserializable annotation (eg. an annotation that just wraps a random non-case class). Can you add a test here showing what happens in that case?
There was a problem hiding this comment.
Does the second to last test in AnnotationTests demonstrate this?
jackkoenig
reviewed
Nov 4, 2021
src/test/scala/firrtlTests/annotationTests/UnrecognizedAnnotationSpec.scala
Show resolved
Hide resolved
- Fixed up white space explosion - UnrecognizedAnnotationSerializerSerializer => UnrecognizedAnnotationSerializer - canonicalized getAnnotationNameFromMappingException - var => val - UnrecognizedAnnotationSpec - simplified sample annotation data, much smaller now
jackkoenig
suggested changes
Nov 11, 2021
- Made GetClassPattern private - Switched to flatMap to compute `val loaded = ...`
jackkoenig
approved these changes
Nov 12, 2021
jackkoenig
added
the
Please Merge
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An application like barstools may contain a main that loads an annotations file containing
annotation classes that are not on it's classpath. This change allows unknown annotations
to be preserved by wrapping them in a UnrecognizedAnnotation.
This feature can be enabled via an AllowUnrecognizedAnnotations
Addresses issue #2384
Contributor Checklist
Type of Improvement
New feature with a minor API addition
API Impact
This new feature is opt in. It adds an Annotation and a CLI flag. Allows Stage subClasses to load annotation files with unrecognized Annotations.
Error messages produced when unrecognized class occur and this feature is not enabled have changed slightly and show class names for all unrecognized annotations. These messages appear via Error level logging.
Backend Code Generation Impact
No change to backend code generation
Desired Merge Strategy
Squash: The PR will be squashed and merged (choose this if you have no preference.
Release Notes
It is possible to disable errors caused by JSON annotation files containing unrecognized annotations. This situation
can occur when, for example applications like barstools with no code level dependency on rocket chip encounters
a rocket chip annotations in an annotation file. This change carries the annotations forward while ignoring their content.
Reviewer Checklist (only modified by reviewer)
Please Merge?