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

[WB-6947] Flush log data without finish with service #3137

Merged
merged 37 commits into from
Feb 22, 2022
Merged

Conversation

kptkin
Copy link
Contributor

@kptkin kptkin commented Jan 12, 2022

Fixes WB-6947

Description

What does the PR do?

This PR moves the partial history logic to the internal process. This PR allows for runs that use the wandb-service path to flush their remaining history without explicitly calling wandb.finish.

This is the first PR in series of PR that move the step tracking logic from the user process to the internal process.

Testing

How was this PR tested?
This PR should keep functionality, it is tested using the existing tests.

Checklist

  • Name PR "[WB-NNNN][WB-MMMM] Add support for..." similar to entries in CHANGELOG.md
  • Include reference to internal ticket "Fixes WB-NNNN" (and github issue "Fixes #NNNN" if applicable)

@codecov
Copy link

codecov bot commented Jan 12, 2022

Codecov Report

Merging #3137 (ed53556) into master (0976290) will increase coverage by 0.06%.
The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3137      +/-   ##
==========================================
+ Coverage   79.90%   79.96%   +0.06%     
==========================================
  Files         213      212       -1     
  Lines       27882    27908      +26     
==========================================
+ Hits        22278    22316      +38     
+ Misses       5604     5592      -12     
Flag Coverage Δ
functest 56.86% <81.77%> (+0.06%) ⬆️
unittest 69.35% <85.71%> (-0.19%) ⬇️

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

Impacted Files Coverage Δ
wandb/sdk/__init__.py 100.00% <ø> (ø)
wandb/sdk/wandb_watch.py 74.54% <80.00%> (-0.90%) ⬇️
wandb/wandb_torch.py 59.93% <83.33%> (+0.06%) ⬆️
wandb/sdk/data_types.py 84.31% <85.71%> (-0.04%) ⬇️
wandb/integration/tensorboard/log.py 86.79% <93.75%> (+0.12%) ⬆️
wandb/sdk/interface/interface.py 89.62% <94.44%> (+0.22%) ⬆️
wandb/sdk/internal/handler.py 90.61% <98.48%> (+0.25%) ⬆️
wandb/data_types.py 83.68% <100.00%> (+0.01%) ⬆️
wandb/integration/tensorboard/__init__.py 100.00% <100.00%> (ø)
wandb/integration/tensorflow/estimator_hook.py 83.87% <100.00%> (+1.72%) ⬆️
... and 16 more

wandb/sdk/wandb_run.py Outdated Show resolved Hide resolved
wandb/wandb_torch.py Outdated Show resolved Hide resolved
@kptkin kptkin requested a review from raubitsj January 18, 2022 20:21
wandb/sdk/wandb_run.py Outdated Show resolved Hide resolved
@kptkin kptkin marked this pull request as ready for review February 17, 2022 05:05
@raubitsj raubitsj added this to the sdk-2022-03.1 milestone Feb 18, 2022
Copy link
Member

@raubitsj raubitsj left a comment

Choose a reason for hiding this comment

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

PERFECT!

@@ -96,9 +92,6 @@ def add_log_hooks_to_pytorch_module(
if name is not None:
prefix = prefix + name

if jupyter_run:
Copy link
Member

Choose a reason for hiding this comment

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

Im just curious. did you look into why this was here and what it was accomplishing before?

Copy link
Contributor Author

@kptkin kptkin Feb 22, 2022

Choose a reason for hiding this comment

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

I couldn't find what is was doing... I saw it is used to recover history...
I guess I can keep it and just use it as a run... (if provided)

I did a simple example in a Notebook without it and it worked, but it's not a guarantee

model,
log_parameters=log_parameters,
log_gradients=log_gradients,
prefix=prefix,
log_freq=log_freq,
jupyter_run=wandb.run if in_jupyter else None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is using the same wandb.run so that's why i though it's ok to remove

@raubitsj raubitsj merged commit 924d0bc into master Feb 22, 2022
@raubitsj raubitsj deleted the WB-6947-1 branch February 22, 2022 21:43
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.

None yet

2 participants