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

Prevent setting attributes on KerasTensor #19920

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

james77777778
Copy link
Contributor

This PR should improve consistency when using KerasTensor

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.98%. Comparing base (558d38c) to head (93b191f).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #19920      +/-   ##
==========================================
- Coverage   79.02%   78.98%   -0.05%     
==========================================
  Files         499      499              
  Lines       46436    46513      +77     
  Branches     8548     8561      +13     
==========================================
+ Hits        36695    36737      +42     
- Misses       8015     8044      +29     
- Partials     1726     1732       +6     
Flag Coverage Δ
keras 78.84% <100.00%> (-0.05%) ⬇️
keras-jax 62.43% <100.00%> (+0.01%) ⬆️
keras-numpy 57.33% <100.00%> (+0.11%) ⬆️
keras-tensorflow 63.62% <100.00%> (-0.03%) ⬇️
keras-torch 62.39% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. In what cases would we set these attributes? Shouldn't these properties be immutable, like they are for a concrete tensor?

@james77777778
Copy link
Contributor Author

james77777778 commented Jun 27, 2024

Thanks for the PR. In what cases would we set these attributes? Shouldn't these properties be immutable, like they are for a concrete tensor?

Sometimes, it is convenient to change them inplace in compute_output_spec:

keras/keras/src/ops/core.py

Lines 105 to 119 in d318fc7

def compute_output_spec(self, f, init, xs, length):
if xs is None:
n = int(length)
x = None
else:
n = (
int(length)
if length is not None
else tree.flatten(xs)[0].shape[0]
)
x = xs[0]
carry_spec, y_spec = backend.compute_output_spec(f, init, x)
y_spec.shape = (n,) + y_spec.shape
return carry_spec, y_spec

elif backend.is_keras_tensor(x):
if self._should_cast(x, autocast, dtype):
x.dtype = dtype
return x

Or should these be considered inappropriate?

@fchollet
Copy link
Member

Or should these be considered inappropriate?

I would say it's better to create a new KerasTensor instance for these (the existing instance might be in use elsewhere). I'd expect these properties to be immutable.

@james77777778
Copy link
Contributor Author

Got it. Let me try to fix them.

@james77777778 james77777778 changed the title Add setters for properties of KerasTensor Prevent setting attributes on KerasTensor Jun 27, 2024
@james77777778 james77777778 force-pushed the improve-keras-tensor branch 2 times, most recently from 6e57ab6 to f25027c Compare June 27, 2024 06:41
Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

keras/src/dtype_policies/dtype_policy.py Outdated Show resolved Hide resolved
keras/src/ops/core.py Outdated Show resolved Hide resolved
@james77777778
Copy link
Contributor Author

james77777778 commented Jun 28, 2024

I've added a clearer error msg for the setters of KerasTensor.
This should guide anyone who tries to modify these attributes to create a new instance.

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Jun 28, 2024
@fchollet fchollet merged commit f26f550 into keras-team:master Jun 28, 2024
6 checks passed
@google-ml-butler google-ml-butler bot removed awaiting review ready to pull Ready to be merged into the codebase kokoro:force-run labels Jun 28, 2024
@james77777778 james77777778 deleted the improve-keras-tensor branch June 28, 2024 05:31
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
PR Queue
Assigned Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

4 participants