-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 leaderboard stats, periodic updates via fastapi-utils #724
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.
awesome
def compute_leader_score(self) -> int: | ||
return ( | ||
self.prompts | ||
+ self.replies_assistant * 4 |
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.
how about making these constants into settings?
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 find a "stable" formula for the leader-score .. calculation is "magic" right now .. the exact way how to calculate it needs more discussion, e.g. could be based on acceptance rations or include negative values..
oasst-shared/oasst_shared/utils.py
Outdated
|
||
|
||
def log_timing(func): | ||
def wrapped(*args, **kwargs): |
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.
just for prettiness, could use functools.wraps
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 made it now a param + non-param decorator .. with optional logging of the kwargs. regarding functool.wraps
..that makes sure that docstring and function name are preserved .. right?
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.
yes, it's just for clarity
# ### end Alembic commands ### | ||
|
||
|
||
def downgrade() -> None: |
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 upgrade drops and recreates the user_stats table, while the downgrade just modifies the columns. it's probably fine though
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.
Yes, intentionally. Somehow alembic didn't manage it to add a second column to the PK as a change, e.g. this table has a combined user_id + time_frame PK and before had only user_id. The table wasn't used before...
Provide new leaderboard REST endpoints:
/api/v1/leaderboards/{day,week,month,total}
Example response: