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

Make all TF operations visible via the Ops API #533

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

karllessard
Copy link
Collaborator

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.

Copy link
Collaborator

@Craigacp Craigacp left a 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 {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

Copy link
Collaborator

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 {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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.

@Craigacp Craigacp marked this pull request as ready for review April 5, 2024 19:21
@karllessard karllessard merged commit cff20e9 into tensorflow:master Apr 5, 2024
9 checks passed
@karllessard karllessard deleted the all-visible-ops branch April 5, 2024 19:22
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.

2 participants