Updates based on recent changes and discussion on RFC PR #262#5

penpornk · 2020-09-09T18:38:44Z

Changes to the Main RFC File

Added the Implementation Conventions and Usage Overview section.
Replaced the Stability / User Impact with the Versioning Strategy and Stability section. Also moved the section higher.
Updated the API design according to the real implementation.
Updated code examples.

Changes to the Versioning Strategy File:

Added/updated contents based on the discussion on the PluggableDevice RFC.

Referenced semver and TensorFlow's Version Compatibility page.
Added the Updating Guidelines, Convention, and Detecting Incompatibility sections.
Merged the current code examples with @yisitu's newer ones on PR #262 and simplified them a bit.

Specifically, addressed the following points in the comments on the PluggableDevice RFC:

Restructured the contents so the conventions/assumptions/procedures are clearer.
Showed that the struct size check works with struct members in any position, not just the last member of the struct.
Added more details on major version bump conventions. Specified that breaking changes require an RFC.
Explicitly mentioned that nontrivial deprecations (that cannot use zero initialization) needs a major version bump.
Added a requirement that fields that treat 0 and NULL as invalid values must have explicit comments saying so, to ensure all plug-ins have consistent behavior, e.g., none of them is using 0 or NULL for special cases.
Updated the deprecation examples to use 'producer' and 'consumer' keywords (instead of plug-in and core TensorFlow) to cover more cases.

* Added the Implementation Conventions and Usage Overview section. * Replaced the Stability / User Impact with the Versioning Strategy and Stability section. Also moved the section higher. * Updated the API design according to the real implementation. * Updated code examples.

@yisitu

…gableDevice RFC (PR tensorflow#262). * Referenced semver and TensorFlow's Version Compatibility page. * Added the Updating Guidelines, Convention, and Detecting Incompatibility sections. * Merged the current code examples with @yisitu's newer ones on PR tensorflow#262 and simplified them a bit.

penpornk · 2020-09-09T18:40:14Z

@yisitu Could you please help review as well? (I can't tag you as a reviewer.) Thank you very much!

rfcs/20200612-stream-executor-c-api/C_API_versioning_strategy.md

Also some more minor edits.

rfcs/20200612-stream-executor-c-api.md

Co-authored-by: Situ Yi <60493024+yisitu@users.noreply.github.com>

rfcs/20200612-stream-executor-c-api.md

rfcs/20200612-stream-executor-c-api/C_API_versioning_strategy.md

yisitu

I've done a round of review, please have a look. Thanks for consolidating the Github comments and polishing up the document Penporn!

penpornk

Thank you for the thorough review! :D

rfcs/20200612-stream-executor-c-api.md

rfcs/20200612-stream-executor-c-api/C_API_versioning_strategy.md

Co-authored-by: Situ Yi <60493024+yisitu@users.noreply.github.com>

Changed "shall" to "must" to make sure people would not misinterpret "shall".

yisitu

LGTM

penpornk and others added 4 commits September 3, 2020 10:33

Merge remote-tracking branch 'annarev/stream_executor_rfc'

2255854

Merge remote-tracking branch 'annarev/stream_executor_rfc'

b3360a8

penpornk requested a review from annarev September 9, 2020 18:39

annarev reviewed Sep 9, 2020

View reviewed changes

rfcs/20200612-stream-executor-c-api/C_API_versioning_strategy.md Outdated Show resolved Hide resolved

rfcs/20200612-stream-executor-c-api/C_API_versioning_strategy.md Outdated Show resolved Hide resolved

Fixes according to annarev@'s comments.

d1ec351

Also some more minor edits.

penpornk changed the title ~~Updated the RFC based on recent changes and discussion on RFC PR #262~~ Updates based on recent changes and discussion on RFC PR #262 Sep 9, 2020

annarev approved these changes Sep 9, 2020

View reviewed changes