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

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

Merged
merged 8 commits into from
Sep 10, 2020

Conversation

penpornk
Copy link
Collaborator

@penpornk penpornk commented Sep 9, 2020

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.

penpornk and others added 4 commits September 3, 2020 10:33
* 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.
…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
Copy link
Collaborator Author

penpornk commented Sep 9, 2020

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

Also some more minor edits.
@penpornk 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
Copy link

@yisitu yisitu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@penpornk penpornk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the thorough review! :D

rfcs/20200612-stream-executor-c-api.md Outdated Show resolved Hide resolved
rfcs/20200612-stream-executor-c-api.md Outdated Show resolved Hide resolved
rfcs/20200612-stream-executor-c-api.md Show resolved Hide resolved
rfcs/20200612-stream-executor-c-api.md Outdated Show resolved Hide resolved
rfcs/20200612-stream-executor-c-api.md Show resolved Hide resolved
rfcs/20200612-stream-executor-c-api.md Outdated Show resolved Hide resolved
rfcs/20200612-stream-executor-c-api.md Show resolved Hide resolved
penpornk and others added 2 commits September 9, 2020 21:42
Changed "shall" to "must" to make sure people would not misinterpret "shall".
Copy link

@yisitu yisitu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@annarev annarev merged commit 2c53747 into annarev:stream_executor_rfc Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants