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

fix(integrations): fix TF compatibility issues with WandbModelCheckpoint #4432

Merged
merged 25 commits into from Nov 30, 2022

Conversation

soumik12345
Copy link
Contributor

@soumik12345 soumik12345 commented Nov 3, 2022

Fixes GROWTH2-109

Description

This PR fixes compatibility issue of WandbModelCheckpoint with TF version < 2.6.0.

Co-author: @ayulockin

Testing

How was this PR tested?

Checklist

  • Include reference to internal ticket "Fixes WB-NNNN" and/or GitHub issue "Fixes #NNNN" (if applicable)
  • Ensure PR title compliance with the conventional commits standards

@soumik12345 soumik12345 self-assigned this Nov 3, 2022
@soumik12345 soumik12345 marked this pull request as draft November 3, 2022 12:48
@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #4432 (423b075) into main (48f9258) will decrease coverage by 0.04%.
The diff coverage is 83.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4432      +/-   ##
==========================================
- Coverage   83.02%   82.98%   -0.05%     
==========================================
  Files         261      261              
  Lines       33242    33257      +15     
==========================================
- Hits        27600    27599       -1     
- Misses       5642     5658      +16     
Flag Coverage Δ
functest 56.65% <83.33%> (+0.01%) ⬆️
unittest 73.19% <16.66%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
...db/integration/keras/callbacks/model_checkpoint.py 72.97% <83.33%> (-10.08%) ⬇️
wandb/integration/tensorboard/monkeypatch.py 91.35% <0.00%> (-2.47%) ⬇️
wandb/filesync/step_prepare.py 93.67% <0.00%> (-1.27%) ⬇️
wandb/sdk/interface/interface_shared.py 86.56% <0.00%> (-0.78%) ⬇️
wandb/sdk/internal/internal_api.py 89.17% <0.00%> (-0.52%) ⬇️
wandb/sdk/wandb_setup.py 88.44% <0.00%> (-0.51%) ⬇️
wandb/sdk/lib/mailbox.py 93.15% <0.00%> (-0.35%) ⬇️
wandb/sdk/wandb_artifacts.py 83.61% <0.00%> (-0.11%) ⬇️
wandb/cli/cli.py 69.40% <0.00%> (-0.10%) ⬇️
... and 3 more

@soumik12345 soumik12345 changed the title fixed version issues wrt keras Fixed TPU compatibility issues with WandbModelCheckpoint Nov 3, 2022
@soumik12345 soumik12345 changed the title Fixed TPU compatibility issues with WandbModelCheckpoint Fix TPU compatibility issues with WandbModelCheckpoint Nov 3, 2022
@ayulockin ayulockin changed the title Fix TPU compatibility issues with WandbModelCheckpoint feat(integrations): fix TPU compatibility issues with WandbModelCheckpoint Nov 8, 2022
@ayulockin ayulockin changed the title feat(integrations): fix TPU compatibility issues with WandbModelCheckpoint fix(integrations): TPU compatibility issues with WandbModelCheckpoint Nov 8, 2022
@github-actions github-actions bot added cc-fix and removed cc-feat labels Nov 8, 2022
@ayulockin ayulockin changed the title fix(integrations): TPU compatibility issues with WandbModelCheckpoint fix(integrations): TF 2.4.x compatibility issues with WandbModelCheckpoint Nov 8, 2022
@github-actions github-actions bot added cc-fix and removed cc-fix labels Nov 8, 2022
@ayulockin ayulockin marked this pull request as ready for review November 8, 2022 17:44
@ayulockin ayulockin changed the title fix(integrations): TF 2.4.x compatibility issues with WandbModelCheckpoint fix(integrations): TF compatibility issues with WandbModelCheckpoint Nov 8, 2022
@github-actions github-actions bot added cc-fix and removed cc-fix labels Nov 8, 2022
Copy link
Member

@dmitryduev dmitryduev left a comment

Choose a reason for hiding this comment

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

One nit, otherwise LGTM. Thanks!

wandb/integration/keras/callbacks/model_checkpoint.py Outdated Show resolved Hide resolved
@ayulockin
Copy link
Member

Hey @dmitryduev is this PR ready to be merged? :)

@dmitryduev dmitryduev self-requested a review November 16, 2022 19:18
Copy link
Member

@dmitryduev dmitryduev left a comment

Choose a reason for hiding this comment

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

Thanks @ayulockin - please see my comments.

wandb/integration/keras/callbacks/model_checkpoint.py Outdated Show resolved Hide resolved
@@ -171,3 +181,10 @@ def _check_filepath(self) -> None:
"This ensures correct interpretation of the logged artifacts.",
repeat=False,
)

def _patch_tf_keras_version(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

the name is a bit misleading as you're not patching anything... I'd do something like
self._is_old_tf_keras_version: Optional[bool] = None
in init
then
convert this one into a property called is_old_tf_keras_version that sets the private self._is_old_tf_keras_version on first invocation and then just returns it -- there's no need to recalculate it every time.

Copy link
Member

Choose a reason for hiding this comment

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

caching was not fixed, I added it in 67756e1

@ayulockin
Copy link
Member

ayulockin commented Nov 24, 2022

Hey @dmitryduev I made the changes as per your feedback.

However, I am not sure if I did this correctly: "You need to add this new shard to both tox.ini and Circle config.yml for the tests to be run."

Also mypy is passing locally but it's failing here.

@ayulockin
Copy link
Member

Hey @dmitryduev the code-check is failing because of this: ERROR: InvocationError for command /mnt/ramdisk/.tox/codecovcheck/bin/python tools/coverage-tool.py check (exited with code 1). What's missing to fix this?

I am getting the same InvocationError when testing locally.

@dmitryduev dmitryduev added this to the sdk-2022-12.1 milestone Nov 30, 2022
@github-actions github-actions bot added cc-fix and removed cc-fix labels Nov 30, 2022
@dmitryduev dmitryduev self-requested a review November 30, 2022 19:30
@dmitryduev dmitryduev merged commit 8c7d8b3 into main Nov 30, 2022
@dmitryduev dmitryduev deleted the fix/keras-version-patch branch November 30, 2022 19:31
@kptkin kptkin changed the title fix(integrations): TF compatibility issues with WandbModelCheckpoint fix(integrations): fix TF compatibility issues with WandbModelCheckpoint Dec 6, 2022
@github-actions github-actions bot added cc-fix and removed cc-fix labels Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants