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

tensorflow_io namespace? #117

Closed
suphoff opened this issue Feb 25, 2019 · 5 comments
Closed

tensorflow_io namespace? #117

suphoff opened this issue Feb 25, 2019 · 5 comments

Comments

@suphoff
Copy link
Contributor

suphoff commented Feb 25, 2019

Should tensorflow-io export any symbols in the tensorflow namespace?

In most cases directly using the tensorflow namespace the is not an issue.
Many tensorflow-io kernels are fully contained in an anonymous namespace and REGISTER_OP just defines a static variable.

However any functionality requiring more then a single file currently exports symbols in the tensorflow namespace. If we ever get a symbol collision with tensorflow then symbol resolution may cause interesting issues.

Should tensorflow-io use a tensorflow_io workspace?
( And is there an automated way to test/protect against exporting symbols into the tensorflow namespace?)

@yongtang
Copy link
Member

I think we could create a different namespace, or anom namespace if possible. The tensorflow-io is always complied with tensorflow, so even if there is a collision we will be able to find out quickly I believe.

@suphoff
Copy link
Contributor Author

suphoff commented Feb 25, 2019

I am a bit more worried about tensorflow changes causing collisions with current/past versions of tensorflow-io then the other way round.

@yongtang
Copy link
Member

@suphoff At the moment we pin to a specific version of tensorflow for each of the past releases. For example, in the past we pin to 1.12.0 in tensorflow-io 0.1.0-0.3.0. For the upcoming tensorflow-io 0.4.0 we plan to pin to 1.13.0.

The reason is that while our python code has largely been forward compatible, there are still some C++ changes in tensorflow from time to time to make it backward/forward incompatible. One example is #54 (comment)

Since tensorlow-io tends to release faster than tensorflow (at least in the near future), I would be less worried about the namespace collision for now. But if there is anything we could do to make it future compatible, e.g., have a namespace tensorflow_io or anonymous namespace, it would be great as well.

@suphoff
Copy link
Contributor Author

suphoff commented Feb 25, 2019

@yongtang : I am mechanically changing the name space of my prototype for #109
from:

namespace tensorflow {
// ....
}  // tensorflow

to

namespace tensorflow_io {
using namespace ::tensorflow;
// ....
}  // tensorflow_io

As sources are distributed over several files anonymous namespaces are not really an option.
The prototype should be ready in a few days and can then serve as an example. (Good or bad)

@yongtang
Copy link
Member

With the recently updates, most of the C++ files should have the correct namespace:

namespace tensorflow {
namespace io {
namespace { // if applicable use anonymous namespace

} // namespace
} // namespace io
} // namespace tensorflow

Think this issue could be closed.

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

2 participants