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

Add RemoteLogger #94

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

theogf
Copy link
Contributor

@theogf theogf commented Mar 14, 2024

Closes #52

This is the port from JuliaLang/julia#48121 by @simonbyrne .

I mostly reused the code from the PR, if that's not okay let me know I'll just close this PR.

I also did not made the modification on using this as a default for new workers as this would be unexpected behaviour.

function Logging.handle_message(logger::RemoteLogger, level::Logging.LogLevel, message, _module, _group, _id,
_file, _line; kwargs...)
@nospecialize
remote_do(logmsg, logger.pid, level, message, _module, _group, _id, _file, _line; pid=myid(), kwargs...)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I thought we could directly use handle_message from the targetted worker but I don't know how to fetch the current_logger there...

Copy link
Member

@fatteneder fatteneder left a comment

Choose a reason for hiding this comment

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

I don't have expertise with Distributed to approve, so we will need to find someone else to review.

src/logger.jl Outdated Show resolved Hide resolved
test/distributed_exec.jl Show resolved Hide resolved
@fatteneder
Copy link
Member

I mostly reused the code from the PR, if that's not okay let me know I'll just close this PR.

You could acknowledge @simonbyrne by adding a line

Co-authored-by: simonbyrne <simonbyrne@users.noreply.github.com>

to a commit message.

theogf added 3 commits July 20, 2024 15:08
Co-authored-by: simonbyrne <simonbyrne@users.noreply.github.com>
Copy link

codecov bot commented Jul 20, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.23%. Comparing base (af89e6c) to head (1857e37).

Files with missing lines Patch % Lines
src/logger.jl 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   79.14%   79.23%   +0.08%     
==========================================
  Files          10       11       +1     
  Lines        1899     1907       +8     
==========================================
+ Hits         1503     1511       +8     
  Misses        396      396              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JamesWrigley
Copy link
Collaborator

As mentioned in the original PR, this feels a bit lacking without support for scopes 🤔 If there's significant demand I would be ok with merging it under a Distributed.Experimental submodule with the understanding that the behaviour would change in the future when scopes become supported.

WRT supporting scopes I kinda like the idea here about RemoteScopedValue: JuliaLang/julia#35757 (comment)

@vchuravy, @jpsamaroo, any thoughts?

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.

add RemoteLogger for distributed logging (with PR code)
3 participants