-
Notifications
You must be signed in to change notification settings - Fork 9
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
Server refactor #23
Server refactor #23
Conversation
import os | ||
import json | ||
|
||
from fastapi import FastAPI |
Check warning
Code scanning / vcs-diff-lint
Unable to import 'fastapi' Warning
import json | ||
|
||
from fastapi import FastAPI | ||
from pydantic import BaseModel |
Check warning
Code scanning / vcs-diff-lint
Unable to import 'pydantic' Warning
cf5d8b4
to
751ffba
Compare
from urllib.parse import urlparse | ||
from urllib.request import urlretrieve | ||
|
||
import requests |
Check warning
Code scanning / vcs-diff-lint
Unable to import 'requests' Warning
from urllib.request import urlretrieve | ||
|
||
import requests | ||
import progressbar |
Check warning
Code scanning / vcs-diff-lint
Unable to import 'progressbar' Warning
import requests | ||
import progressbar | ||
|
||
from llama_cpp import Llama |
Check warning
Code scanning / vcs-diff-lint
Unable to import 'llama_cpp' Warning
@@ -0,0 +1,132 @@ | |||
import logging |
Check warning
Code scanning / vcs-diff-lint
Similar lines in 2 files ==logdetective.logdetective:[63:70] ==logdetective.server:[37:44] log_summary = extractor(log) Warning
==logdetective.logdetective:[63:70]
==logdetective.server:[37:44]
log_summary = extractor(log)
Last commit has a typo |
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 to me. Just a few nitpicks and questions.
logdetective/logdetective.py
Outdated
parser.add_argument("-M", "--model", type=str, default=DEFAULT_ADVISOR) | ||
parser.add_argument("-S", "--summarizer", type=str, default="drain") | ||
parser.add_argument("-N", "--n_lines", type=int, default=5) | ||
parser.add_argument("url", type=str, default="", help="The URL of the log file to be analyzed.") |
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 also taking local paths now. I think it should be mentioned in help
.
logdetective/logdetective.py
Outdated
type=str, default=DEFAULT_ADVISOR) | ||
parser.add_argument("-S", "--summarizer", type=str, default="drain", | ||
help="Choose between LLM and Drain template miner as the log summarizer.\ | ||
LLM must be specified as path to a model, URL or local.") |
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.
I wonder what local
means here.
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.
in the file system.
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.
I think it needs more explanation. Does it mean path to local model or name of the model in cache directory? If the second how it should be specified.
Could you please improve the help a bit?
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 path, not a name, the help says that. At no point is any cache mentioned. User shouldn't know about it and he shouldn't even know about it. It's an implementation detail.
9858ba1
to
9f2a1f4
Compare
I can see the timeout is configurable but we'll need to increase the default to at least 10 minutes; it's really frustrating to wait just to get timeouted |
I finally have a successful run \o/
|
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, none of the comments/suggestions are blocking
self.verbose = verbose | ||
self.context = context | ||
|
||
def __call__(self, log: str) -> str: |
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.
I'm not a fan of callable instances of classes. It looks pretty confusing in the code.
parser.add_argument("-v", "--verbose", action='count', default=0) | ||
parser.add_argument("-q", "--quiet", action='store_true') |
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.
we need to get logging setup to the server as well
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.
You mean the silent option? I was thinking about that and decided against it. The major reason for quiet mode is that all the output pollutes the terminal and makes things difficult to read. But if it's going to run as a server, we don't need to worry about that.
Especially if it's going to be containerized, because in that case the only logs stored for the pod will all belong to our service, and will be easy to filter.
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.
sorry, I commented on the wrong line
my point is that there is no logger handler for logdetective module when running directly from CLI using fastapi so there are no logs printed in the console; I manually added one while playing with this PR so that I could see what's happening
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.
Thanks for the updates, still LGTM!
Enabling runtime setting of cluster number Removing poetry import Reworked help New arg for clusters Adding fastapi dependency Moved inference and model initialization into utils Adding pydantic for server Prompt simplified We no longer need to notify model about special substrings, now that we retrieve representative log samples instead of templates. Exposing n_clusters arg Restoring local retrieval of logs Docstrings and import rearangement Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
General code refactoring in anticipation of deployment as a server. The server option is only intended for experimental use, and will not be documented at this time.
Among other changes: