-
Notifications
You must be signed in to change notification settings - Fork 621
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(sdk): add infrastructure for roll-out of next generation python client #7641
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7641 +/- ##
==========================================
+ Coverage 74.29% 75.76% +1.47%
==========================================
Files 500 502 +2
Lines 55727 54168 -1559
==========================================
- Hits 41400 41041 -359
+ Misses 13918 12718 -1200
Partials 409 409
Flags with carried forward coverage won't be shown. Click here to find out more.
|
client/pkg/session/session.go
Outdated
|
||
// setupLogger sets up the default logger for the session | ||
func (s *Session) setupLogger() { | ||
if file, _ := observability.GetLoggerPath("client"); file != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invert if statement to reduce nesting
if x { doLotsOfStuff() }
=> if !x { return } doLotsOfStuff()
client/pkg/session/session.go
Outdated
CorePath string | ||
} | ||
|
||
// Session manages the lifecycle of a connection session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not immediately clear to me that this refers to a connection to the wandb-core internal process
Session is a connection to the internal "core" process.
also, maybe it would be more informative to call this InternalProcess
instead of Session
. Each "session" corresponds to one process, but the naming leaves open the possibility that there could be multiple sessions per process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elaborated what connection we're referring to in the docstring.
As for the naming, I think Session is more appropriate. We are making it public with the intent to allow Go users to connect to a wandb-core service already running either locally or on the network.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the intent to allow Go users to connect to a wandb-core service already running
I see, that makes sense. It has a Start()
method that launches a new internal process; I suppose we'd eventually add a Connect()
method?
What if a user wants to start a process without connecting to it, with the intention to connect from other processes? It seems there's an opportunity to separate concepts:
WandbProcess
is a reference to a running process.WandbProcess::Connect() Session
opens and returns a connection to the process.WandbProcess::Kill()
kills the process.
Session
is a connection to a running W&B process.Session::Close()
closes the connection and frees any resources.- Other methods for communicating with the session.
These should be interfaces to make code that uses them testable.
Optionally, we can have WandbProcessLauncher
and SessionStarter
, but I think we can get away with top-level Launch(settings) WandbProcess
and Connect(address) Session
functions.
The simplest usage in Go would be:
process := wandb.Launch(...)
defer process.Kill()
session := process.Connect()
defer session.Close()
session.Send(...)
Or to connect to a process running elsewhere:
session := wandb.Connect(...)
defer session.Close()
session.Send(...)
client/pkg/session/session.go
Outdated
// internalProcess is the launcher that manages the wandb-core process | ||
internalProcess *launcher.Launcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by the name, this sounds like it would launch the process, not manage it
client/pkg/session/session.go
Outdated
CorePath string | ||
} | ||
|
||
// Session manages the lifecycle of a connection session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the intent to allow Go users to connect to a wandb-core service already running
I see, that makes sense. It has a Start()
method that launches a new internal process; I suppose we'd eventually add a Connect()
method?
What if a user wants to start a process without connecting to it, with the intention to connect from other processes? It seems there's an opportunity to separate concepts:
WandbProcess
is a reference to a running process.WandbProcess::Connect() Session
opens and returns a connection to the process.WandbProcess::Kill()
kills the process.
Session
is a connection to a running W&B process.Session::Close()
closes the connection and frees any resources.- Other methods for communicating with the session.
These should be interfaces to make code that uses them testable.
Optionally, we can have WandbProcessLauncher
and SessionStarter
, but I think we can get away with top-level Launch(settings) WandbProcess
and Connect(address) Session
functions.
The simplest usage in Go would be:
process := wandb.Launch(...)
defer process.Kill()
session := process.Connect()
defer session.Close()
session.Send(...)
Or to connect to a process running elsewhere:
session := wandb.Connect(...)
defer session.Close()
session.Send(...)
Co-authored-by: Timofey Peshin <timoffex@gmail.com>
Description
This PR adds infrastructure for the roll-out of the new Go-based python client.
Testing
How was this PR tested?