-
Notifications
You must be signed in to change notification settings - Fork 39
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
chore(weave): add weave.finish #1696
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=2efc932535932b9e4c660231e4eac5e886df860a |
weave/api.py
Outdated
# This is the stream-table backend. Disabling it in favor of the new | ||
# trace-server backend. | ||
# return _weave_init.init_wandb(project_name).client | ||
# return _weave_init.init_trace_remote(project_name).client | ||
return _weave_init.init_weave(project_name).client | ||
_current_inited_client = _weave_init.init_weave(project_name) |
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.
non blocking comment: can you share why the api.py
layer is responsible for handling the _current_inited_client
state? It seems like this might want to be part of weave_init.py
?
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.
no reasoning, it does make more sense to move that logic lower
@@ -104,15 +106,16 @@ def init_weave( | |||
entity_name, project_name, remote_server, ensure_project_exists | |||
) | |||
|
|||
init_client = InitializedClient(client) | |||
global _current_inited_client | |||
_current_inited_client = InitializedClient(client) |
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.
Should we be checking to see if _current_inited_client
is already set? If it is, would that be something we should throw for?
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.
Looks good - a few things (#1 is probably the main thing to consider):
- Can you add a test like the one in your PR description's screenshot
- See comment in
weave/weave_init.py
- The stuff around
because OpenAI calls are not logged in local mode currently
might not be true anymore - it might be a nice followup to move these two calls to the correct locations.
internal jira
https://wandb.atlassian.net/browse/WB-18715
You can now stop tracing stuff with weave.finish()
![Screenshot 2024-05-30 at 9 41 05 AM](https://private-user-images.githubusercontent.com/25037002/335306664-d3c4b38b-33e3-49fc-9cc5-f3ec105dfe2c.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTk5MTg0OTEsIm5iZiI6MTcxOTkxODE5MSwicGF0aCI6Ii8yNTAzNzAwMi8zMzUzMDY2NjQtZDNjNGIzOGItMzNlMy00OWZjLTljYzUtZjNlYzEwNWRmZTJjLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA3MDIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNzAyVDExMDMxMVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTI1NWU4N2U1NDQ4OTMxOTBmZjY5OGI3MWFlMDVkM2ZlZmMwN2FmYzg5ZjQwNzhiZDc0YWFhNjMzM2I3NmNiZGUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.mot6vkqid4hnU3erPc4B4UoJoATft3118GqE1zEgNh4)
Something like this now only traces the middle call