-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add compat/tensorflow_stub for non-TensorFlow build #1453
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
Conversation
4fa08af
to
f8ee7fc
Compare
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 did some analysis of the parts of TensorFlow that are actually invoked by code within the tensorboard-notf build as represented by #1418, and we can prune a lot of the tensorflow_stub copied python files. I've made comments within the PR on how to break some of those unnecessary dependencies.
The only files that we actually need should be:
- app.py
- compat.py
- dtypes.py
- error_codes.py
- errors.py
- flags.py
- gfile.py
- logging.py
- pywrap_tensorflow.py
- resource_loader.py
- tensor_manip.py
- tensor_shape.py
tensorboard/compat/__init__.py
Outdated
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.
typo: later -> layer
tensorboard/compat/__init__.py
Outdated
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.
FWIW, does the notf
module even need to contain any symbols? It seems like we could just check if it can be imported, and if so, set USING_TF = False.
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.
Conventionally if there's one "main" target for a BUILD file, it's named the same thing as the directory, i.e. tensorflow_stub
in this case. This allows shorthand references to it as //tensorboard/compat/tensorflow_stub
(where it gets auto-expanded to //tensorboard/compat/tensorflow_stub:tensorflow_stub
).
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.
We can avoid the ops.Tensor reference here by just removing the special message in favor of:
if x is not None:
return x
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.
Let's just update this function so we skip the inoperative _FilterNotTensor entirely - this is the only place it's used so them we can remove the definition above and avoid the ops.Tensor reference inside it, which doesn't work anyway.
def _Assertconvertible(value, dtype):
# If dtype is None or not recognized, assume it's convertible.
if dtype is None or dtype not in _TF_TO_IS_OK:
return
fn_list = _TF_TO_IS_OK.get(dtype)
mismatch = _FirstNotNone([fn(values) for fn in fn_list])
if mismatch is not None:
raise TypeError(
"Expected %s, got %s of type '%s' instead."
% (dtype.name, repr(mismatch), type(mismatch).__name__)
)
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.
We can just replace tf_inspect with the standard library module here, since we won't be needing to handle any tf_decorators once we also remove those. So just import inspect as _inspect
.
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.
We don't actually need to use tf_decorator, we can just return wrapper
here. That way nothing we need depends on tf_decorator.py and we can remove it.
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.
None of the tf_export functionality is necessary for our purposes, so this whole file can be removed. I think essentially every reference to it is an @tf_export decorator that has already been commented out.
f8ee7fc
to
ced9f54
Compare
Thanks for the great review! All changes made to match the comments. Also, I was able to get projector plugin working without TensorFlow (and sample data from tensorboardX) so added that support as well. |
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.
Looking good - thanks for all the changes! I'm a little confused about the projector plugin - that would be great to have it work without TF, but I'd have expected it to need a few more changes/shims to avoid unimplemented TF codepaths. Do you happen to have the tensorboardX commands you used to produce data that made it work?
Also, I don't think the image.py resize_images() implementation is actually needed - I'm pretty sure the projector_plugin.py _make_sprite_image() logic is dead code, I don't see it invoked anywhere and I think the current usage has you create the sprite yourself (that's what tensorboardX does). Does the projector still work if you omit that?
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.
Is there something that still needs tf.registry/registry.py? I didn't see anything when I checked (but I could have missed something).
Let me check on resize_images and remove if it works without. For embedding, I think the README.md for https://github.com/lanpa/tensorboardX has an example |
And you were right - removed |
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.
Gotcha, I see - #1418 has other pending changes to projector_plugin.py to make it work, that's what I was confused by.
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 know it's tiny but could we put this change in #1418 with the rest of the projector_plugin changes as well? That way this PR isn't touching anything outside tensorboard/compat
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.
Will do.
Corrected and rebased #1418 on top. |
SGTM, I just merged #1451 so this has a couple conflicts - once those are resolved I'll merge this one. |
b26c521
to
bf6126a
Compare
Okay - should be good to merge. Once this lands I'll rebase #1418 |
This adds
compat/tensorflow_stub/
that can (mostly) replace the parts of TensorFlow used by TensorBoard. This will be used by thetensorflow-notf
build target.This will follow #1451
Broken out of #1418
cc @jart @martinwicke @ml7 @jspisak @nfelt