-
Notifications
You must be signed in to change notification settings - Fork 46
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
Remove changes to support logging for AI #661
Conversation
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.
One minor thing, @jlewi.
internal/cmd/server.go
Outdated
@@ -137,9 +137,10 @@ The kernel is used to run long running processes like shells and interacting wit | |||
cmd.Flags().StringVarP(&addr, "address", "a", defaultAddr, "Address to create unix (unix:///path/to/socket) or IP socket (localhost:7890)") | |||
cmd.Flags().BoolVar(&devMode, "dev", false, "Enable development mode") | |||
cmd.Flags().BoolVar(&enableRunner, "runner", true, "Enable runner service (legacy, defaults to true)") | |||
cmd.Flags().BoolVar(&enableAILogs, "ai-logs", false, "Enable logs to support training an AI") | |||
// The AIFlag is no longer used, we can remove it as soon as the option has been removed from the frontend. | |||
cmd.Flags().BoolVar(&enableAILogs, "ai-logs", false, "(Obsolete) This flag is no longer used.") |
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.
Perhaps add _ = cmd.Flags().MarkDeprecated("ai-logs", "This flag is no longer used.")
instead of a note?
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.
Done.
Re test failures. Seems like test passes locally at.
Lets hope it passes at the latest commit in CI. |
Seems like a different error this time in CI
When I ran it locally; I got a different test failure.
|
Quality Gate failedFailed conditions |
Tests pass on retry; seems like there might be some flakiness. But I couldn't reproduce the flakiness locally.
|
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.
✅ LGTM
As described in jlewi/foyle#211 we will no longer rely on processing RunMe grpc logs to train the AI. This means we can simplify the Runme logging code and revert some of the changes in #585
We can simplify initialization and configuration of the logger because we no longer need to always log to a file
We can remove the tests to verify the log messages for the request are always there
The
ai-logs
flag should no longer be needed but I think its best to keep it until we have rolled out the changes to the frontend to ensure it never sets the flag.Related to Use the Foyle API to report Log Events vscode-runme#1589