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

Save Nested Layers For Rubik #377

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Conversation

NihalHarish
Copy link
Contributor

@NihalHarish NihalHarish commented Oct 19, 2020

Description of changes:

  • This PR is in the draft stage, I need to update tests, refactor and add comments.
  • Adds ability to save nested layers with the use of the _flatten_layers API. See tests/tensorflow2/test_nested_layers.py
  • Adds the ability to save repeated calls to the same layer object with model subclassing by storing the outputs of the layers in a list in the InputOutputSaver object. See tests/tensorflow2/test_model_that_reuses_layers.py
  • Creates a unique name for each layer output and input to make it more intuitive for the user. See tests/tensorflow2/test_concat_layer.py

Style and formatting:

I have run pre-commit install to ensure that auto-formatting happens with every commit.

Issue number, if available

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-io
Copy link

codecov-io commented Oct 19, 2020

Codecov Report

Merging #377 into master will decrease coverage by 2.72%.
The diff coverage is 98.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #377      +/-   ##
==========================================
- Coverage   85.49%   82.76%   -2.73%     
==========================================
  Files          86       86              
  Lines        6514     6539      +25     
==========================================
- Hits         5569     5412     -157     
- Misses        945     1127     +182     
Impacted Files Coverage Δ
smdebug/tensorflow/keras.py 80.03% <97.77%> (-12.29%) ⬇️
smdebug/tensorflow/utils.py 63.45% <100.00%> (-24.88%) ⬇️
smdebug/tensorflow/singleton_utils.py 83.33% <0.00%> (-16.67%) ⬇️
smdebug/tensorflow/callable_cache.py 69.56% <0.00%> (-13.05%) ⬇️
smdebug/tensorflow/collection.py 84.53% <0.00%> (-11.35%) ⬇️
smdebug/tensorflow/tensor_ref.py 82.25% <0.00%> (-6.46%) ⬇️
smdebug/tensorflow/base_hook.py 76.19% <0.00%> (-4.37%) ⬇️
smdebug/tensorflow/session.py 88.46% <0.00%> (-3.37%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 311a6f4...6826d4b. Read the comment docs.

@NihalHarish NihalHarish changed the title [Draft] Save Nested Layers For Rubik Save Nested Layers For Rubik Oct 20, 2020
@@ -794,9 +832,16 @@ def _save_layer_values(self, logs):
# Layer Inputs are flattened and passed as a list into
# the next layer. Unpacking it speeds up the _make_numpy fn.
layer_input = layer_input[0]
layer_input_tensor_name = get_export_name_for_keras(str(layer_name), "input")
layer_name = str(layer_name)
idx = layer_name_dict.get(layer_name, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this be 0 always because this is the first time the dict has been accessed after L820?

@@ -849,6 +894,9 @@ def _on_any_batch_end(self, batch, mode, logs=None):
self._export_model()
self._exported_model[self.mode] = True

if is_tf_version_2x():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any restriction for eager/non-eager/tf.function?

if tensor_type in ["input", "output", "weight"]:
if isinstance(layer, str):
# Tensor.name is meaningless when eager execution is enabled.
return f"{layer}/{tensor_type}s"
return f"{layer}_{layer_idx}/{tensor_type}_{tensor_idx}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this change how layer/tensor names have looked so far?

Comment on lines +559 to +579
layer_inputs = self.saved_layers[layer_name].layer_input
for layer_idx, tensor in enumerate(layer_inputs):
if isinstance(tensor, list):
tensor_list = tensor
else:
tensor_list = [tensor]
if hasattr(tensor_list[0], "numpy") is False:
self.logger.warning(
"cannot save layer values during forward pass with tf.function"
)
continue
else:
for t_idx, t in enumerate(tensor_list):
export_name = get_export_name_for_keras(
layer_name,
tensor_type="input",
tensor=tensor,
layer_idx=layer_idx,
tensor_idx=t_idx,
)
self._save_tensor_to_file(export_name, t, layer_collection)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks similar to what's done for outputs below. possible to make it common?

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.

3 participants