Skip to content

Conversation

@ashutosh1919
Copy link
Contributor

Related to #1294 .

@gabrieldemarmiesse - Please review this one.

@bot-of-gabrieldemarmiesse

@SSaishruthi @manzilz @shreyashpatodia @PhilJd @pkan2 @junjiek @CyberZHG @lokhande-vishnu

You are owners of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

Fixes look good, tyvm for the catch and much needed tests! Would you mind using the serialization test util for the comparisons though:
https://github.com/tensorflow/addons/blob/master/tensorflow_addons/testing/serialization.py#L53

In the future we could add a specialized check_optimizer_serialization as well (not needed for this PR).

@ashutosh1919
Copy link
Contributor Author

@seanpmorgan , I think the method you suggested is too constrained.
I am checking this serialization.check_config(new_optimizer.get_config(), optimizer, False) and it is giving error as AttributeError: 'ConditionalGradient' object has no attribute '__name__'. What do it mean by that?

@ashutosh1919
Copy link
Contributor Author

@seanpmorgan ,Please take a look at latest commit. With the use of check_config() in serialization.py, I have modified only one file and its tests - conditional_gradient.py and conditional_graident_test.py files. I have also slightly modified the function check_config(). Please review these three files and guide me whether I am doing right or not?

0xshreyash
0xshreyash previously approved these changes Apr 26, 2020
@seanpmorgan
Copy link
Member

seanpmorgan commented Apr 26, 2020

Hi @ashutosh1919 so the bazel tests are failing because you have to explicitly link python imports that are not in the same directory. See #1740 (just the optimizer BUILD) as a fix or we can merge master into this branch after that PR merges.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@seanpmorgan
Copy link
Member

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@seanpmorgan seanpmorgan added this to the 0.10 milestone Apr 26, 2020
@seanpmorgan
Copy link
Member

@seanpmorgan , I think the method you suggested is too constrained.
I am checking this serialization.check_config(new_optimizer.get_config(), optimizer, False) and it is giving error as AttributeError: 'ConditionalGradient' object has no attribute '__name__'. What do it mean by that?

Ah, this is a bit too restrictive. Could you revert your commits using the shared serialization check please. I'll create an issue to standardize all optimizer serialization checks, but want to get the bugfix in, using your first serialization check.

@ashutosh1919
Copy link
Contributor Author

@seanpmorgan - I have reverted the changes. And please create a separate issue to add serailizability check for optimizers in serialization.py. I definitely want to work on it.

Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the fix! #1747 created for the next steps!

@seanpmorgan seanpmorgan merged commit c91cf2d into tensorflow:master Apr 27, 2020
ashutosh1919 added a commit to ashutosh1919/addons that referenced this pull request Jul 12, 2020
…ty bug in yogi (tensorflow#1728)

* Added serializability tests in all optimizers and fixed serializability bug in yogi
jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020
…ty bug in yogi (tensorflow#1728)

* Added serializability tests in all optimizers and fixed serializability bug in yogi
@ashutosh1919 ashutosh1919 deleted the serializability_tests branch May 11, 2021 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants