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-8710] Support logging from multiple processes with wandb-service #3285

Merged
merged 33 commits into from
Mar 8, 2022

Conversation

kptkin
Copy link
Collaborator

@kptkin kptkin commented Feb 23, 2022

Fixes WB-8710

Description

What does the PR do?

  • This PR is the second in the series of PRs dealing with the process of moving the step logic out of the user process and into the internal process. For this PR we require that Image don't depend on step for visualization in the app and adding more tests for multiprocessing cases for step logic.
  • This PR also fixes issues that were overlooked in the attach PR: #3308. Moving the set up of globals after we finish _attach and add _attach_pid to indicate current process and save _init_pid for the source pid

Testing

How was this PR tested?

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 Feb 24, 2022

Codecov Report

Merging #3285 (e223ac0) into master (b3c989a) will increase coverage by 0.05%.
The diff coverage is 94.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3285      +/-   ##
==========================================
+ Coverage   81.32%   81.38%   +0.05%     
==========================================
  Files         232      232              
  Lines       28304    28320      +16     
==========================================
+ Hits        23019    23047      +28     
+ Misses       5285     5273      -12     
Flag Coverage Δ
functest 57.80% <92.38%> (+0.09%) ⬆️
unittest 71.67% <79.04%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
wandb/sdk/wandb_init.py 85.08% <ø> (-0.04%) ⬇️
wandb/sdk/wandb_run.py 89.69% <92.20%> (+0.27%) ⬆️
wandb/sdk/interface/interface.py 89.57% <100.00%> (-0.06%) ⬇️
wandb/sdk/internal/handler.py 90.89% <100.00%> (+0.27%) ⬆️
wandb/sdk/lib/git.py 76.35% <0.00%> (ø)
wandb/sdk/service/server_sock.py 92.70% <0.00%> (+1.04%) ⬆️
wandb/filesync/step_prepare.py 94.28% <0.00%> (+1.42%) ⬆️
wandb/sdk/internal/meta.py 90.74% <0.00%> (+3.08%) ⬆️

wandb/sdk/wandb_run.py Outdated Show resolved Hide resolved
@kptkin kptkin changed the title modify step logic [WB-6947] Move step logic to the internal process Feb 25, 2022
wandb/sdk/wandb_run.py Outdated Show resolved Hide resolved
@kptkin kptkin requested a review from raubitsj March 2, 2022 03:51
@kptkin kptkin marked this pull request as ready for review March 3, 2022 15:38
@kptkin kptkin changed the title [WB-6947] Move step logic to the internal process [WB-8710] Move step logic to the internal process Mar 3, 2022
@kptkin kptkin requested review from KyleGoyette and dmitryduev March 5, 2022 00:29
wandb/sdk/wandb_run.py Outdated Show resolved Hide resolved
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! A few minor points.

functional_tests/mp/06-1-share-child-base.yea Outdated Show resolved Hide resolved
functional_tests/mp/06-1-share-child-base.py Outdated Show resolved Hide resolved
functional_tests/mp/06-1-share-child-base.py Outdated Show resolved Hide resolved
functional_tests/mp/06-2-share-child-gt-step.py Outdated Show resolved Hide resolved
wandb/sdk/wandb_init.py Show resolved Hide resolved
):
if cls._is_attaching:
message = f"Trying to attach `{func.__name__}` while in the middle of attaching `{cls._is_attaching}`"
raise RuntimeError(message)
Copy link
Member

Choose a reason for hiding this comment

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

add a yea test to check this logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is hard to do that since we need a Run method that calls another Run method while we are doing the attaching... it means to break the current code... I could override some Run function don't know if it has a lot of value...

Copy link
Member

Choose a reason for hiding this comment

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

then could this ever happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah if for example i add:

@Attach._attach
def new_func(self, ...):
     self.log(....)

and someone trying to use the new function in and attached process, it should fail... but it means that we wrote a bad code... I guess we need a way to test all the new methods in functional tests... (with attach)

wandb/sdk/wandb_run.py Show resolved Hide resolved
wandb/sdk/wandb_run.py Show resolved Hide resolved
wandb/sdk/wandb_run.py Show resolved Hide resolved
@kptkin kptkin merged commit fe46ec7 into master Mar 8, 2022
@kptkin kptkin deleted the WB-6947-2 branch March 8, 2022 22:32
@kptkin kptkin added this to the sdk-2022-04.1 milestone Mar 25, 2022
@raubitsj raubitsj changed the title [WB-8710] Move step logic to the internal process [WB-8710] Support logging from multiple processes with wandb-service Apr 4, 2022
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.

2 participants