-
Notifications
You must be signed in to change notification settings - Fork 223
TensorFlow Tools #5
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
ee73e88
Add support for Tensor NIO
karllessard b2dd45b
Quick fixes in the README file
karllessard b22a74f
Apply post-review changes and rename artifacts
karllessard 401c8a4
Move Shape to upper level
karllessard 0828ca6
Rename utils to tools
karllessard 8735909
Add raw buffer factories
karllessard 6c02c89
Rename data layouts and adapters
karllessard 4cd0760
Bound types to TType in generics
karllessard File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,4 +3,8 @@ op { | |
| endpoint { | ||
| name: "collective.BroadcastRecv" | ||
| } | ||
| out_arg: { | ||
| name: "data" | ||
| rename_to: "output" | ||
| } | ||
| } | ||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,4 +3,8 @@ op { | |
| endpoint { | ||
| name: "collective.BroadcastSend" | ||
| } | ||
| out_arg: { | ||
| name: "data" | ||
| rename_to: "output" | ||
| } | ||
| } | ||
7 changes: 7 additions & 0 deletions
7
tensorflow-core/tensorflow-core-api/src/bazel/api_def/api_def_CollectiveGather.pbtxt
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| op { | ||
| graph_op_name: "CollectiveGather" | ||
| out_arg: { | ||
| name: "data" | ||
| rename_to: "output" | ||
| } | ||
| } |
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,4 +3,8 @@ op { | |
| endpoint { | ||
| name: "collective.AllReduce" | ||
| } | ||
| out_arg: { | ||
| name: "data" | ||
| rename_to: "output" | ||
| } | ||
| } | ||
6 changes: 6 additions & 0 deletions
6
tensorflow-core/tensorflow-core-api/src/bazel/api_def/api_def_KafkaDataset.pbtxt
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| op { | ||
| graph_op_name: "KafkaDataset" | ||
| endpoint { | ||
| name: "data.KafkaDataset" | ||
| } | ||
| } |
4 changes: 4 additions & 0 deletions
4
tensorflow-core/tensorflow-core-api/src/bazel/api_def/api_def_NcclAllReduce.pbtxt
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,7 @@ | ||
| op { | ||
| graph_op_name: "NcclAllReduce" | ||
| out_arg: { | ||
| name: "data" | ||
| rename_to: "output" | ||
| } | ||
| } |
4 changes: 4 additions & 0 deletions
4
tensorflow-core/tensorflow-core-api/src/bazel/api_def/api_def_NcclReduce.pbtxt
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,7 @@ | ||
| op { | ||
| graph_op_name: "NcclReduce" | ||
| out_arg: { | ||
| name: "data" | ||
| rename_to: "output" | ||
| } | ||
| } |
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That feels strange. Why do we need this dependency here? Is it just because that we can't have a separate module until we move away from the code generator in C++?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tensorare now based onNdArrayfor mapping their memory and allowing direct access to it, so it must depend onnio-utils. I'm not sure to understand what is strange, you meant that memory mapping should occur outside the core API?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just the names that are confusing me. When I read something like
tensorflow-core, I think like this means the actual "core" without dependencies on other modules. Maybe it's just thattensorflow-nioshould really be named something else and not "TensorFlow"? That might also help with adoption from other projects. That's probably something we should continue on the mailing list you created about that and one of the first things to clear up with guys from MXNet, DL4J, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, that's why I renamed the artifact
tensorflow-nio-utilstonio-utils(though I'm not a huge fan of that name neither so if anyone comes up with a better one...). So thetensorflowyou've noticed there is just to mention our organization in the group and does not name the artifact itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ndarray-core? I thinknio-utilsis likely to make people think ofjava.nio.*and as the language moves away from those classes (and they are 15+ years old at this point), calling them "new" is something of a misnomer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we will group artifacts if there is a need but right now, I cannot think of any other utility library than this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a functional need to group things together. If it's just to put them in abstract categories, it will make it hard to decide which category each module belongs to, possibly without any actual benefits. Maven doesn't even put the parent name in artifact coordinates, so unless we do it explicitly like with
tensorflow-core->tensorflow-core-api, etc then the benefits are even more marginal.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, here is what I ended up to do: I preserved the
tensorflow-utilsname but now it contains the code of the library itself, includingDataBufferandNdArrayAPIs.So if in the future it happens that we have more small utility classes like these that we would like to share with the world and that are independent from TF runtime, they will end up in this library as well (kind of a Guava for ML).
There is a lot of presumptions and questions that makes it hard to pick the right name for this library right away, so let's simply rename it in the future if we need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, but let's name it
tensorflow-utiljust to be consistent with the package name? Or inversely, let's name the packageorg.tensorflow.utils? I don't have preference for either, I just think that consistency is a good thing to have whenever it makes sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to have them mismatch here,
tensorflow-utilssounds more natural for the name of library (as there is more than one utility class in it) while package names are often singular by convention (e.g.java.util.*).In addition, we don't have a
org.tensorflow.corepackage neither in our core artifacts.