-
Notifications
You must be signed in to change notification settings - Fork 201
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
Make all TF operations visible via the Ops API #533
Conversation
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.
Most of this looks fine, but some of them probably shouldn't be visible (e.g. EstimatorOps and RiscOps).
* | ||
* @see {@link Ops} | ||
*/ | ||
public final class EstimatorOps { |
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.
Didn't estimators get removed in TF 2.16? Or is this a different meaning of estimator? It looks like it's about boosted trees and all that stuff got move out into tfdf.
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.
Yeah... we have different options:
1. Preserve the operator and the Ops classes but marking them as deprecated (the generator used to support that and I think that is still the case)
2. Leave the operator classes but not the Ops classes
3. Discard all of them
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.
Yep, looks like everything under estimator
in Python is deprecated: https://www.tensorflow.org/versions/r2.15/api_docs/python/tf/estimator
Since we haven't released anything officially yet, I suggest that we skip them entirely
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.
* | ||
* @see {@link Ops} | ||
*/ | ||
public final class RiscOps { |
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.
Any idea what this is for? It looks like it's just some core ops but for a different CPU, but we don't know what CPU.
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.
Google (the search engine) tells me it is related to TensorFlow Lite on RISC-V: https://hackmd.io/@nctu-cas-lab/BJ75A0IsK
So is not making much sense for TFJava?
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.
Yeah, I think we should hide those.
Many operations in TensorFlow are by default "hidden" to the public API. While this default visibility setting should be set agnostically of any language bindings, often it seems that it is hidden only because the Python API is wrapping it. Marking all operators as "visible" in the TF Java API definitions makes them more easily accessible to Java users.