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

feat(integrations): add ability to log learning rate using WandbMetricsLogger #4391

Merged
merged 37 commits into from Nov 30, 2022

Conversation

soumik12345
Copy link
Contributor

Adds GROWTH2-77

Description

Add support for tracking learning rate with WandbMetricsLogger

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

cc: @ayulockin @dmitryduev

Re-raising #4378

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 @soumik12345. Please fix the issues reported by mypy, they are all real. + looks like your functional test is either non-deterministic or something else is going on there.
Please ensure the CI is green as much as possible before marking the PR as ready for review and requesting reviews.

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #4391 (4e69732) into main (8535615) will increase coverage by 0.00%.
The diff coverage is 86.66%.

❗ Current head 4e69732 differs from pull request most recent head 152ca45. Consider uploading reports for the commit 152ca45 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4391   +/-   ##
=======================================
  Coverage   83.10%   83.11%           
=======================================
  Files         259      259           
  Lines       32984    33006   +22     
=======================================
+ Hits        27413    27434   +21     
- Misses       5571     5572    +1     
Flag Coverage Δ
functest 56.97% <86.66%> (+0.22%) ⬆️
unittest 73.28% <6.66%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
...andb/integration/keras/callbacks/metrics_logger.py 89.65% <86.66%> (-4.79%) ⬇️
wandb/sdk/wandb_run.py 90.72% <0.00%> (-0.18%) ⬇️
wandb/sdk/lib/git.py 79.01% <0.00%> (ø)
wandb/sdk/interface/artifacts.py 79.03% <0.00%> (+0.34%) ⬆️
wandb/sdk/internal/system/system_info.py 91.55% <0.00%> (+3.24%) ⬆️

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.

@soumik12345 the CI is still red, please address.

@soumik12345 soumik12345 marked this pull request as draft October 26, 2022 03:39
@soumik12345
Copy link
Contributor Author

@ayulockin I don't think that the WandbMetricsLogger callback from this branch is logging system metrics. Here's a sample run for your reference.

@soumik12345 soumik12345 marked this pull request as ready for review November 7, 2022 14:34
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 @soumik12345 @ayulockin. A few requests + would appreciate input on the system metrics stuff (see above).

@morganmcg1
Copy link
Member

@ayulockin , @soumik12345 can you update the name and description of the PR to reflect the broader updates that were added please?

@raubitsj raubitsj added this to the sdk-2022-12.1 milestone Nov 17, 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.

Looks great! Please add the tests (see comments) and it should be good to go in.

@ayulockin
Copy link
Member

Hey @dmitryduev should we have separate tests to cover the missing logics. Since we are only using yea test, should we have multiple yea tests to cover the different functionalities. What should be recommended here?

@dmitryduev
Copy link
Member

@ayulockin yea, adding more yea tests is probably the simplest thing to do here.

@dmitryduev dmitryduev merged commit 48f9258 into main Nov 30, 2022
@dmitryduev dmitryduev deleted the feature/keras-lr-track branch November 30, 2022 18:57
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

6 participants