Conversation
Codecov Report
@@ Coverage Diff @@
## master #3362 +/- ##
==========================================
+ Coverage 81.51% 81.56% +0.05%
==========================================
Files 234 234
Lines 28744 28760 +16
==========================================
+ Hits 23431 23459 +28
+ Misses 5313 5301 -12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
dmitryduev
left a comment
There was a problem hiding this comment.
One small suggestion, otherwise LGTM.
Also, we should either uncomment all those noop decorators or rm the comments :)
| _init_pid = run._init_pid | ||
| run._init_pid = _init_pid + 1 | ||
| with pytest.raises(MultiprocessError) as excinfo: | ||
| run.log(dict(this=1)) | ||
| assert "log() does not support multiprocessing" in str(excinfo.value) | ||
| assert "`log` does not support multiprocessing" in str(excinfo.value) | ||
| run._init_pid = _init_pid |
| class Dummy: | ||
| ... |
There was a problem hiding this comment.
This is a hacky hack, like it!
There was a problem hiding this comment.
why hacky hack? 😂
how would you do it?
There was a problem hiding this comment.
I just don't think I've seen such tricks before, just an expression of appreciation :)
There was a problem hiding this comment.
oh i see... got ya... so thanks 😊
| return False | ||
|
|
||
| @Attach._attach | ||
| # @_run_decorator._noop |
There was a problem hiding this comment.
Have you tried uncommenting these, does it break anything?
There was a problem hiding this comment.
yeah... it works!
@raubitsj prefers not to add it to all the functions, since he is worried that it will break users code that worked although it shouldn't have (log already had this logic before)
| if settings and settings["strict"]: | ||
| wandb.termerror(message, repeat=False) | ||
| raise errors.MultiprocessError( | ||
| f"`{func.__name__}` does not support multiprocessing" |
There was a problem hiding this comment.
Let's ping the user to use service here?
There was a problem hiding this comment.
sounds good... I can add a link to our doc
There was a problem hiding this comment.
wait... i have the link in the message wburls.get("multiprocess")
I just need to add a section about service in there
Yeah, I can remove it and just add it if we want to support other functions... (i was just keeping it so it would be easier for me to change later... but it is pure laziness ...) |
Fixes WB-6946
Fixes #NNNN
Description
What does the PR do?
Handle the non-service case: if the user tries to share a run we make all the log and finish a no-op
Testing
How was this PR tested?
Added new functional test + testing user warnings/errors in a unit-test
Checklist