add lb option to show-stats #283
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR extends the show-stats command with an optional leaderboard_name filter.
- Adds a
leaderboard_nameparameter togenerate_statsand internal stats methods - Updates SQL queries to apply leaderboard filtering when provided
- Exposes the option in the Discord slash command and installs
jqin the CI workflow
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/discord-cluster-manager/leaderboard_db.py | Added leaderboard_name parameter and SQL filters in stats |
| src/discord-cluster-manager/cogs/admin_cog.py | Exposed new command parameter with autocomplete |
| .github/workflows/nvidia_workflow.yml | Installed jq for JSON parsing |
Comments suppressed due to low confidence (2)
src/discord-cluster-manager/leaderboard_db.py:533
- Add unit tests for
generate_stats(and underlying methods) that cover scenarios with and withoutleaderboard_nameto validate the filtering logic.
def generate_stats(self, last_day: bool, leaderboard_name: Optional[str] = None):
src/discord-cluster-manager/leaderboard_db.py:543
- Add a leading space before 'AND' (e.g.,
f" AND ...") so the SQL clause doesn't run together and remains valid.
select_expr += f"AND s.leaderboard_id = (SELECT id FROM leaderboard.leaderboard WHERE name = '{leaderboard_name}')"
| select_expr += f"AND s.leaderboard_id = (SELECT id FROM leaderboard.leaderboard WHERE name = '{leaderboard_name}')" | ||
| if not last_day: | ||
| select_expr = select_expr.replace("AND", "WHERE") | ||
| # per-runner stats | ||
| self.cursor.execute( | ||
| f""" | ||
| SELECT | ||
| runner, | ||
| COUNT(*), | ||
| COUNT(*) FILTER (WHERE passed), | ||
| COUNT(score), | ||
| COUNT(*) FILTER (WHERE secret), | ||
| MAX(runs.start_time - s.submission_time), | ||
| AVG(runs.start_time - s.submission_time), | ||
| SUM(runs.end_time - runs.start_time) | ||
| FROM leaderboard.runs JOIN leaderboard.submission s ON submission_id = s.id | ||
| {select_expr} | ||
| GROUP BY runner; | ||
| """ | ||
| ) | ||
| f""" |
There was a problem hiding this comment.
Interpolating leaderboard_name directly into SQL with f-strings can lead to SQL injection. Use parameterized queries or query arguments instead.
| select_expr += f"AND s.leaderboard_id = (SELECT id FROM leaderboard.leaderboard WHERE name = '{leaderboard_name}')" | |
| if not last_day: | |
| select_expr = select_expr.replace("AND", "WHERE") | |
| # per-runner stats | |
| self.cursor.execute( | |
| f""" | |
| SELECT | |
| runner, | |
| COUNT(*), | |
| COUNT(*) FILTER (WHERE passed), | |
| COUNT(score), | |
| COUNT(*) FILTER (WHERE secret), | |
| MAX(runs.start_time - s.submission_time), | |
| AVG(runs.start_time - s.submission_time), | |
| SUM(runs.end_time - runs.start_time) | |
| FROM leaderboard.runs JOIN leaderboard.submission s ON submission_id = s.id | |
| {select_expr} | |
| GROUP BY runner; | |
| """ | |
| ) | |
| f""" | |
| select_expr += "AND s.leaderboard_id = (SELECT id FROM leaderboard.leaderboard WHERE name = %s)" | |
| if not last_day: | |
| select_expr = select_expr.replace("AND", "WHERE") | |
| # per-runner stats | |
| query = f""" |
Copilot uses AI. Check for mistakes.
| @@ -530,32 +530,36 @@ def get_leaderboard_submissions( | |||
| for submission in self.cursor.fetchall() | |||
| ] | |||
|
|
|||
| def generate_stats(self, last_day: bool): | |||
| def generate_stats(self, last_day: bool, leaderboard_name: Optional[str] = None): | |||
There was a problem hiding this comment.
Update the docstring for generate_stats to include leaderboard_name, describe its purpose, and any behavior when it's omitted.
| def generate_stats(self, last_day: bool, leaderboard_name: Optional[str] = None): | |
| def generate_stats(self, last_day: bool, leaderboard_name: Optional[str] = None): | |
| """ | |
| Generate statistics for submissions and runs. | |
| Args: | |
| last_day (bool): If True, only include data from the last 24 hours. | |
| leaderboard_name (Optional[str]): The name of the leaderboard to filter statistics for. | |
| If omitted, statistics are generated for all leaderboards. | |
| Returns: | |
| dict: A dictionary containing the generated statistics. | |
| """ |
Copilot uses AI. Check for mistakes.
| except Exception as e: | ||
| logging.exception("error generating stats", exc_info=e) | ||
| raise | ||
|
|
||
| def _generate_runner_stats(self, last_day: bool = False): | ||
| def _generate_runner_stats(self, last_day: bool = False, leaderboard_name: Optional[str] = None): |
There was a problem hiding this comment.
[nitpick] The leaderboard filter logic is duplicated across multiple methods; consider extracting a helper to build and append the WHERE/AND clauses.
Copilot uses AI. Check for mistakes.
| @discord.app_commands.describe(last_day_only="Only show stats for the last day") | ||
| async def show_bot_stats(self, interaction: discord.Interaction, last_day_only: bool): | ||
| @discord.app_commands.describe(leaderboard_name="Name of the leaderboard") | ||
| @app_commands.autocomplete(leaderboard_name=leaderboard_name_autocomplete) | ||
| async def show_bot_stats(self, interaction: discord.Interaction, last_day_only: bool, leaderboard_name: Optional[str] = None): |
There was a problem hiding this comment.
[nitpick] Consider renaming last_day_only to last_day (or vice versa) to match the database method’s parameter name and improve consistency.
Copilot uses AI. Check for mistakes.
|
So main reason I'm pausing on this is because I think we should probably just create a table with admin stats and then we can have significantly simpler queries for both the per leaderboard stats and the per day stats. RIght now there's tons of code duplication between our sql queries |
This is a pretty simple pr that adds an lb option to show-stats. Right now it just shows average runtime as an extra stat, but I'll add a baseline time in a future pr (this is involved enough I wanted to break it out)
Here's a pic of stats. First one is the default and the second is for a single leaderboard.