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

Add prefix for all implemented ops #510

Closed
yongtang opened this issue Sep 26, 2019 · 18 comments
Closed

Add prefix for all implemented ops #510

yongtang opened this issue Sep 26, 2019 · 18 comments

Comments

@yongtang
Copy link
Member

TensorFlow RFC tensorflow/community#126 introduced namespaces for custom TF ops that are separated by > characters.

Since an op’s name uniquely identifies it, different TF packages should ensure their op names are globally unique across the entire TF ecosystem. To do so, prepend the package’s name to the op’s name and separate with a ‘>’. An op named “MatMul” inside the “tensorflow_addons” package should be named “Addons>MatMul”, for example.

We should follow the same prefix rule so that tensorflow-io plays well in overall tensorflow ecosystem.

@ghost
Copy link

ghost commented Oct 4, 2019

Hello @yongtang , was just understanding the changes needed for this,
If you are ok, I would like to take this up?. Can cite some examples here as well, before making changes, so that we can finalize the changes needed.

@yongtang
Copy link
Member Author

yongtang commented Oct 5, 2019

@ricky3dec That would be great!

@ghost
Copy link

ghost commented Oct 5, 2019

@yongtang thanks for your support!

Referring to one of the many classes here : tensorflow_io/pubsub/ops/dataset_ops.cc

REGISTER_OP("PubSubDataset") needs to be changed to REGISTER_OP("IO>PubSubDataset") i.e. prefix the ops with IO>.

And similarly it's usage should become and elsewhere as well:

REGISTER_KERNEL_BUILDER(Name("IO>PubSubDataset").Device(DEVICE_CPU),
PubSubDatasetOp);

If this looks ok, please assign this CR to me, and also please add me to the list of contributors.

Also, I assume the changes should be against: R0.8 branch?

Thanks!

@ghost
Copy link

ghost commented Oct 7, 2019

Hello @yongtang , if everything looks ok, can you help me get this going?

@yongtang yongtang assigned ghost Oct 8, 2019
@yongtang
Copy link
Member Author

yongtang commented Oct 8, 2019

@ricky3dec Assigned the issue to you, we always creates pull requests against master branch. Once it is in master branch it will be cherry-picked to a specific branch (if needed).

@ghost
Copy link

ghost commented Oct 8, 2019

Thanks @yongtang , will plan to send out the pull request soon.

@ghost
Copy link

ghost commented Oct 13, 2019

Hello @yongtang I created an initial pull request for this : #540.
I had followed the similar changes done for tensorflow_addons module.

Though I see some failure in nightly build:https://travis-ci.org/tensorflow/io/jobs/597223205. Please let me know If I am missing something obvious, until then I will try to debug the error. Since this is something new for me.

Please note I have not modified the python operations, please let me know if that is required too and also help with 1-2 examples.

Thanks

@ghost
Copy link

ghost commented Oct 13, 2019

@yongtang , I am able to debug locally now, please hold on the review for now, until I request again. Thanks!

@ghost
Copy link

ghost commented Oct 13, 2019

Hello @yongtang , I hope there are no more obvious issues with build (except that some jobs are timing out especially macos ones and linux one executed fine, so not sure about that).

You may review the PR now: #540

These are minimalistic changes, to make things going. Please let me know anything additional to be done as part of this

yongtang pushed a commit that referenced this issue Oct 14, 2019
* PR #510: Add prefix for all implemented ops in .cc modules

* PR #510: Add prefix for all implemented ops in .cc modules

* PR #510: Add prefix for all implemented ops in .cc modules

* PR #510: removed io namespace at some places.

* PR #510: removed io namespace at some places.

* PR #510: add io prefix to python ops as well.

* PR #510: and attempt to resolve conflicts.
@ghost
Copy link

ghost commented Oct 14, 2019

Thanks @yongtang for your support!

@ghost
Copy link

ghost commented Oct 15, 2019

@yongtang thanks for working on fixing additional tests, looks like I should have re-checked changes after the merge from master. I shall take care of it next time.

That brings to me another point, could you please share the build and test steps reference, so that I can do some sanity checks on my end?
Also, I think we should document these naming conventions in development guide, so that anyone contributing can follow them. I know it's not fool proof, but still some guidance would be there. Just a suggestion!

Is there anything pending in this issue now?

@yongtang
Copy link
Member Author

@ricky3dec The developing and testing is covered in https://github.com/tensorflow/io#python

It may not be very detailed or complete. If you find anything missing, we can improve the documentation and update.

The naming conventions is a good content for docs. Would be great if you can help?

Our initial docs directory has been setup in #498

The tensorflow/docs team is actively helping us getting tensorflow-io's documentation into tensorflow.org. I think it would be great if we could get documentation for development guide in tensorflow_io/docs directory. Then tensorflow/docs team can help us get the content published in tensorflow.org website!

@ghost
Copy link

ghost commented Oct 15, 2019

Sure @yongtang , I can work on that!

Can you please help create an issue and assign that to me, with details about what all needs to be taken care from documentation side?

@ghost
Copy link

ghost commented Oct 17, 2019

Hello @yongtang , I did not get a chance to follow up on all discussions, but just wanted to know, did we face any issues with current naming convention "IO>"? I see we had to change it to 'Io' as a prefix in current release.

@yongtang
Copy link
Member Author

@ricky3dec The "IO>" works with TF 1.15.0rc3 but not TF 1.15.0(final). Since we are pending release for 0.8.0 (compatible TF1.15.0), I switch toIoso that all python changes could remain (only C++ changes from "IO>"=>"Io"`.

Our 0.9.0 release (will happen as soon as we release 0.8.0) with switch back to "IO>" as this is the way to move forward.

See https://github.com/tensorflow/io/pull/555/commits which reverts back the change. (will only merge after 0.8.0 release).

@ghost
Copy link

ghost commented Oct 17, 2019

Thanks for your explanation @yongtang !

@ghost
Copy link

ghost commented Nov 6, 2019

Hello @yongtang , anything more pending in this issue? Not sure, when we actually close the issues :-)

@yongtang
Copy link
Member Author

yongtang commented Nov 6, 2019

@ricky3dec All have been completed. Thanks for the great work! 👍 🎉

@yongtang yongtang closed this as completed Nov 6, 2019
i-ony pushed a commit to i-ony/io that referenced this issue Feb 8, 2021
…ensorflow#540)

* PR tensorflow#510: Add prefix for all implemented ops in .cc modules

* PR tensorflow#510: Add prefix for all implemented ops in .cc modules

* PR tensorflow#510: Add prefix for all implemented ops in .cc modules

* PR tensorflow#510: removed io namespace at some places.

* PR tensorflow#510: removed io namespace at some places.

* PR tensorflow#510: add io prefix to python ops as well.

* PR tensorflow#510: and attempt to resolve conflicts.
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

No branches or pull requests

1 participant