-
Notifications
You must be signed in to change notification settings - Fork 79
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 log support for dedicated deployment #335
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.
Left a couple of comments.
roboflow/deployment.py
Outdated
if log["insert_id"] in log_ids: | ||
continue | ||
log_ids.add(log["insert_id"]) | ||
print(f'[{datetime.fromisoformat(log["timestamp"]).strftime("%Y-%m-%d %H:%M:%S.%f")}] {log["payload"]}') |
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 what form is the log["payload"]? Is it always plaintext or can it be json too?
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 can be either text or json, but in our inference server, it only generate texts (no structured log, because it's not tied to any cloud provider)
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.
confirmed that the f
string support dict (json)
as well
roboflow/deployment.py
Outdated
status_code, msg = deploymentapi.get_deployment_log(api_key, args.deployment_name, from_timestamp, to_timestamp) | ||
if status_code != 200: | ||
print(f"{status_code}: {msg}") | ||
return |
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 should return a non-zero exit to the cli command when there is an error, this is useful in using the command in a script.
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.
Good idea!
deployment_log_parser.add_argument( | ||
"-d", "--duration", help="duration of log (from now) in seconds", type=int, default=3600 | ||
) | ||
deployment_log_parser.add_argument("-f", "--follow", help="follow log output", 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.
Is it worth having a flag to show the last n lines of the log?
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.
Good idea! Added
…boflow-python into dedicated-deployment-add-log
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!
Description
Add log support for dedicated deployment.
Type of change
Please delete options that are not relevant.
How has this change been tested, please provide a testcase or example of how you tested the change?
Tested locally.
Any specific deployment considerations
For example, documentation changes, usability, usage/costs, secrets, etc.
Docs